| Message ID | 20170622190110.27788.19910.stgit@phlsvslse11.ph.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hi Mike, for the patch itself, it addresses a bug so,
Acked-by: Sagi Grimberg <sagi@grimberg.me>
But,
As for not having any max pages for FMR it's really annoying.
What do you think about exposing max_fast_reg_page_list_len to
limit also for FMRs? We can also rename it to max_reg_page_list_len
to get rid of the FRWR association.
Thoughts?
--
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
> Hi Mike, for the patch itself, it addresses a bug so, > > Acked-by: Sagi Grimberg <sagi@grimberg.me> > > But, > > As for not having any max pages for FMR it's really annoying. > > What do you think about exposing max_fast_reg_page_list_len to > limit also for FMRs? We can also rename it to max_reg_page_list_len > to get rid of the FRWR association. > > Thoughts? I have no issue with your proposal, but I would rather get this fixed and adjust the API with an additional fix. I suspect we probably want to mark this fix stable. I did audit the kernel ULPs for FMR vs. FRMR usage and the only one that looked at max_fast_reg_page_list_len unconditionally was iser. Thomas, can you verify this patch and reply with a Tested-by: Meanwhile, I will audit kernel providers for which ones besides qib do not support the extensions and FMR. Mike
Hi Mike, will take a look at it tomorrow, on which kernel version is the patch based? Thomas On 28 Jun 2017, at 19:46, Marciniszyn, Mike wrote: >> Hi Mike, for the patch itself, it addresses a bug so, >> >> Acked-by: Sagi Grimberg <sagi@grimberg.me> >> >> But, >> >> As for not having any max pages for FMR it's really annoying. >> >> What do you think about exposing max_fast_reg_page_list_len to >> limit also for FMRs? We can also rename it to max_reg_page_list_len >> to get rid of the FRWR association. >> >> Thoughts? > > I have no issue with your proposal, but I would rather get this fixed > and adjust the API with an additional fix. I suspect we probably > want to mark this fix stable. > > I did audit the kernel ULPs for FMR vs. FRMR usage and the only one > that looked at max_fast_reg_page_list_len unconditionally was iser. > > Thomas, can you verify this patch and reply with a Tested-by: > > Meanwhile, I will audit kernel providers for which ones besides qib do > not support the extensions and FMR. > > Mike -- 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
> Hi Mike, > > will take a look at it tomorrow, on which kernel version is the patch > based? > > Thomas > The latest 4.12-rc should be fine. Mike -- 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
> I have no issue with your proposal, but I would rather get this fixed and adjust the API with an additional fix. I suspect we probably want to mark this fix stable. I agree, we should get it in and fix the API incrementally. > I did audit the kernel ULPs for FMR vs. FRMR usage and the only one that looked at max_fast_reg_page_list_len unconditionally was iser. Correct, but it doesn't mean that others get it right. nfs uses a hard coded value without even knowing what the device is capable of, and srp calculates it from max_mr_size which is a user-space mr capability and has nothing to do with fmrs. So no one got it right, which is not surprising as we don't have a cap for it. -- 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
Tested-by: Thomas Rosenstein <thomas.rosenstein@creamfinance.com> On 22 Jun 2017, at 21:01, Mike Marciniszyn wrote: > max_fast_reg_page_list_len is only valid when the > memory management extentions are signaled by the underlying > driver. > > Fix by adjusting iser_calc_scsi_params() to use > ISCSI_ISER_MAX_SG_TABLESIZE when the extentions are not indicated. > > Reported-by: Thomas Rosenstein <thomas.rosenstein@creamfinance.com> > Fixes: Commit df749cdc45d9 ("IB/iser: Support up to 8MB data transfer > in a single command") > Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com> > Signed-off-by: Mike Marciniszyn <mike.marciniszyn@intel.com> > --- > drivers/infiniband/ulp/iser/iser_verbs.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c > b/drivers/infiniband/ulp/iser/iser_verbs.c > index c538a38..26a004e 100644 > --- a/drivers/infiniband/ulp/iser/iser_verbs.c > +++ b/drivers/infiniband/ulp/iser/iser_verbs.c > @@ -708,8 +708,14 @@ static void iser_connect_error(struct rdma_cm_id > *cma_id) > unsigned short sg_tablesize, sup_sg_tablesize; > > sg_tablesize = DIV_ROUND_UP(max_sectors * 512, SIZE_4K); > - sup_sg_tablesize = min_t(unsigned, ISCSI_ISER_MAX_SG_TABLESIZE, > - device->ib_device->attrs.max_fast_reg_page_list_len); > + if (device->ib_device->attrs.device_cap_flags & > + IB_DEVICE_MEM_MGT_EXTENSIONS) > + sup_sg_tablesize = > + min_t( > + uint, ISCSI_ISER_MAX_SG_TABLESIZE, > + device->ib_device->attrs.max_fast_reg_page_list_len); > + else > + sup_sg_tablesize = ISCSI_ISER_MAX_SG_TABLESIZE; > > iser_conn->scsi_sg_tablesize = min(sg_tablesize, sup_sg_tablesize); > } > > -- > 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 -- 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
>> As for not having any max pages for FMR it's really annoying. >> >> What do you think about exposing max_fast_reg_page_list_len to >> limit also for FMRs? We can also rename it to max_reg_page_list_len >> to get rid of the FRWR association. >> >> Thoughts? > > I have no issue with your proposal, but I would rather get this fixed and adjust the API with an additional fix. I suspect we probably want to mark this fix stable. > > I did audit the kernel ULPs for FMR vs. FRMR usage and the only one that looked at max_fast_reg_page_list_len unconditionally was iser. > > Thomas, can you verify this patch and reply with a Tested-by: > > Meanwhile, I will audit kernel providers for which ones besides qib do not support the extensions and FMR. So what is the page_list limitation for qib? I want to prepare some patches... -- 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
> So what is the page_list limitation for qib? I want to prepare some > patches... Ping? -- 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
> Subject: Re: [PATCH] IB/iser: Handle lack of memory management > extentions correctly > > > So what is the page_list limitation for qib? I want to prepare some > > patches... > Sorry, still catching up after a few days off. I would suggest UINT_MAX which matches hfi1 registration for fast reg now. Mike
On 6/22/2017 3:01 PM, Mike Marciniszyn wrote: > max_fast_reg_page_list_len is only valid when the > memory management extentions are signaled by the underlying > driver. > > Fix by adjusting iser_calc_scsi_params() to use > ISCSI_ISER_MAX_SG_TABLESIZE when the extentions are not indicated. > > Reported-by: Thomas Rosenstein <thomas.rosenstein@creamfinance.com> > Fixes: Commit df749cdc45d9 ("IB/iser: Support up to 8MB data transfer in a single command") > Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com> > Signed-off-by: Mike Marciniszyn <mike.marciniszyn@intel.com> This was accepted in 4.13-rc, thanks.
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c index c538a38..26a004e 100644 --- a/drivers/infiniband/ulp/iser/iser_verbs.c +++ b/drivers/infiniband/ulp/iser/iser_verbs.c @@ -708,8 +708,14 @@ static void iser_connect_error(struct rdma_cm_id *cma_id) unsigned short sg_tablesize, sup_sg_tablesize; sg_tablesize = DIV_ROUND_UP(max_sectors * 512, SIZE_4K); - sup_sg_tablesize = min_t(unsigned, ISCSI_ISER_MAX_SG_TABLESIZE, - device->ib_device->attrs.max_fast_reg_page_list_len); + if (device->ib_device->attrs.device_cap_flags & + IB_DEVICE_MEM_MGT_EXTENSIONS) + sup_sg_tablesize = + min_t( + uint, ISCSI_ISER_MAX_SG_TABLESIZE, + device->ib_device->attrs.max_fast_reg_page_list_len); + else + sup_sg_tablesize = ISCSI_ISER_MAX_SG_TABLESIZE; iser_conn->scsi_sg_tablesize = min(sg_tablesize, sup_sg_tablesize); }
max_fast_reg_page_list_len is only valid when the memory management extentions are signaled by the underlying driver. Fix by adjusting iser_calc_scsi_params() to use ISCSI_ISER_MAX_SG_TABLESIZE when the extentions are not indicated. Reported-by: Thomas Rosenstein <thomas.rosenstein@creamfinance.com> Fixes: Commit df749cdc45d9 ("IB/iser: Support up to 8MB data transfer in a single command") Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com> Signed-off-by: Mike Marciniszyn <mike.marciniszyn@intel.com> --- drivers/infiniband/ulp/iser/iser_verbs.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 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