On Wed, Oct 23, 2019 at 11:06:44AM +0800, Honggang LI wrote:
> On Tue, Oct 22, 2019 at 03:10:26PM -0700, Bart Van Assche wrote:
> > On 2019-10-22 00:00, Honggang LI wrote:
> > > +static bool use_imm_data(void)
> > > +{
> > > +#ifdef __linux__
> > > + bool ret = false;
> > > + char flag = 0;
> > > + int cnt;
> > > + int fd = open("/sys/module/ib_srp/parameters/use_imm_data", O_RDONLY);
> > > +
> > > + if (fd < 0)
> > > + return false;
> > > + cnt = read(fd, &flag, 1);
> > > + if (cnt != 1)
> > > + return false;
> > > +
> > > + if (!strncmp(&flag, "Y", 1))
> > > + ret = true;
> > > + close(fd);
> > > + return ret;
> > > +#else
> > > + return false;
> > > +#endif
> > > +}
> >
> > There is already plenty of Linux-specific code in srp_daemon. The #ifdef
> > __linux__ / #endif guard does not seem useful to me.
>
> Will delete the guard.
>
> >
> > There is a file descriptor leak in the above function, namely if read()
> > returns another value than 1.
>
> Yes, will fix it.
>
> >
> > The use_imm_data kernel module parameter was introduced in kernel v5.0
> > (commit 882981f4a411; "RDMA/srp: Add support for immediate data"). The
> > max_it_iu_size will be introduced in kernel v5.5 (commit 547ed331bbe8;
> > "RDMA/srp: Add parse function for maximum initiator to target IU size").
> >
> > So the above check will help for kernel versions before v5.0 but not for
> > kernel versions [v5.0..v5.5).
>
> Yes, you are right. The patch does not fix the issue for kernel
> versions [v5.0..v5.5). But it also does not do anything bad for
> kernel versions before v5.5 (commit 547ed331bbe8). It will fix
> the issue for kernel after 547ed331bbe8.
Well, for kernel versions [v5.0..v5.5), this patch will cause kernel to
emit warning message of unknown parameter. It seems we need add a flag
like this for ib_srp module:
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index b7f7a5f7bd98..96434f743a91 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -74,6 +74,7 @@ static bool allow_ext_sg;
static bool prefer_fr = true;
static bool register_always = true;
static bool never_register;
+static bool has_max_it_iu_size = true;
static int topspin_workarounds = 1;
module_param(srp_sg_tablesize, uint, 0444);
@@ -103,6 +104,10 @@ module_param(register_always, bool, 0444);
MODULE_PARM_DESC(register_always,
"Use memory registration even for contiguous memory regions");
+module_param(has_max_it_iu_size, bool, 0444);
+MODULE_PARM_DESC(has_max_it_iu_size,
+ "Indicate the module supports max_it_iu_size login parameter");
+
module_param(never_register, bool, 0444);
MODULE_PARM_DESC(never_register, "Never register memory");
==============
Then, srp_daemon should check file "/sys/module/ib_srp/parameters/has_max_it_iu_size"
instead of "/sys/module/ib_srp/parameters/use_imm_data".
