linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Bart Van Assche <bvanassche@acm.org>
To: Leon Romanovsky <leon@kernel.org>,
	Doug Ledford <dledford@redhat.com>,
	Jason Gunthorpe <jgg@mellanox.com>,
	Christoph Hellwig <hch@infradead.org>
Cc: Leon Romanovsky <leonro@mellanox.com>,
	RDMA mailing list <linux-rdma@vger.kernel.org>,
	Or Gerlitz <ogerlitz@mellanox.com>,
	Yamin Friedman <yaminf@mellanox.com>,
	Saeed Mahameed <saeedm@mellanox.com>,
	linux-netdev <netdev@vger.kernel.org>
Subject: Re: [PATCH rdma-next v2 2/3] RDMA/rw: Support threshold for registration vs scattering to local pages
Date: Mon, 7 Oct 2019 08:07:55 -0700	[thread overview]
Message-ID: <c0105196-b0e4-854e-88ff-40f5ba2d4105@acm.org> (raw)
In-Reply-To: <20191007135933.12483-3-leon@kernel.org>

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.

  parent reply	other threads:[~2019-10-07 15:08 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-07 13:59 [PATCH rdma-next v2 0/3] Optimize SGL registration Leon Romanovsky
2019-10-07 13:59 ` [PATCH mlx5-next v2 1/3] net/mlx5: Expose optimal performance scatter entries capability Leon Romanovsky
2019-10-07 15:02   ` Bart Van Assche
2019-10-07 15:54     ` Leon Romanovsky
2019-10-08  5:55   ` Leon Romanovsky
2019-10-07 13:59 ` [PATCH rdma-next v2 2/3] RDMA/rw: Support threshold for registration vs scattering to local pages Leon Romanovsky
2019-10-07 15:00   ` Christoph Hellwig
2019-10-07 15:57     ` Leon Romanovsky
2019-10-07 15:07   ` Bart Van Assche [this message]
2019-10-07 16:03     ` Leon Romanovsky
2019-10-07 22:22       ` Bart Van Assche
2019-10-08  5:53         ` Leon Romanovsky
2019-10-07 15:12   ` Bart Van Assche
2019-10-07 15:58     ` Leon Romanovsky
2019-10-07 13:59 ` [PATCH rdma-next v2 3/3] RDMA/mlx5: Add capability for max sge to get optimized performance Leon Romanovsky
2019-10-15  8:15 ` [PATCH rdma-next v2 0/3] Optimize SGL registration Leon Romanovsky
2019-10-22 17:42 ` Jason Gunthorpe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c0105196-b0e4-854e-88ff-40f5ba2d4105@acm.org \
    --to=bvanassche@acm.org \
    --cc=dledford@redhat.com \
    --cc=hch@infradead.org \
    --cc=jgg@mellanox.com \
    --cc=leon@kernel.org \
    --cc=leonro@mellanox.com \
    --cc=linux-rdma@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=ogerlitz@mellanox.com \
    --cc=saeedm@mellanox.com \
    --cc=yaminf@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).