| Message ID | 20200909134726.10348-1-bharat@chelsio.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA/iw_cxgb4: disable delayed ack by default | expand |
Missed rdma-next subject prefix. Resending. -----Original Message----- From: Potnuri Bharat Teja <bharat@chelsio.com> Sent: Wednesday, September 9, 2020 7:17 PM To: jgg@ziepe.ca; dledford@redhat.com Cc: linux-rdma@vger.kernel.org; Potnuri Bharat Teja <bharat@chelsio.com> Subject: [PATCH] RDMA/iw_cxgb4: disable delayed ack by default Receive side delayed ack mode is needed only for certain area networks/ connections. Therefore disable it by default. Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com> --- drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c index 1f288c73ccfc..8769e7aa097f 100644 --- a/drivers/infiniband/hw/cxgb4/cm.c +++ b/drivers/infiniband/hw/cxgb4/cm.c @@ -77,9 +77,9 @@ static int enable_ecn; module_param(enable_ecn, int, 0644); MODULE_PARM_DESC(enable_ecn, "Enable ECN (default=0/disabled)"); -static int dack_mode = 1; +static int dack_mode; module_param(dack_mode, int, 0644); -MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=1)"); +MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=0)"); uint c4iw_max_read_depth = 32; module_param(c4iw_max_read_depth, int, 0644); -- 2.24.0
On Wed, Sep 09, 2020 at 03:28:44PM +0000, Potnuri Bharat Teja wrote: > Missed rdma-next subject prefix. Resending. > > -----Original Message----- > From: Potnuri Bharat Teja <bharat@chelsio.com> > Sent: Wednesday, September 9, 2020 7:17 PM > To: jgg@ziepe.ca; dledford@redhat.com > Cc: linux-rdma@vger.kernel.org; Potnuri Bharat Teja <bharat@chelsio.com> > Subject: [PATCH] RDMA/iw_cxgb4: disable delayed ack by default > > Receive side delayed ack mode is needed only for certain area networks/ connections. Therefore disable it by default. > > Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com> > --- > drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c > index 1f288c73ccfc..8769e7aa097f 100644 > --- a/drivers/infiniband/hw/cxgb4/cm.c > +++ b/drivers/infiniband/hw/cxgb4/cm.c > @@ -77,9 +77,9 @@ static int enable_ecn; module_param(enable_ecn, int, 0644); MODULE_PARM_DESC(enable_ecn, "Enable ECN (default=0/disabled)"); > > -static int dack_mode = 1; > +static int dack_mode; > module_param(dack_mode, int, 0644); > -MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=1)"); > +MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=0)"); Are you sure that this doesn't break user scripts? Thanks > > uint c4iw_max_read_depth = 32; > module_param(c4iw_max_read_depth, int, 0644); > -- > 2.24.0
>> Subject: [PATCH] RDMA/iw_cxgb4: disable delayed ack by default >> >> Receive side delayed ack mode is needed only for certain area networks/ connections. Therefore disable it by default. >> >> Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com> >> --- >> drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c >> index 1f288c73ccfc..8769e7aa097f 100644 >> --- a/drivers/infiniband/hw/cxgb4/cm.c >> +++ b/drivers/infiniband/hw/cxgb4/cm.c >> @@ -77,9 +77,9 @@ static int enable_ecn; module_param(enable_ecn, int, 0644); MODULE_PARM_DESC(enable_ecn, "Enable ECN (default=0/disabled)"); >> >> -static int dack_mode = 1; >> +static int dack_mode; >> module_param(dack_mode, int, 0644); >> -MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=1)"); >> +MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=0)"); > >Are you sure that this doesn't break user scripts? Yes, I am sure. This does not interfere with user/kernel RDMA functionalities. Thanks, Bharat. > >Thanks > >> >> uint c4iw_max_read_depth = 32; >> module_param(c4iw_max_read_depth, int, 0644); >> -- >> 2.24.0
On Thu, Sep 10, 2020 at 07:14:50PM +0000, Potnuri Bharat Teja wrote: > >> Subject: [PATCH] RDMA/iw_cxgb4: disable delayed ack by default > >> > >> Receive side delayed ack mode is needed only for certain area networks/ connections. Therefore disable it by default. > >> > >> Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com> > >> --- > >> drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c > >> index 1f288c73ccfc..8769e7aa097f 100644 > >> --- a/drivers/infiniband/hw/cxgb4/cm.c > >> +++ b/drivers/infiniband/hw/cxgb4/cm.c > >> @@ -77,9 +77,9 @@ static int enable_ecn; module_param(enable_ecn, int, 0644); MODULE_PARM_DESC(enable_ecn, "Enable ECN (default=0/disabled)"); > >> > >> -static int dack_mode = 1; > >> +static int dack_mode; > >> module_param(dack_mode, int, 0644); > >> -MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=1)"); > >> +MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=0)"); > > > >Are you sure that this doesn't break user scripts? > Yes, I am sure. This does not interfere with user/kernel RDMA functionalities. How is it possible? Before this change user that did "modprobe iw_cxgb4" had delayed mode enabled, after this he will need to issue "modprobe iw_cxgb4 dack_mode=1" https://github.com/linux-rdma/rdma-core/blob/master/kernel-boot/rdma-hw-modules.rules#L12 Thanks > > Thanks, > Bharat. > > > >Thanks > > > >> > >> uint c4iw_max_read_depth = 32; > >> module_param(c4iw_max_read_depth, int, 0644); > >> -- > >> 2.24.0
On Friday, September 09/11/20, 2020 at 18:41:52 +0530, Leon Romanovsky wrote: > On Thu, Sep 10, 2020 at 07:14:50PM +0000, Potnuri Bharat Teja wrote: > > >> Subject: [PATCH] RDMA/iw_cxgb4: disable delayed ack by default > > >> > > >> Receive side delayed ack mode is needed only for certain area networks/ connections. Therefore disable it by default. > > >> > > >> Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com> > > >> --- > > >> drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- > > >> 1 file changed, 2 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c > > >> index 1f288c73ccfc..8769e7aa097f 100644 > > >> --- a/drivers/infiniband/hw/cxgb4/cm.c > > >> +++ b/drivers/infiniband/hw/cxgb4/cm.c > > >> @@ -77,9 +77,9 @@ static int enable_ecn; module_param(enable_ecn, int, 0644); MODULE_PARM_DESC(enable_ecn, "Enable ECN (default=0/disabled)"); > > >> > > >> -static int dack_mode = 1; > > >> +static int dack_mode; > > >> module_param(dack_mode, int, 0644); > > >> -MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=1)"); > > >> +MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=0)"); > > > > > >Are you sure that this doesn't break user scripts? > > Yes, I am sure. This does not interfere with user/kernel RDMA functionalities. > > How is it possible? > Before this change user that did "modprobe iw_cxgb4" had delayed mode > enabled, after this he will need to issue "modprobe iw_cxgb4 dack_mode=1" > Sorry I didnt get it right earlier. Yes now if user wants delayed ack mode, user can issue "modprobe iw_cxgb4 dack_mode=1" or change it dynamically via /sys/module/<>/paramters. Chelsio adapters for better performance in most cases needs delayed ack mode to be disabled. This change somehow got missed upstream all this while. In fewer explicit cases, user can enable delayed ack mode where it is needed. > https://github.com/linux-rdma/rdma-core/blob/master/kernel-boot/rdma-hw-modules.rules#L12 > > Thanks > > > > > Thanks, > > Bharat. > > > > > >Thanks > > > > > >> > > >> uint c4iw_max_read_depth = 32; > > >> module_param(c4iw_max_read_depth, int, 0644); > > >> -- > > >> 2.24.0
On Fri, Sep 11, 2020 at 09:39:57PM +0530, Potnuri Bharat Teja wrote: > On Friday, September 09/11/20, 2020 at 18:41:52 +0530, Leon Romanovsky wrote: > > On Thu, Sep 10, 2020 at 07:14:50PM +0000, Potnuri Bharat Teja wrote: > > > >> Subject: [PATCH] RDMA/iw_cxgb4: disable delayed ack by default > > > >> > > > >> Receive side delayed ack mode is needed only for certain area networks/ connections. Therefore disable it by default. > > > >> > > > >> Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com> > > > >> --- > > > >> drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- > > > >> 1 file changed, 2 insertions(+), 2 deletions(-) > > > >> > > > >> diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c > > > >> index 1f288c73ccfc..8769e7aa097f 100644 > > > >> --- a/drivers/infiniband/hw/cxgb4/cm.c > > > >> +++ b/drivers/infiniband/hw/cxgb4/cm.c > > > >> @@ -77,9 +77,9 @@ static int enable_ecn; module_param(enable_ecn, int, 0644); MODULE_PARM_DESC(enable_ecn, "Enable ECN (default=0/disabled)"); > > > >> > > > >> -static int dack_mode = 1; > > > >> +static int dack_mode; > > > >> module_param(dack_mode, int, 0644); > > > >> -MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=1)"); > > > >> +MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=0)"); > > > > > > > >Are you sure that this doesn't break user scripts? > > > Yes, I am sure. This does not interfere with user/kernel RDMA functionalities. > > > > How is it possible? > > Before this change user that did "modprobe iw_cxgb4" had delayed mode > > enabled, after this he will need to issue "modprobe iw_cxgb4 dack_mode=1" > > > Sorry I didnt get it right earlier. Yes now if user wants delayed ack mode, > user can issue "modprobe iw_cxgb4 dack_mode=1" or change it dynamically via > /sys/module/<>/paramters. > Chelsio adapters for better performance in most cases needs delayed ack mode to > be disabled. This change somehow got missed upstream all this while. > In fewer explicit cases, user can enable delayed ack mode where it is needed. So why doesn't rdma-core have dack_mode=0 in kernel-boot scripts? Thanks
On Sunday, September 09/13/20, 2020 at 12:43:25 +0530, Leon Romanovsky wrote: > On Fri, Sep 11, 2020 at 09:39:57PM +0530, Potnuri Bharat Teja wrote: > > On Friday, September 09/11/20, 2020 at 18:41:52 +0530, Leon Romanovsky wrote: > > > On Thu, Sep 10, 2020 at 07:14:50PM +0000, Potnuri Bharat Teja wrote: > > > > >> Subject: [PATCH] RDMA/iw_cxgb4: disable delayed ack by default > > > > >> > > > > >> Receive side delayed ack mode is needed only for certain area networks/ connections. Therefore disable it by default. > > > > >> > > > > >> Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com> > > > > >> --- > > > > >> drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- > > > > >> 1 file changed, 2 insertions(+), 2 deletions(-) > > > > >> > > > > >> diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c > > > > >> index 1f288c73ccfc..8769e7aa097f 100644 > > > > >> --- a/drivers/infiniband/hw/cxgb4/cm.c > > > > >> +++ b/drivers/infiniband/hw/cxgb4/cm.c > > > > >> @@ -77,9 +77,9 @@ static int enable_ecn; module_param(enable_ecn, int, 0644); MODULE_PARM_DESC(enable_ecn, "Enable ECN (default=0/disabled)"); > > > > >> > > > > >> -static int dack_mode = 1; > > > > >> +static int dack_mode; > > > > >> module_param(dack_mode, int, 0644); > > > > >> -MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=1)"); > > > > >> +MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=0)"); > > > > > > > > > >Are you sure that this doesn't break user scripts? > > > > Yes, I am sure. This does not interfere with user/kernel RDMA functionalities. > > > > > > How is it possible? > > > Before this change user that did "modprobe iw_cxgb4" had delayed mode > > > enabled, after this he will need to issue "modprobe iw_cxgb4 dack_mode=1" > > > > > Sorry I didnt get it right earlier. Yes now if user wants delayed ack mode, > > user can issue "modprobe iw_cxgb4 dack_mode=1" or change it dynamically via > > /sys/module/<>/paramters. > > Chelsio adapters for better performance in most cases needs delayed ack mode to > > be disabled. This change somehow got missed upstream all this while. > > In fewer explicit cases, user can enable delayed ack mode where it is needed. > > So why doesn't rdma-core have dack_mode=0 in kernel-boot scripts? > Do you mean why dont I simply change rdma-hw-modules.rules instead? I could do that if this is a user mode only setting. But this is needed for kernel mode too where in some cases rdma-core may not be installed. Thanks.
On Mon, Sep 14, 2020 at 10:31:17PM +0530, Potnuri Bharat Teja wrote: > On Sunday, September 09/13/20, 2020 at 12:43:25 +0530, Leon Romanovsky wrote: > > On Fri, Sep 11, 2020 at 09:39:57PM +0530, Potnuri Bharat Teja wrote: > > > On Friday, September 09/11/20, 2020 at 18:41:52 +0530, Leon Romanovsky wrote: > > > > On Thu, Sep 10, 2020 at 07:14:50PM +0000, Potnuri Bharat Teja wrote: > > > > > >> Subject: [PATCH] RDMA/iw_cxgb4: disable delayed ack by default > > > > > >> > > > > > >> Receive side delayed ack mode is needed only for certain area networks/ connections. Therefore disable it by default. > > > > > >> > > > > > >> Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com> > > > > > >> --- > > > > > >> drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- > > > > > >> 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > >> > > > > > >> diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c > > > > > >> index 1f288c73ccfc..8769e7aa097f 100644 > > > > > >> --- a/drivers/infiniband/hw/cxgb4/cm.c > > > > > >> +++ b/drivers/infiniband/hw/cxgb4/cm.c > > > > > >> @@ -77,9 +77,9 @@ static int enable_ecn; module_param(enable_ecn, int, 0644); MODULE_PARM_DESC(enable_ecn, "Enable ECN (default=0/disabled)"); > > > > > >> > > > > > >> -static int dack_mode = 1; > > > > > >> +static int dack_mode; > > > > > >> module_param(dack_mode, int, 0644); > > > > > >> -MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=1)"); > > > > > >> +MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=0)"); > > > > > > > > > > > >Are you sure that this doesn't break user scripts? > > > > > Yes, I am sure. This does not interfere with user/kernel RDMA functionalities. > > > > > > > > How is it possible? > > > > Before this change user that did "modprobe iw_cxgb4" had delayed mode > > > > enabled, after this he will need to issue "modprobe iw_cxgb4 dack_mode=1" > > > > > > > Sorry I didnt get it right earlier. Yes now if user wants delayed ack mode, > > > user can issue "modprobe iw_cxgb4 dack_mode=1" or change it dynamically via > > > /sys/module/<>/paramters. > > > Chelsio adapters for better performance in most cases needs delayed ack mode to > > > be disabled. This change somehow got missed upstream all this while. > > > In fewer explicit cases, user can enable delayed ack mode where it is needed. > > > > So why doesn't rdma-core have dack_mode=0 in kernel-boot scripts? > > > Do you mean why dont I simply change rdma-hw-modules.rules instead? > I could do that if this is a user mode only setting. But this is needed for kernel > mode too where in some cases rdma-core may not be installed. No, I'm asking why if "dack_mode=0" is better and preferable way to operate, it was never set in rdma-core. I'm not saying that your kernel patch is wrong, just trying to get sense on implications for the rdma-core. Thanks > > Thanks. >
On Tuesday, September 09/15/20, 2020 at 12:17:06 +0530, Leon Romanovsky wrote: > On Mon, Sep 14, 2020 at 10:31:17PM +0530, Potnuri Bharat Teja wrote: > > On Sunday, September 09/13/20, 2020 at 12:43:25 +0530, Leon Romanovsky wrote: > > > On Fri, Sep 11, 2020 at 09:39:57PM +0530, Potnuri Bharat Teja wrote: > > > > On Friday, September 09/11/20, 2020 at 18:41:52 +0530, Leon Romanovsky wrote: > > > > > On Thu, Sep 10, 2020 at 07:14:50PM +0000, Potnuri Bharat Teja wrote: > > > > > > >> Subject: [PATCH] RDMA/iw_cxgb4: disable delayed ack by default > > > > > > >> > > > > > > >> Receive side delayed ack mode is needed only for certain area networks/ connections. Therefore disable it by default. > > > > > > >> > > > > > > >> Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com> > > > > > > >> --- > > > > > > >> drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- > > > > > > >> 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > >> > > > > > > >> diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c > > > > > > >> index 1f288c73ccfc..8769e7aa097f 100644 > > > > > > >> --- a/drivers/infiniband/hw/cxgb4/cm.c > > > > > > >> +++ b/drivers/infiniband/hw/cxgb4/cm.c > > > > > > >> @@ -77,9 +77,9 @@ static int enable_ecn; module_param(enable_ecn, int, 0644); MODULE_PARM_DESC(enable_ecn, "Enable ECN (default=0/disabled)"); > > > > > > >> > > > > > > >> -static int dack_mode = 1; > > > > > > >> +static int dack_mode; > > > > > > >> module_param(dack_mode, int, 0644); > > > > > > >> -MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=1)"); > > > > > > >> +MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=0)"); > > > > > > > > > > > > > >Are you sure that this doesn't break user scripts? > > > > > > Yes, I am sure. This does not interfere with user/kernel RDMA functionalities. > > > > > > > > > > How is it possible? > > > > > Before this change user that did "modprobe iw_cxgb4" had delayed mode > > > > > enabled, after this he will need to issue "modprobe iw_cxgb4 dack_mode=1" > > > > > > > > > Sorry I didnt get it right earlier. Yes now if user wants delayed ack mode, > > > > user can issue "modprobe iw_cxgb4 dack_mode=1" or change it dynamically via > > > > /sys/module/<>/paramters. > > > > Chelsio adapters for better performance in most cases needs delayed ack mode to > > > > be disabled. This change somehow got missed upstream all this while. > > > > In fewer explicit cases, user can enable delayed ack mode where it is needed. > > > > > > So why doesn't rdma-core have dack_mode=0 in kernel-boot scripts? > > > > > Do you mean why dont I simply change rdma-hw-modules.rules instead? > > I could do that if this is a user mode only setting. But this is needed for kernel > > mode too where in some cases rdma-core may not be installed. > > No, I'm asking why if "dack_mode=0" is better and preferable way to > operate, it was never set in rdma-core. > Found this only recently that it was wrongly enabled by default which affects severely the performance of some of our adapters with low HW resources for larger IO sizes. I could have mentioned this in commit description but I thought it's a trivial change code wise. > I'm not saying that your kernel patch is wrong, just trying to get sense > on implications for the rdma-core. > ok. yes rdma-hw-modules.rules provides a cleaner way. I am still used to setting module params via /sys/module/<>/parameters/*. Thanks for the review.
diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c index 1f288c73ccfc..8769e7aa097f 100644 --- a/drivers/infiniband/hw/cxgb4/cm.c +++ b/drivers/infiniband/hw/cxgb4/cm.c @@ -77,9 +77,9 @@ static int enable_ecn; module_param(enable_ecn, int, 0644); MODULE_PARM_DESC(enable_ecn, "Enable ECN (default=0/disabled)"); -static int dack_mode = 1; +static int dack_mode; module_param(dack_mode, int, 0644); -MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=1)"); +MODULE_PARM_DESC(dack_mode, "Delayed ack mode (default=0)"); uint c4iw_max_read_depth = 32; module_param(c4iw_max_read_depth, int, 0644);
Receive side delayed ack mode is needed only for certain area networks/ connections. Therefore disable it by default. Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com> --- drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)