[PATCH] IB/iser: Handle lack of memory management extentions correctly

[PATCH] IB/iser: Handle lack of memory management extentions correctly

[Mike Marciniszyn]

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-+32LHaKS/h5VtLMLtJdGzA@public.gmane.org>
Fixes: Commit df749cdc45d9 ("IB/iser: Support up to 8MB data transfer in a single command")
Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: Handle lack of memory management extentions correctly

[Sagi Grimberg]

Hi Mike, for the patch itself, it addresses a bug so,

Acked-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] IB/iser: Handle lack of memory management extentions correctly

[Marciniszyn, Mike]

> 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

Re: [PATCH] IB/iser: Handle lack of memory management extentions correctly

[Thomas Rosenstein]

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-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
>>
>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] IB/iser: Handle lack of memory management extentions correctly

[Marciniszyn, Mike]

> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: Handle lack of memory management extentions correctly

[Sagi Grimberg]

> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: Handle lack of memory management extentions correctly

[Thomas Rosenstein]

Tested-by: Thomas Rosenstein <thomas.rosenstein-+32LHaKS/h5VtLMLtJdGzA@public.gmane.org>

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-+32LHaKS/h5VtLMLtJdGzA@public.gmane.org>
> Fixes: Commit df749cdc45d9 ("IB/iser: Support up to 8MB data transfer 
> in a single command")
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: Handle lack of memory management extentions correctly

[Sagi Grimberg]

>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: Handle lack of memory management extentions correctly

[Sagi Grimberg]

> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] IB/iser: Handle lack of memory management extentions correctly

[Marciniszyn, Mike]

> 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

Re: [PATCH] IB/iser: Handle lack of memory management extentions correctly

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 905 bytes --]

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-+32LHaKS/h5VtLMLtJdGzA@public.gmane.org>
> Fixes: Commit df749cdc45d9 ("IB/iser: Support up to 8MB data transfer in a single command")
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

This was accepted in 4.13-rc, thanks.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]