| Message ID | 20170515145203.10708.16921.stgit@klimt.1015granger.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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@oracle.com> 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> On May 15, 2017, at 10:57 AM, Leon Romanovsky <leon@kernel.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@oracle.com> > > 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@vger.kernel.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 15, 2017 at 11:09:06AM -0400, Chuck Lever wrote: > > > On May 15, 2017, at 10:57 AM, Leon Romanovsky <leon@kernel.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@oracle.com> > > > > 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@vger.kernel.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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> On May 15, 2017, at 1:00 PM, Leon Romanovsky <leon@kernel.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@kernel.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@oracle.com> >>> >>> 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@vger.kernel.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@vger.kernel.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On May 16, 2017, at 3:22 AM, Sagi Grimberg <sagi@grimberg.me> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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"); /*
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@oracle.com> --- drivers/infiniband/core/rw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 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