netdev - Re: [PATCH rdma-next v2 2/3] RDMA/rw: Support threshold for registration vs scattering to local pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Oct 2019 08:07:55 -0700
From:   Bart Van Assche <bvanassche@....org>
To:     Leon Romanovsky <leon@...nel.org>,
        Doug Ledford <dledford@...hat.com>,
        Jason Gunthorpe <jgg@...lanox.com>,
        Christoph Hellwig <hch@...radead.org>
Cc:     Leon Romanovsky <leonro@...lanox.com>,
        RDMA mailing list <linux-rdma@...r.kernel.org>,
        Or Gerlitz <ogerlitz@...lanox.com>,
        Yamin Friedman <yaminf@...lanox.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        linux-netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH rdma-next v2 2/3] RDMA/rw: Support threshold for
 registration vs scattering to local pages

On 10/7/19 6:59 AM, Leon Romanovsky wrote:
>   /*
> - * Check if the device might use memory registration.  This is currently only
> - * true for iWarp devices. In the future we can hopefully fine tune this based
> - * on HCA driver input.
> + * Check if the device might use memory registration. This is currently
> + * true for iWarp devices and devices that have optimized SGL registration
> + * logic.
>    */

The following sentence in the above comment looks confusing to me: 
"Check if the device might use memory registration." That sentence 
suggests that the HCA decides whether or not to use memory registration. 
Isn't it the RDMA R/W code that decides whether or not to use memory 
registration?

> + * For RDMA READs we must use MRs on iWarp and can optionaly use them as an
> + * optimaztion otherwise.  Additionally we have a debug option to force usage
> + * of MRs to help testing this code path.

You may want to change "optionaly" into "optionally" and "optimaztion" 
into "optimization".

>   static inline bool rdma_rw_io_needs_mr(struct ib_device *dev, u8 port_num,
>   		enum dma_data_direction dir, int dma_nents)
>   {
> -	if (rdma_protocol_iwarp(dev, port_num) && dir == DMA_FROM_DEVICE)
> -		return true;
> +	if (dir == DMA_FROM_DEVICE) {
> +		if (rdma_protocol_iwarp(dev, port_num))
> +			return true;
> +		if (dev->attrs.max_sgl_rd && dma_nents > dev->attrs.max_sgl_rd)
> +			return true;
> +	}
>   	if (unlikely(rdma_rw_force_mr))
>   		return true;
>   	return false;

Should this function be renamed? The function name suggests if this 
function returns 'true' that using memory registration is mandatory. My 
understanding is if this function returns true for the mlx5 HCA that 
using memory registration improves performance but is not mandatory.

Thanks,

Bart.