[PATCH] ib_core: Enable and expose force_mr module parameter

[PATCH] ib_core: Enable and expose force_mr module parameter

[Chuck Lever]

The fourth parameter of the module_param_named macro is a set of
file permissions. Passing 0 there means that module parameter is
not created and that adding "options ib_core force_mr=1" to a
modprobe.conf file has no effect.

The default setting of rdma_rw_force_mr continues to be 0, or false.

Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/rw.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
index dbfd854..1cc8f07 100644
--- a/drivers/infiniband/core/rw.c
+++ b/drivers/infiniband/core/rw.c
@@ -23,7 +23,7 @@ enum {
 };
 
 static bool rdma_rw_force_mr;
-module_param_named(force_mr, rdma_rw_force_mr, bool, 0);
+module_param_named(force_mr, rdma_rw_force_mr, bool, 0644);
 MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations");
 
 /*

--
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_core: Enable and expose force_mr module parameter

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]

On Mon, May 15, 2017 at 10:52:44AM -0400, Chuck Lever wrote:
> The fourth parameter of the module_param_named macro is a set of
> file permissions. Passing 0 there means that module parameter is
> not created and that adding "options ib_core force_mr=1" to a
> modprobe.conf file has no effect.
>
> The default setting of rdma_rw_force_mr continues to be 0, or false.
>
> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

Chuck,

Do we really need this parameter?

> ---
>  drivers/infiniband/core/rw.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
> index dbfd854..1cc8f07 100644
> --- a/drivers/infiniband/core/rw.c
> +++ b/drivers/infiniband/core/rw.c
> @@ -23,7 +23,7 @@ enum {
>  };
>
>  static bool rdma_rw_force_mr;
> -module_param_named(force_mr, rdma_rw_force_mr, bool, 0);
> +module_param_named(force_mr, rdma_rw_force_mr, bool, 0644);
>  MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations");
>
>  /*
>
> --
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ib_core: Enable and expose force_mr module parameter

[Chuck Lever]

> On May 15, 2017, at 10:57 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Mon, May 15, 2017 at 10:52:44AM -0400, Chuck Lever wrote:
>> The fourth parameter of the module_param_named macro is a set of
>> file permissions. Passing 0 there means that module parameter is
>> not created and that adding "options ib_core force_mr=1" to a
>> modprobe.conf file has no effect.
>> 
>> The default setting of rdma_rw_force_mr continues to be 0, or false.
>> 
>> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
>> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> Chuck,
> 
> Do we really need this parameter?

Hi Leon-

Christoph and Sagi introduced this with the new rdma_rw API.
The default behavior is to avoid the use of FRWR, even for
devices that can support FRWR but also work without. FRWR is
used only for iWARP devices, which require it; but FRWR can
have a (slight) negative performance impact, which is why it
is not the default.

The purpose of this module parameter is to enable the use of
FRWR for devices that can go either way, as a way to test the
FRWR capability when using those devices.

I'm not sure we _need_ to have the module parameter. But I've
certainly used it while developing the NFSD conversion to use
the rdma_rw API, since I have no iWARP devices here.


>> ---
>> drivers/infiniband/core/rw.c |    2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
>> index dbfd854..1cc8f07 100644
>> --- a/drivers/infiniband/core/rw.c
>> +++ b/drivers/infiniband/core/rw.c
>> @@ -23,7 +23,7 @@ enum {
>> };
>> 
>> static bool rdma_rw_force_mr;
>> -module_param_named(force_mr, rdma_rw_force_mr, bool, 0);
>> +module_param_named(force_mr, rdma_rw_force_mr, bool, 0644);
>> MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations");
>> 
>> /*
>> 
>> --
>> 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

--
Chuck Lever



--
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_core: Enable and expose force_mr module parameter

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2854 bytes --]

On Mon, May 15, 2017 at 11:09:06AM -0400, Chuck Lever wrote:
>
> > On May 15, 2017, at 10:57 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > On Mon, May 15, 2017 at 10:52:44AM -0400, Chuck Lever wrote:
> >> The fourth parameter of the module_param_named macro is a set of
> >> file permissions. Passing 0 there means that module parameter is
> >> not created and that adding "options ib_core force_mr=1" to a
> >> modprobe.conf file has no effect.
> >>
> >> The default setting of rdma_rw_force_mr continues to be 0, or false.
> >>
> >> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
> >> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> >
> > Chuck,
> >
> > Do we really need this parameter?
>
> Hi Leon-
>
> Christoph and Sagi introduced this with the new rdma_rw API.
> The default behavior is to avoid the use of FRWR, even for
> devices that can support FRWR but also work without. FRWR is
> used only for iWARP devices, which require it; but FRWR can
> have a (slight) negative performance impact, which is why it
> is not the default.
>
> The purpose of this module parameter is to enable the use of
> FRWR for devices that can go either way, as a way to test the
> FRWR capability when using those devices.
>
> I'm not sure we _need_ to have the module parameter. But I've
> certainly used it while developing the NFSD conversion to use
> the rdma_rw API, since I have no iWARP devices here.

I asked that question because commit a060b5629ab0 was added a year ago
and since then no one noticed that it is not usable, including authors.

I think that we can safely remove it and for development, we can set
rdma_rw_force_mr internally in the code.

Thanks

>
>
> >> ---
> >> drivers/infiniband/core/rw.c |    2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
> >> index dbfd854..1cc8f07 100644
> >> --- a/drivers/infiniband/core/rw.c
> >> +++ b/drivers/infiniband/core/rw.c
> >> @@ -23,7 +23,7 @@ enum {
> >> };
> >>
> >> static bool rdma_rw_force_mr;
> >> -module_param_named(force_mr, rdma_rw_force_mr, bool, 0);
> >> +module_param_named(force_mr, rdma_rw_force_mr, bool, 0644);
> >> MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations");
> >>
> >> /*
> >>
> >> --
> >> 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
>
> --
> Chuck Lever
>
>
>
> --
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ib_core: Enable and expose force_mr module parameter

[Chuck Lever]

> On May 15, 2017, at 1:00 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Mon, May 15, 2017 at 11:09:06AM -0400, Chuck Lever wrote:
>> 
>>> On May 15, 2017, at 10:57 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>> 
>>> On Mon, May 15, 2017 at 10:52:44AM -0400, Chuck Lever wrote:
>>>> The fourth parameter of the module_param_named macro is a set of
>>>> file permissions. Passing 0 there means that module parameter is
>>>> not created and that adding "options ib_core force_mr=1" to a
>>>> modprobe.conf file has no effect.
>>>> 
>>>> The default setting of rdma_rw_force_mr continues to be 0, or false.
>>>> 
>>>> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
>>>> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>>> 
>>> Chuck,
>>> 
>>> Do we really need this parameter?
>> 
>> Hi Leon-
>> 
>> Christoph and Sagi introduced this with the new rdma_rw API.
>> The default behavior is to avoid the use of FRWR, even for
>> devices that can support FRWR but also work without. FRWR is
>> used only for iWARP devices, which require it; but FRWR can
>> have a (slight) negative performance impact, which is why it
>> is not the default.
>> 
>> The purpose of this module parameter is to enable the use of
>> FRWR for devices that can go either way, as a way to test the
>> FRWR capability when using those devices.
>> 
>> I'm not sure we _need_ to have the module parameter. But I've
>> certainly used it while developing the NFSD conversion to use
>> the rdma_rw API, since I have no iWARP devices here.
> 
> I asked that question because commit a060b5629ab0 was added a year ago
> and since then no one noticed that it is not usable, including authors.

I noticed a while ago, but haven't had time to track it down
until now.


> I think that we can safely remove it and for development, we can set
> rdma_rw_force_mr internally in the code.

It is safe to remove. Since it was never exposed to user
space due to this bug, removal would not result in a kernel
API breakage.

I find the feature useful. But it is correct to say that such
development knobs are often left to kernel compile-time, as
it is not especially useful to end users.

In this case, the module parameter is global: one has to take
some care when there are multiple HCAs in a system and they
each have different MEMMGT capabilities. Instead of removing
the setting altogether, one might replace it with a driver-
mediated per-device-port attribute.

I will let Sagi and Christoph have the last word.


> Thanks
> 
>> 
>> 
>>>> ---
>>>> drivers/infiniband/core/rw.c |    2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
>>>> index dbfd854..1cc8f07 100644
>>>> --- a/drivers/infiniband/core/rw.c
>>>> +++ b/drivers/infiniband/core/rw.c
>>>> @@ -23,7 +23,7 @@ enum {
>>>> };
>>>> 
>>>> static bool rdma_rw_force_mr;
>>>> -module_param_named(force_mr, rdma_rw_force_mr, bool, 0);
>>>> +module_param_named(force_mr, rdma_rw_force_mr, bool, 0644);
>>>> MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations");
>>>> 
>>>> /*
>>>> 
>>>> --
>>>> 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
>> 
>> --
>> Chuck Lever
>> 
>> 
>> 
>> --
>> 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

--
Chuck Lever



--
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_core: Enable and expose force_mr module parameter

[Jason Gunthorpe]

On Mon, May 15, 2017 at 01:22:59PM -0400, Chuck Lever wrote:

> I find the feature useful. But it is correct to say that such
> development knobs are often left to kernel compile-time, as
> it is not especially useful to end users.

Put it in debugfs?

Jason
--
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_core: Enable and expose force_mr module parameter

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

On Mon, May 15, 2017 at 11:46:46AM -0600, Jason Gunthorpe wrote:
> On Mon, May 15, 2017 at 01:22:59PM -0400, Chuck Lever wrote:
>
> > I find the feature useful. But it is correct to say that such
> > development knobs are often left to kernel compile-time, as
> > it is not especially useful to end users.
>
> Put it in debugfs?

Why bother? In good year, we will have one new user of this RW API who
will need that knob during development phase.

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ib_core: Enable and expose force_mr module parameter

[Christoph Hellwig]

On Mon, May 15, 2017 at 01:22:59PM -0400, Chuck Lever wrote:
> In this case, the module parameter is global: one has to take
> some care when there are multiple HCAs in a system and they
> each have different MEMMGT capabilities. Instead of removing
> the setting altogether, one might replace it with a driver-
> mediated per-device-port attribute.
> 
> I will let Sagi and Christoph have the last word.

I think I only added it on requests from someone (you?), and when
writing the code debugged it by manually forcing it to on.  Thay
being said a module parameter is entirely trivial and thus I'm fine
with it.  Think like debugfs or per-device setting for such a trivial
debug tool have just way to much boilerplate code.
--
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_core: Enable and expose force_mr module parameter

[Sagi Grimberg]

> I think I only added it on requests from someone (you?), and when
> writing the code debugged it by manually forcing it to on.  Thay
> being said a module parameter is entirely trivial and thus I'm fine
> with it.  Think like debugfs or per-device setting for such a trivial
> debug tool have just way to much boilerplate code.

I tend to agree. It's a very simple knob, can't imagine why we'd want it
to be per-device.
--
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_core: Enable and expose force_mr module parameter

[Chuck Lever]

> On May 16, 2017, at 3:22 AM, Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org> wrote:
> 
> 
>> I think I only added it on requests from someone (you?), and when
>> writing the code debugged it by manually forcing it to on.  Thay
>> being said a module parameter is entirely trivial and thus I'm fine
>> with it.  Think like debugfs or per-device setting for such a trivial
>> debug tool have just way to much boilerplate code.

Agree that debugfs would work, but might be overkill.


> I tend to agree. It's a very simple knob, can't imagine why we'd want it
> to be per-device.

Per-device port ... The internal helper includes @port_num as a
parameter:

static inline bool rdma_rw_can_use_mr(struct ib_device *dev, u8 port_num)

IIRC the idea was to eventually have a white list of devices
(besides iWARP devices, which require it) that prefer FRWR. And
some modes of operation (VF, or split RoCE-IB) might want to
force FRWR or not. See core/rw.c:

 48  * XXX: In the future we can hopefully fine tune this based on HCA driver
 49  * input.

The original impetus was to have a simple method of forcing the
use of FRWR so we could study the performance differences. When I
started using rdma_rw for NFSD, however, it was clear that there
were different constraints on ULP behavior depending on whether
FRWR was used or not.

However, I'm not stuck on keeping it. It's certainly not hard to
build a patched kernel that forces the use of FRWR, when I need
to test specific code paths.


--
Chuck Lever



--
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

[PATCH] ib_core: Enable and expose force_mr module parameter

[Chuck Lever]

The fourth parameter of the module_param_named macro is a set of
file permissions. Passing 0 there means that module parameter is
not created and that adding "options ib_core force_mr=1" to a
modprobe.conf file has no effect.

The default setting of rdma_rw_force_mr continues to be 0, or false.

Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
Hi Doug-

This doesn't seem appropriate to go through Bruce's tree for 4.13.

Last discussion didn't seem to conclude with full consensus.
Probably people don't care enough one way or another. But I'd like
to see this get fixed if there aren't strong objections. Would you
take it for 4.13?


 drivers/infiniband/core/rw.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
index dbfd854..1cc8f07 100644
--- a/drivers/infiniband/core/rw.c
+++ b/drivers/infiniband/core/rw.c
@@ -23,7 +23,7 @@ enum {
 };
 
 static bool rdma_rw_force_mr;
-module_param_named(force_mr, rdma_rw_force_mr, bool, 0);
+module_param_named(force_mr, rdma_rw_force_mr, bool, 0644);
 MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations");
 
 /*

--
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_core: Enable and expose force_mr module parameter

[Christoph Hellwig]

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>

(it's a nice and cheap debug tool!)
--
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_core: Enable and expose force_mr module parameter

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
--
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_core: Enable and expose force_mr module parameter

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1798 bytes --]

On Mon, Jun 19, 2017 at 11:26:40AM -0400, Chuck Lever wrote:
> The fourth parameter of the module_param_named macro is a set of
> file permissions. Passing 0 there means that module parameter is
> not created and that adding "options ib_core force_mr=1" to a
> modprobe.conf file has no effect.
>
> The default setting of rdma_rw_force_mr continues to be 0, or false.
>
> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
> Hi Doug-
>
> This doesn't seem appropriate to go through Bruce's tree for 4.13.
>
> Last discussion didn't seem to conclude with full consensus.
> Probably people don't care enough one way or another. But I'd like
> to see this get fixed if there aren't strong objections. Would you
> take it for 4.13?

I care and believe that it should be removed from exposure to users.
The variable force_mr should be leaved in the code for the debug,
but module_param should be removed.

Thanks


>
>
>  drivers/infiniband/core/rw.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
> index dbfd854..1cc8f07 100644
> --- a/drivers/infiniband/core/rw.c
> +++ b/drivers/infiniband/core/rw.c
> @@ -23,7 +23,7 @@ enum {
>  };
>
>  static bool rdma_rw_force_mr;
> -module_param_named(force_mr, rdma_rw_force_mr, bool, 0);
> +module_param_named(force_mr, rdma_rw_force_mr, bool, 0644);
>  MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations");
>
>  /*
>
> --
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ib_core: Enable and expose force_mr module parameter

[Chuck Lever]

Hi Leon-


> On Jun 20, 2017, at 3:32 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Mon, Jun 19, 2017 at 11:26:40AM -0400, Chuck Lever wrote:
>> The fourth parameter of the module_param_named macro is a set of
>> file permissions. Passing 0 there means that module parameter is
>> not created and that adding "options ib_core force_mr=1" to a
>> modprobe.conf file has no effect.
>> 
>> The default setting of rdma_rw_force_mr continues to be 0, or false.
>> 
>> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
>> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>> ---
>> Hi Doug-
>> 
>> This doesn't seem appropriate to go through Bruce's tree for 4.13.
>> 
>> Last discussion didn't seem to conclude with full consensus.
>> Probably people don't care enough one way or another. But I'd like
>> to see this get fixed if there aren't strong objections. Would you
>> take it for 4.13?
> 
> I care and believe that it should be removed from exposure to users.
> The variable force_mr should be leaved in the code for the debug,
> but module_param should be removed.

I don't understand the technical grounds of your objection.
Why is

 options mlx4_core internal_err_reset=0

acceptable, but

 options ib_core force_mr=1

not?


> Thanks
> 
> 
>> 
>> 
>> drivers/infiniband/core/rw.c |    2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
>> index dbfd854..1cc8f07 100644
>> --- a/drivers/infiniband/core/rw.c
>> +++ b/drivers/infiniband/core/rw.c
>> @@ -23,7 +23,7 @@ enum {
>> };
>> 
>> static bool rdma_rw_force_mr;
>> -module_param_named(force_mr, rdma_rw_force_mr, bool, 0);
>> +module_param_named(force_mr, rdma_rw_force_mr, bool, 0644);
>> MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations");
>> 
>> /*
>> 
>> --
>> 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

--
Chuck Lever



--
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_core: Enable and expose force_mr module parameter

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]

On Tue, Jun 20, 2017 at 11:43:56AM -0400, Chuck Lever wrote:
> Hi Leon-
>
>
> > On Jun 20, 2017, at 3:32 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > On Mon, Jun 19, 2017 at 11:26:40AM -0400, Chuck Lever wrote:
> >> The fourth parameter of the module_param_named macro is a set of
> >> file permissions. Passing 0 there means that module parameter is
> >> not created and that adding "options ib_core force_mr=1" to a
> >> modprobe.conf file has no effect.
> >>
> >> The default setting of rdma_rw_force_mr continues to be 0, or false.
> >>
> >> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
> >> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> >> ---
> >> Hi Doug-
> >>
> >> This doesn't seem appropriate to go through Bruce's tree for 4.13.
> >>
> >> Last discussion didn't seem to conclude with full consensus.
> >> Probably people don't care enough one way or another. But I'd like
> >> to see this get fixed if there aren't strong objections. Would you
> >> take it for 4.13?
> >
> > I care and believe that it should be removed from exposure to users.
> > The variable force_mr should be leaved in the code for the debug,
> > but module_param should be removed.
>
> I don't understand the technical grounds of your objection.
> Why is
>
>  options mlx4_core internal_err_reset=0
>
> acceptable, but
>
>  options ib_core force_mr=1
>
> not?

That mlx4 variable was exposed before I started to follow after
Mellanox's submissions.

The force_mr parameter is for debug of new ULP and/or conversion of
existing ULP to RW API. This is not very common situation and it is
for the developers and not for the users.

Thanks

>
>
> > Thanks
> >
> >
> >>
> >>
> >> drivers/infiniband/core/rw.c |    2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
> >> index dbfd854..1cc8f07 100644
> >> --- a/drivers/infiniband/core/rw.c
> >> +++ b/drivers/infiniband/core/rw.c
> >> @@ -23,7 +23,7 @@ enum {
> >> };
> >>
> >> static bool rdma_rw_force_mr;
> >> -module_param_named(force_mr, rdma_rw_force_mr, bool, 0);
> >> +module_param_named(force_mr, rdma_rw_force_mr, bool, 0644);
> >> MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations");
> >>
> >> /*
> >>
> >> --
> >> 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
>
> --
> Chuck Lever
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ib_core: Enable and expose force_mr module parameter

[Chuck Lever]

> On Jun 20, 2017, at 2:03 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Tue, Jun 20, 2017 at 11:43:56AM -0400, Chuck Lever wrote:
>> Hi Leon-
>> 
>> 
>>> On Jun 20, 2017, at 3:32 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>> 
>>> On Mon, Jun 19, 2017 at 11:26:40AM -0400, Chuck Lever wrote:
>>>> The fourth parameter of the module_param_named macro is a set of
>>>> file permissions. Passing 0 there means that module parameter is
>>>> not created and that adding "options ib_core force_mr=1" to a
>>>> modprobe.conf file has no effect.
>>>> 
>>>> The default setting of rdma_rw_force_mr continues to be 0, or false.
>>>> 
>>>> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
>>>> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>> Hi Doug-
>>>> 
>>>> This doesn't seem appropriate to go through Bruce's tree for 4.13.
>>>> 
>>>> Last discussion didn't seem to conclude with full consensus.
>>>> Probably people don't care enough one way or another. But I'd like
>>>> to see this get fixed if there aren't strong objections. Would you
>>>> take it for 4.13?
>>> 
>>> I care and believe that it should be removed from exposure to users.
>>> The variable force_mr should be leaved in the code for the debug,
>>> but module_param should be removed.
>> 
>> I don't understand the technical grounds of your objection.
>> Why is
>> 
>> options mlx4_core internal_err_reset=0
>> 
>> acceptable, but
>> 
>> options ib_core force_mr=1
>> 
>> not?
> 
> That mlx4 variable was exposed before I started to follow after
> Mellanox's submissions.
> 
> The force_mr parameter is for debug of new ULP and/or conversion of
> existing ULP to RW API. This is not very common situation and it is
> for the developers and not for the users.

No disagreement on that, but that's still not an argument
not to expose it. What is the risk, as you see it?


> Thanks
> 
>> 
>> 
>>> Thanks
>>> 
>>> 
>>>> 
>>>> 
>>>> drivers/infiniband/core/rw.c |    2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
>>>> index dbfd854..1cc8f07 100644
>>>> --- a/drivers/infiniband/core/rw.c
>>>> +++ b/drivers/infiniband/core/rw.c
>>>> @@ -23,7 +23,7 @@ enum {
>>>> };
>>>> 
>>>> static bool rdma_rw_force_mr;
>>>> -module_param_named(force_mr, rdma_rw_force_mr, bool, 0);
>>>> +module_param_named(force_mr, rdma_rw_force_mr, bool, 0644);
>>>> MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations");
>>>> 
>>>> /*
>>>> 
>>>> --
>>>> 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
>> 
>> --
>> Chuck Lever

--
Chuck Lever



--
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_core: Enable and expose force_mr module parameter

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3356 bytes --]

On Tue, Jun 20, 2017 at 02:23:21PM -0400, Chuck Lever wrote:
>
> > On Jun 20, 2017, at 2:03 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > On Tue, Jun 20, 2017 at 11:43:56AM -0400, Chuck Lever wrote:
> >> Hi Leon-
> >>
> >>
> >>> On Jun 20, 2017, at 3:32 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >>>
> >>> On Mon, Jun 19, 2017 at 11:26:40AM -0400, Chuck Lever wrote:
> >>>> The fourth parameter of the module_param_named macro is a set of
> >>>> file permissions. Passing 0 there means that module parameter is
> >>>> not created and that adding "options ib_core force_mr=1" to a
> >>>> modprobe.conf file has no effect.
> >>>>
> >>>> The default setting of rdma_rw_force_mr continues to be 0, or false.
> >>>>
> >>>> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
> >>>> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> >>>> ---
> >>>> Hi Doug-
> >>>>
> >>>> This doesn't seem appropriate to go through Bruce's tree for 4.13.
> >>>>
> >>>> Last discussion didn't seem to conclude with full consensus.
> >>>> Probably people don't care enough one way or another. But I'd like
> >>>> to see this get fixed if there aren't strong objections. Would you
> >>>> take it for 4.13?
> >>>
> >>> I care and believe that it should be removed from exposure to users.
> >>> The variable force_mr should be leaved in the code for the debug,
> >>> but module_param should be removed.
> >>
> >> I don't understand the technical grounds of your objection.
> >> Why is
> >>
> >> options mlx4_core internal_err_reset=0
> >>
> >> acceptable, but
> >>
> >> options ib_core force_mr=1
> >>
> >> not?
> >
> > That mlx4 variable was exposed before I started to follow after
> > Mellanox's submissions.
> >
> > The force_mr parameter is for debug of new ULP and/or conversion of
> > existing ULP to RW API. This is not very common situation and it is
> > for the developers and not for the users.
>
> No disagreement on that, but that's still not an argument
> not to expose it. What is the risk, as you see it?

It will be "marked" as UAPI and we won't be able to change it,
which IMHO bad for the debug. It also invites users to set it
and in perfect world, we should test our HCAs with this flag
on and off, because curious user can set it.

Thanks

>
>
> > Thanks
> >
> >>
> >>
> >>> Thanks
> >>>
> >>>
> >>>>
> >>>>
> >>>> drivers/infiniband/core/rw.c |    2 +-
> >>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
> >>>> index dbfd854..1cc8f07 100644
> >>>> --- a/drivers/infiniband/core/rw.c
> >>>> +++ b/drivers/infiniband/core/rw.c
> >>>> @@ -23,7 +23,7 @@ enum {
> >>>> };
> >>>>
> >>>> static bool rdma_rw_force_mr;
> >>>> -module_param_named(force_mr, rdma_rw_force_mr, bool, 0);
> >>>> +module_param_named(force_mr, rdma_rw_force_mr, bool, 0644);
> >>>> MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations");
> >>>>
> >>>> /*
> >>>>
> >>>> --
> >>>> 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
> >>
> >> --
> >> Chuck Lever
>
> --
> Chuck Lever
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ib_core: Enable and expose force_mr module parameter

[Chuck Lever]

[ Removing Doug since all mail to him seems to be bouncing ]

> On Jun 20, 2017, at 2:40 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Tue, Jun 20, 2017 at 02:23:21PM -0400, Chuck Lever wrote:
>> 
>>> On Jun 20, 2017, at 2:03 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>> 
>>> On Tue, Jun 20, 2017 at 11:43:56AM -0400, Chuck Lever wrote:
>>>> Hi Leon-
>>>> 
>>>> 
>>>>> On Jun 20, 2017, at 3:32 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>>>> 
>>>>> On Mon, Jun 19, 2017 at 11:26:40AM -0400, Chuck Lever wrote:
>>>>>> The fourth parameter of the module_param_named macro is a set of
>>>>>> file permissions. Passing 0 there means that module parameter is
>>>>>> not created and that adding "options ib_core force_mr=1" to a
>>>>>> modprobe.conf file has no effect.
>>>>>> 
>>>>>> The default setting of rdma_rw_force_mr continues to be 0, or false.
>>>>>> 
>>>>>> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
>>>>>> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>>>>>> ---
>>>>>> Hi Doug-
>>>>>> 
>>>>>> This doesn't seem appropriate to go through Bruce's tree for 4.13.
>>>>>> 
>>>>>> Last discussion didn't seem to conclude with full consensus.
>>>>>> Probably people don't care enough one way or another. But I'd like
>>>>>> to see this get fixed if there aren't strong objections. Would you
>>>>>> take it for 4.13?
>>>>> 
>>>>> I care and believe that it should be removed from exposure to users.
>>>>> The variable force_mr should be leaved in the code for the debug,
>>>>> but module_param should be removed.
>>>> 
>>>> I don't understand the technical grounds of your objection.
>>>> Why is
>>>> 
>>>> options mlx4_core internal_err_reset=0
>>>> 
>>>> acceptable, but
>>>> 
>>>> options ib_core force_mr=1
>>>> 
>>>> not?
>>> 
>>> That mlx4 variable was exposed before I started to follow after
>>> Mellanox's submissions.
>>> 
>>> The force_mr parameter is for debug of new ULP and/or conversion of
>>> existing ULP to RW API. This is not very common situation and it is
>>> for the developers and not for the users.
>> 
>> No disagreement on that, but that's still not an argument
>> not to expose it. What is the risk, as you see it?
> 
> It will be "marked" as UAPI and we won't be able to change it,
> which IMHO bad for the debug. It also invites users to set it
> and in perfect world, we should test our HCAs with this flag
> on and off, because curious user can set it.

We have loads of legacy debugging interfaces. 

And yes, ULP developers should be aware of it and test with
both settings.

I'm not convinced any of this is a problem for this particular
setting. However, I guess the thing for you to do is propose a
patch.


> Thanks
> 
>> 
>> 
>>> Thanks
>>> 
>>>> 
>>>> 
>>>>> Thanks
>>>>> 
>>>>> 
>>>>>> 
>>>>>> 
>>>>>> drivers/infiniband/core/rw.c |    2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>> 
>>>>>> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
>>>>>> index dbfd854..1cc8f07 100644
>>>>>> --- a/drivers/infiniband/core/rw.c
>>>>>> +++ b/drivers/infiniband/core/rw.c
>>>>>> @@ -23,7 +23,7 @@ enum {
>>>>>> };
>>>>>> 
>>>>>> static bool rdma_rw_force_mr;
>>>>>> -module_param_named(force_mr, rdma_rw_force_mr, bool, 0);
>>>>>> +module_param_named(force_mr, rdma_rw_force_mr, bool, 0644);
>>>>>> MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations");
>>>>>> 
>>>>>> /*
>>>>>> 
>>>>>> --
>>>>>> 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
>>>> 
>>>> --
>>>> Chuck Lever
>> 
>> --
>> Chuck Lever
>> 
>> 
>> 

--
Chuck Lever



--
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_core: Enable and expose force_mr module parameter

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 4163 bytes --]

On Wed, Jun 21, 2017 at 10:46:44AM -0400, Chuck Lever wrote:
> [ Removing Doug since all mail to him seems to be bouncing ]
>
> > On Jun 20, 2017, at 2:40 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > On Tue, Jun 20, 2017 at 02:23:21PM -0400, Chuck Lever wrote:
> >>
> >>> On Jun 20, 2017, at 2:03 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >>>
> >>> On Tue, Jun 20, 2017 at 11:43:56AM -0400, Chuck Lever wrote:
> >>>> Hi Leon-
> >>>>
> >>>>
> >>>>> On Jun 20, 2017, at 3:32 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >>>>>
> >>>>> On Mon, Jun 19, 2017 at 11:26:40AM -0400, Chuck Lever wrote:
> >>>>>> The fourth parameter of the module_param_named macro is a set of
> >>>>>> file permissions. Passing 0 there means that module parameter is
> >>>>>> not created and that adding "options ib_core force_mr=1" to a
> >>>>>> modprobe.conf file has no effect.
> >>>>>>
> >>>>>> The default setting of rdma_rw_force_mr continues to be 0, or false.
> >>>>>>
> >>>>>> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
> >>>>>> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> >>>>>> ---
> >>>>>> Hi Doug-
> >>>>>>
> >>>>>> This doesn't seem appropriate to go through Bruce's tree for 4.13.
> >>>>>>
> >>>>>> Last discussion didn't seem to conclude with full consensus.
> >>>>>> Probably people don't care enough one way or another. But I'd like
> >>>>>> to see this get fixed if there aren't strong objections. Would you
> >>>>>> take it for 4.13?
> >>>>>
> >>>>> I care and believe that it should be removed from exposure to users.
> >>>>> The variable force_mr should be leaved in the code for the debug,
> >>>>> but module_param should be removed.
> >>>>
> >>>> I don't understand the technical grounds of your objection.
> >>>> Why is
> >>>>
> >>>> options mlx4_core internal_err_reset=0
> >>>>
> >>>> acceptable, but
> >>>>
> >>>> options ib_core force_mr=1
> >>>>
> >>>> not?
> >>>
> >>> That mlx4 variable was exposed before I started to follow after
> >>> Mellanox's submissions.
> >>>
> >>> The force_mr parameter is for debug of new ULP and/or conversion of
> >>> existing ULP to RW API. This is not very common situation and it is
> >>> for the developers and not for the users.
> >>
> >> No disagreement on that, but that's still not an argument
> >> not to expose it. What is the risk, as you see it?
> >
> > It will be "marked" as UAPI and we won't be able to change it,
> > which IMHO bad for the debug. It also invites users to set it
> > and in perfect world, we should test our HCAs with this flag
> > on and off, because curious user can set it.
>
> We have loads of legacy debugging interfaces.
>
> And yes, ULP developers should be aware of it and test with
> both settings.
>
> I'm not convinced any of this is a problem for this particular
> setting. However, I guess the thing for you to do is propose a
> patch.

Thanks,
I'll do it with great pleasure.

>
>
> > Thanks
> >
> >>
> >>
> >>> Thanks
> >>>
> >>>>
> >>>>
> >>>>> Thanks
> >>>>>
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> drivers/infiniband/core/rw.c |    2 +-
> >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
> >>>>>> index dbfd854..1cc8f07 100644
> >>>>>> --- a/drivers/infiniband/core/rw.c
> >>>>>> +++ b/drivers/infiniband/core/rw.c
> >>>>>> @@ -23,7 +23,7 @@ enum {
> >>>>>> };
> >>>>>>
> >>>>>> static bool rdma_rw_force_mr;
> >>>>>> -module_param_named(force_mr, rdma_rw_force_mr, bool, 0);
> >>>>>> +module_param_named(force_mr, rdma_rw_force_mr, bool, 0644);
> >>>>>> MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations");
> >>>>>>
> >>>>>> /*
> >>>>>>
> >>>>>> --
> >>>>>> 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
> >>>>
> >>>> --
> >>>> Chuck Lever
> >>
> >> --
> >> Chuck Lever
> >>
> >>
> >>
>
> --
> Chuck Lever
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ib_core: Enable and expose force_mr module parameter

[Sagi Grimberg]

>> It will be "marked" as UAPI and we won't be able to change it,
>> which IMHO bad for the debug. It also invites users to set it
>> and in perfect world, we should test our HCAs with this flag
>> on and off, because curious user can set it.
> 
> We have loads of legacy debugging interfaces.
> 
> And yes, ULP developers should be aware of it and test with
> both settings.
> 
> I'm not convinced any of this is a problem for this particular
> setting. However, I guess the thing for you to do is propose a
> patch.

Are you guys seriously still on this? Its simply a trivial
debug flag, whats all the fuss really about?
--
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_core: Enable and expose force_mr module parameter

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]

On Tue, Jun 27, 2017 at 12:24:20PM +0300, Sagi Grimberg wrote:
>
> > > It will be "marked" as UAPI and we won't be able to change it,
> > > which IMHO bad for the debug. It also invites users to set it
> > > and in perfect world, we should test our HCAs with this flag
> > > on and off, because curious user can set it.
> >
> > We have loads of legacy debugging interfaces.
> >
> > And yes, ULP developers should be aware of it and test with
> > both settings.
> >
> > I'm not convinced any of this is a problem for this particular
> > setting. However, I guess the thing for you to do is propose a
> > patch.
>
> Are you guys seriously still on this? Its simply a trivial
> debug flag, whats all the fuss really about?

I hope that we are done.
https://patchwork.kernel.org/patch/9808615/

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ib_core: Enable and expose force_mr module parameter

[Sagi Grimberg]

> I hope that we are done.
> https://patchwork.kernel.org/patch/9808615/

You have a bogus empty line change there.
--
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_core: Enable and expose force_mr module parameter

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 257 bytes --]

On Wed, Jun 28, 2017 at 01:15:39PM +0300, Sagi Grimberg wrote:
> > I hope that we are done.
> > https://patchwork.kernel.org/patch/9808615/
>
> You have a bogus empty line change there.

If I remember correctly, It was needed to silence checkpatch.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ib_core: Enable and expose force_mr module parameter

[Sagi Grimberg]

>>> I hope that we are done.
>>> https://patchwork.kernel.org/patch/9808615/
>>
>> You have a bogus empty line change there.
> 
> If I remember correctly, It was needed to silence checkpatch.

So it should not be a part of the patch.

Also, I think that the statement is _really_ unnecessary:

"* Setting it to "true" will allow to mimic with IB devices,
  * the slow memory registration of iWARP devices."

And, its simply not true. Its designed to test the rw logic for
sg patterns that would normally not be registered. The fact
that iWARP registers memory for rdma reads is completely orthogonal.

And this statement is not true either:

"* This parameter is useful for new ULP bringup
  * and/or conversion to this R/W API."

Its designed to test the code itself, not for ULP
bringup or conversion, although it is useful for that.

And, I can't say that I 100% agree with not making it
configurable, but since you insist so much, at least
give a proper description.
--
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_core: Enable and expose force_mr module parameter

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]

On Wed, Jun 28, 2017 at 05:48:33PM +0300, Sagi Grimberg wrote:
>
> > > > I hope that we are done.
> > > > https://patchwork.kernel.org/patch/9808615/
> > >
> > > You have a bogus empty line change there.
> >
> > If I remember correctly, It was needed to silence checkpatch.
>
> So it should not be a part of the patch.
>
> Also, I think that the statement is _really_ unnecessary:
>
> "* Setting it to "true" will allow to mimic with IB devices,
>  * the slow memory registration of iWARP devices."
>
> And, its simply not true. Its designed to test the rw logic for
> sg patterns that would normally not be registered. The fact
> that iWARP registers memory for rdma reads is completely orthogonal.
>
> And this statement is not true either:
>
> "* This parameter is useful for new ULP bringup
>  * and/or conversion to this R/W API."
>
> Its designed to test the code itself, not for ULP
> bringup or conversion, although it is useful for that.
>
> And, I can't say that I 100% agree with not making it
> configurable, but since you insist so much, at least
> give a proper description.

No problem, I'll send updated version.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ib_core: Enable and expose force_mr module parameter

[Chuck Lever]

> On Jun 28, 2017, at 5:56 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Tue, Jun 27, 2017 at 12:24:20PM +0300, Sagi Grimberg wrote:
>> 
>>>> It will be "marked" as UAPI and we won't be able to change it,
>>>> which IMHO bad for the debug. It also invites users to set it
>>>> and in perfect world, we should test our HCAs with this flag
>>>> on and off, because curious user can set it.
>>> 
>>> We have loads of legacy debugging interfaces.
>>> 
>>> And yes, ULP developers should be aware of it and test with
>>> both settings.
>>> 
>>> I'm not convinced any of this is a problem for this particular
>>> setting. However, I guess the thing for you to do is propose a
>>> patch.
>> 
>> Are you guys seriously still on this? Its simply a trivial
>> debug flag, whats all the fuss really about?
> 
> I hope that we are done.
> https://patchwork.kernel.org/patch/9808615/

Somehow this got by me.

The patch description says "exposed to regular users." The
force_mr option is not exposed to regular users. Module
parameters are visible only to system administrators.

Also, just looking at the patch, it's not obvious why "The
force_mr module parameter wasn't exposed ... from the
beginning". IMO an explanation of what has prevented
exposure is needed. Commit a060b5629ab0 got the fourth
parameter of module_param_named wrong. Maybe a Fixes:
tag is needed too.

But frankly there is no visible change from this patch.
Before the patch, force_mr is not visible, and after the
patch it is not visible. Why then is this change necessary?
It would be helpful to cite a policy about why removal is
preferred over fixing the incorrect parameter.

Therefore IMO the patch description is not adequate.


--
Chuck Lever



--
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_core: Enable and expose force_mr module parameter

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

On Wed, Jun 28, 2017 at 10:49:00AM -0400, Chuck Lever wrote:
>
> > On Jun 28, 2017, at 5:56 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > On Tue, Jun 27, 2017 at 12:24:20PM +0300, Sagi Grimberg wrote:
> >>
> >>>> It will be "marked" as UAPI and we won't be able to change it,
> >>>> which IMHO bad for the debug. It also invites users to set it
> >>>> and in perfect world, we should test our HCAs with this flag
> >>>> on and off, because curious user can set it.
> >>>
> >>> We have loads of legacy debugging interfaces.
> >>>
> >>> And yes, ULP developers should be aware of it and test with
> >>> both settings.
> >>>
> >>> I'm not convinced any of this is a problem for this particular
> >>> setting. However, I guess the thing for you to do is propose a
> >>> patch.
> >>
> >> Are you guys seriously still on this? Its simply a trivial
> >> debug flag, whats all the fuss really about?
> >
> > I hope that we are done.
> > https://patchwork.kernel.org/patch/9808615/
>
> Somehow this got by me.
>
> The patch description says "exposed to regular users." The
> force_mr option is not exposed to regular users. Module
> parameters are visible only to system administrators.
>
> Also, just looking at the patch, it's not obvious why "The
> force_mr module parameter wasn't exposed ... from the
> beginning". IMO an explanation of what has prevented
> exposure is needed. Commit a060b5629ab0 got the fourth
> parameter of module_param_named wrong. Maybe a Fixes:
> tag is needed too.
>
> But frankly there is no visible change from this patch.
> Before the patch, force_mr is not visible, and after the
> patch it is not visible. Why then is this change necessary?
> It would be helpful to cite a policy about why removal is
> preferred over fixing the incorrect parameter.
>
> Therefore IMO the patch description is not adequate.

I'll update it.

Thanks

>
>
> --
> Chuck Lever
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]