| Message ID | 20150724161820.25617.63886.stgit@build2.ogc.int (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Jul 24, 2015 at 11:18:21AM -0500, Steve Wise wrote: > Currently the sg tablesize, which dictates fast register page list > depth to use, does not take into account the limits of the rdma device. > So adjust it once we discover the device fastreg max depth limit. Also > adjust the max_sectors based on the resulting sg tablesize. Huh. How does this relate to the max_page_list_len argument: struct ib_mr *ib_alloc_fast_reg_mr(struct ib_pd *pd, int max_page_list_len) Shouldn't max_fast_reg_page_list_len be checked during the above? Ie does this still make sense: drivers/infiniband/ulp/iser/iser_verbs.c: desc->data_mr = ib_alloc_fast_reg_mr(pd, ISCSI_ISER_SG_TABLESIZE + 1); ? The only ULP that checks this is SRP, so basically, all our ULPs are probably quietly broken? cxgb3 has a limit of 10 (!?!?!!) Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com] > Sent: Friday, July 24, 2015 11:41 AM > To: Steve Wise > Cc: dledford@redhat.com; infinipath@intel.com; sagig@mellanox.com; ogerlitz@mellanox.com; roid@mellanox.com; linux- > rdma@vger.kernel.org; eli@mellanox.com; target-devel@vger.kernel.org; linux-nfs@vger.kernel.org; bfields@fieldses.org > Subject: Re: [PATCH V6 1/9] RDMA/iser: Limit sg tablesize and max_sectors to device fastreg max depth > > On Fri, Jul 24, 2015 at 11:18:21AM -0500, Steve Wise wrote: > > Currently the sg tablesize, which dictates fast register page list > > depth to use, does not take into account the limits of the rdma device. > > So adjust it once we discover the device fastreg max depth limit. Also > > adjust the max_sectors based on the resulting sg tablesize. > > Huh. How does this relate to the max_page_list_len argument: > > struct ib_mr *ib_alloc_fast_reg_mr(struct ib_pd *pd, int max_page_list_len) > > Shouldn't max_fast_reg_page_list_len be checked during the above? > > Ie does this still make sense: > > drivers/infiniband/ulp/iser/iser_verbs.c: desc->data_mr = ib_alloc_fast_reg_mr(pd, ISCSI_ISER_SG_TABLESIZE + 1); > > ? > > The only ULP that checks this is SRP, so basically, all our ULPs are > probably quietly broken? cxgb3 has a limit of 10 (!?!?!!) > Yea seems like some drivers need to enforce this in ib_alloc_fast_reg_mr() as well as ib_alloc_fast_reg_page_list(), and ULPs need to not exceed the device max. I will fix iser to limit the mr and page_list allocation based on the device max. Steve. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 24, 2015 at 01:40:17PM -0500, Steve Wise wrote: > > Huh. How does this relate to the max_page_list_len argument: > > > > struct ib_mr *ib_alloc_fast_reg_mr(struct ib_pd *pd, int max_page_list_len) > > > > Shouldn't max_fast_reg_page_list_len be checked during the above? > > > > Ie does this still make sense: > > > > drivers/infiniband/ulp/iser/iser_verbs.c: desc->data_mr = ib_alloc_fast_reg_mr(pd, ISCSI_ISER_SG_TABLESIZE + 1); > > > > ? > > > > The only ULP that checks this is SRP, so basically, all our ULPs are > > probably quietly broken? cxgb3 has a limit of 10 (!?!?!!) > > > > Yea seems like some drivers need to enforce this in ib_alloc_fast_reg_mr() as well as ib_alloc_fast_reg_page_list(), and ULPs need > to not exceed the device max. Great, Sagi, can you incorporate that in your series so that ib_alloc_mr's max_entires is checked against max_fast_reg_page_list_len and EINVAL's if it is too great? Also, after your series can we drop ib_alloc_fast_reg_page_list, and then also the associated WR stuff? Thanks, Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/24/2015 9:40 PM, Steve Wise wrote: > > >> -----Original Message----- >> From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com] >> Sent: Friday, July 24, 2015 11:41 AM >> To: Steve Wise >> Cc: dledford@redhat.com; infinipath@intel.com; sagig@mellanox.com; ogerlitz@mellanox.com; roid@mellanox.com; linux- >> rdma@vger.kernel.org; eli@mellanox.com; target-devel@vger.kernel.org; linux-nfs@vger.kernel.org; bfields@fieldses.org >> Subject: Re: [PATCH V6 1/9] RDMA/iser: Limit sg tablesize and max_sectors to device fastreg max depth >> >> On Fri, Jul 24, 2015 at 11:18:21AM -0500, Steve Wise wrote: >>> Currently the sg tablesize, which dictates fast register page list >>> depth to use, does not take into account the limits of the rdma device. >>> So adjust it once we discover the device fastreg max depth limit. Also >>> adjust the max_sectors based on the resulting sg tablesize. >> >> Huh. How does this relate to the max_page_list_len argument: >> >> struct ib_mr *ib_alloc_fast_reg_mr(struct ib_pd *pd, int max_page_list_len) >> >> Shouldn't max_fast_reg_page_list_len be checked during the above? >> >> Ie does this still make sense: >> >> drivers/infiniband/ulp/iser/iser_verbs.c: desc->data_mr = ib_alloc_fast_reg_mr(pd, ISCSI_ISER_SG_TABLESIZE + 1); >> >> ? >> >> The only ULP that checks this is SRP, so basically, all our ULPs are >> probably quietly broken? cxgb3 has a limit of 10 (!?!?!!) >> > > Yea seems like some drivers need to enforce this in ib_alloc_fast_reg_mr() as well as ib_alloc_fast_reg_page_list(), and ULPs need > to not exceed the device max. > > I will fix iser to limit the mr and page_list allocation based on the device max. Steve, I have a patch that addresses this in the pipe. The patch is support for up to 8MB transfer size for 4.3 (hopefully). So no need for you to tackle this. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/24/2015 10:14 PM, Jason Gunthorpe wrote: > On Fri, Jul 24, 2015 at 01:40:17PM -0500, Steve Wise wrote: >>> Huh. How does this relate to the max_page_list_len argument: >>> >>> struct ib_mr *ib_alloc_fast_reg_mr(struct ib_pd *pd, int max_page_list_len) >>> >>> Shouldn't max_fast_reg_page_list_len be checked during the above? >>> >>> Ie does this still make sense: >>> >>> drivers/infiniband/ulp/iser/iser_verbs.c: desc->data_mr = ib_alloc_fast_reg_mr(pd, ISCSI_ISER_SG_TABLESIZE + 1); >>> >>> ? >>> >>> The only ULP that checks this is SRP, so basically, all our ULPs are >>> probably quietly broken? cxgb3 has a limit of 10 (!?!?!!) >>> >> >> Yea seems like some drivers need to enforce this in ib_alloc_fast_reg_mr() as well as ib_alloc_fast_reg_page_list(), and ULPs need >> to not exceed the device max. > > Great, Sagi, can you incorporate that in your series so that > ib_alloc_mr's max_entires is checked against > max_fast_reg_page_list_len and EINVAL's if it is too great? Yes. I'll take care of that. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c index 6a594aa..de8730d 100644 --- a/drivers/infiniband/ulp/iser/iscsi_iser.c +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c @@ -640,6 +640,15 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, SHOST_DIX_GUARD_CRC); } + /* + * Limit the sg_tablesize and max_sectors based on the device + * max fastreg page list length. + */ + shost->sg_tablesize = min_t(unsigned short, shost->sg_tablesize, + ib_conn->device->dev_attr.max_fast_reg_page_list_len); + shost->max_sectors = min_t(unsigned int, + 1024, (shost->sg_tablesize * PAGE_SIZE) >> 9); + if (iscsi_host_add(shost, ib_conn->device->ib_device->dma_device)) { mutex_unlock(&iser_conn->state_mutex);
Currently the sg tablesize, which dictates fast register page list depth to use, does not take into account the limits of the rdma device. So adjust it once we discover the device fastreg max depth limit. Also adjust the max_sectors based on the resulting sg tablesize. Signed-off-by: Steve Wise <swise@opengridcomputing.com> --- drivers/infiniband/ulp/iser/iscsi_iser.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html