[PATCH for-next v2] RDMA/cma: Replace RMW with atomic bit-ops

[PATCH for-next v2] RDMA/cma: Replace RMW with atomic bit-ops

[Håkon Bugge]

The struct rdma_id_private contains three bit-fields, tos_set,
timeout_set, and min_rnr_timer_set. These are set by accessor
functions without any synchronization. If two or all accessor
functions are invoked in close proximity in time, there will be
Read-Modify-Write from several contexts to the same variable, and the
result will be intermittent.

Replace with a flag variable and an inline function for set with
appropriate memory barriers and the use of test_bit().

Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
Signed-off-by: Hans Westgaard Ry<hans.westgaard.ry@oracle.com>

---
	v1 -> v2:
	   * Removed define wizardry and replaced with a set function
             with memory barriers. Suggested by Leon.
	   * Removed zero-initialization of flags, due to kzalloc(),
             as suggested by Leon
	   * Review comments from Stefan implicitly adapted due to
             first bullet above
	   * Moved defines and inline function from header file to
             cma.c, as suggested by the undersigned
	   * Renamed enum to cm_id_priv_flag_bits as suggested by the
             undersigned
---
 drivers/infiniband/core/cma.c      | 38 +++++++++++++++++++++++++-------------
 drivers/infiniband/core/cma_priv.h |  4 +---
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 2b9ffc2..cc8ad82 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -48,6 +48,21 @@
 #define CMA_IBOE_PACKET_LIFETIME 18
 #define CMA_PREFERRED_ROCE_GID_TYPE IB_GID_TYPE_ROCE_UDP_ENCAP
 
+static void set_bit_mb(unsigned long nr, unsigned long *flags)
+{
+	/* set_bit() does not imply a memory barrier */
+	smp_mb__before_atomic();
+	set_bit(nr, flags);
+	/* set_bit() does not imply a memory barrier */
+	smp_mb__after_atomic();
+}
+
+enum cm_id_priv_flag_bits {
+	TOS_SET,
+	TIMEOUT_SET,
+	MIN_RNR_TIMER_SET,
+};
+
 static const char * const cma_events[] = {
 	[RDMA_CM_EVENT_ADDR_RESOLVED]	 = "address resolved",
 	[RDMA_CM_EVENT_ADDR_ERROR]	 = "address error",
@@ -844,9 +859,6 @@ static void cma_id_put(struct rdma_id_private *id_priv)
 	id_priv->id.event_handler = event_handler;
 	id_priv->id.ps = ps;
 	id_priv->id.qp_type = qp_type;
-	id_priv->tos_set = false;
-	id_priv->timeout_set = false;
-	id_priv->min_rnr_timer_set = false;
 	id_priv->gid_type = IB_GID_TYPE_IB;
 	spin_lock_init(&id_priv->lock);
 	mutex_init(&id_priv->qp_mutex);
@@ -1134,10 +1146,10 @@ int rdma_init_qp_attr(struct rdma_cm_id *id, struct ib_qp_attr *qp_attr,
 		ret = -ENOSYS;
 	}
 
-	if ((*qp_attr_mask & IB_QP_TIMEOUT) && id_priv->timeout_set)
+	if ((*qp_attr_mask & IB_QP_TIMEOUT) && test_bit(TIMEOUT_SET, &id_priv->flags))
 		qp_attr->timeout = id_priv->timeout;
 
-	if ((*qp_attr_mask & IB_QP_MIN_RNR_TIMER) && id_priv->min_rnr_timer_set)
+	if ((*qp_attr_mask & IB_QP_MIN_RNR_TIMER) && test_bit(MIN_RNR_TIMER_SET, &id_priv->flags))
 		qp_attr->min_rnr_timer = id_priv->min_rnr_timer;
 
 	return ret;
@@ -2472,7 +2484,7 @@ static int cma_iw_listen(struct rdma_id_private *id_priv, int backlog)
 		return PTR_ERR(id);
 
 	id->tos = id_priv->tos;
-	id->tos_set = id_priv->tos_set;
+	id->tos_set = test_bit(TOS_SET, &id_priv->flags);
 	id->afonly = id_priv->afonly;
 	id_priv->cm_id.iw = id;
 
@@ -2533,7 +2545,7 @@ static int cma_listen_on_dev(struct rdma_id_private *id_priv,
 	cma_id_get(id_priv);
 	dev_id_priv->internal_id = 1;
 	dev_id_priv->afonly = id_priv->afonly;
-	dev_id_priv->tos_set = id_priv->tos_set;
+	dev_id_priv->flags = id_priv->flags;
 	dev_id_priv->tos = id_priv->tos;
 
 	ret = rdma_listen(&dev_id_priv->id, id_priv->backlog);
@@ -2582,7 +2594,7 @@ void rdma_set_service_type(struct rdma_cm_id *id, int tos)
 
 	id_priv = container_of(id, struct rdma_id_private, id);
 	id_priv->tos = (u8) tos;
-	id_priv->tos_set = true;
+	set_bit_mb(TOS_SET, &id_priv->flags);
 }
 EXPORT_SYMBOL(rdma_set_service_type);
 
@@ -2610,7 +2622,7 @@ int rdma_set_ack_timeout(struct rdma_cm_id *id, u8 timeout)
 
 	id_priv = container_of(id, struct rdma_id_private, id);
 	id_priv->timeout = timeout;
-	id_priv->timeout_set = true;
+	set_bit_mb(TIMEOUT_SET, &id_priv->flags);
 
 	return 0;
 }
@@ -2647,7 +2659,7 @@ int rdma_set_min_rnr_timer(struct rdma_cm_id *id, u8 min_rnr_timer)
 
 	id_priv = container_of(id, struct rdma_id_private, id);
 	id_priv->min_rnr_timer = min_rnr_timer;
-	id_priv->min_rnr_timer_set = true;
+	set_bit_mb(MIN_RNR_TIMER_SET, &id_priv->flags);
 
 	return 0;
 }
@@ -3033,7 +3045,7 @@ static int cma_resolve_iboe_route(struct rdma_id_private *id_priv)
 
 	u8 default_roce_tos = id_priv->cma_dev->default_roce_tos[id_priv->id.port_num -
 					rdma_start_port(id_priv->cma_dev->device)];
-	u8 tos = id_priv->tos_set ? id_priv->tos : default_roce_tos;
+	u8 tos = test_bit(TOS_SET, &id_priv->flags) ? id_priv->tos : default_roce_tos;
 
 
 	work = kzalloc(sizeof *work, GFP_KERNEL);
@@ -3081,7 +3093,7 @@ static int cma_resolve_iboe_route(struct rdma_id_private *id_priv)
 	 * PacketLifeTime = local ACK timeout/2
 	 * as a reasonable approximation for RoCE networks.
 	 */
-	route->path_rec->packet_life_time = id_priv->timeout_set ?
+	route->path_rec->packet_life_time = test_bit(TIMEOUT_SET, &id_priv->flags) ?
 		id_priv->timeout - 1 : CMA_IBOE_PACKET_LIFETIME;
 
 	if (!route->path_rec->mtu) {
@@ -4107,7 +4119,7 @@ static int cma_connect_iw(struct rdma_id_private *id_priv,
 		return PTR_ERR(cm_id);
 
 	cm_id->tos = id_priv->tos;
-	cm_id->tos_set = id_priv->tos_set;
+	cm_id->tos_set = test_bit(TOS_SET, &id_priv->flags);
 	id_priv->cm_id.iw = cm_id;
 
 	memcpy(&cm_id->local_addr, cma_src_addr(id_priv),
diff --git a/drivers/infiniband/core/cma_priv.h b/drivers/infiniband/core/cma_priv.h
index 5c463da..5d3d0db 100644
--- a/drivers/infiniband/core/cma_priv.h
+++ b/drivers/infiniband/core/cma_priv.h
@@ -82,11 +82,9 @@ struct rdma_id_private {
 	u32			qkey;
 	u32			qp_num;
 	u32			options;
+	unsigned long		flags;
 	u8			srq;
 	u8			tos;
-	u8			tos_set:1;
-	u8                      timeout_set:1;
-	u8			min_rnr_timer_set:1;
 	u8			reuseaddr;
 	u8			afonly;
 	u8			timeout;
-- 
1.8.3.1

Re: [PATCH for-next v2] RDMA/cma: Replace RMW with atomic bit-ops

[Leon Romanovsky]

On Thu, Jun 17, 2021 at 02:59:25PM +0200, Håkon Bugge wrote:
> The struct rdma_id_private contains three bit-fields, tos_set,
> timeout_set, and min_rnr_timer_set. These are set by accessor
> functions without any synchronization. If two or all accessor
> functions are invoked in close proximity in time, there will be
> Read-Modify-Write from several contexts to the same variable, and the
> result will be intermittent.
> 
> Replace with a flag variable and an inline function for set with
> appropriate memory barriers and the use of test_bit().
> 
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> Signed-off-by: Hans Westgaard Ry<hans.westgaard.ry@oracle.com>
> 
> ---
> 	v1 -> v2:
> 	   * Removed define wizardry and replaced with a set function
>              with memory barriers. Suggested by Leon.
> 	   * Removed zero-initialization of flags, due to kzalloc(),
>              as suggested by Leon
> 	   * Review comments from Stefan implicitly adapted due to
>              first bullet above
> 	   * Moved defines and inline function from header file to
>              cma.c, as suggested by the undersigned
> 	   * Renamed enum to cm_id_priv_flag_bits as suggested by the
>              undersigned
> ---
>  drivers/infiniband/core/cma.c      | 38 +++++++++++++++++++++++++-------------
>  drivers/infiniband/core/cma_priv.h |  4 +---
>  2 files changed, 26 insertions(+), 16 deletions(-)

This patch generates checkpatch warnings.

➜  kernel git:(rdma-next) git checkpatch
WARNING: line length of 86 exceeds 80 columns
#69: FILE: drivers/infiniband/core/cma.c:1149:
+	if ((*qp_attr_mask & IB_QP_TIMEOUT) && test_bit(TIMEOUT_SET, &id_priv->flags))

WARNING: line length of 98 exceeds 80 columns
#73: FILE: drivers/infiniband/core/cma.c:1152:
+	if ((*qp_attr_mask & IB_QP_MIN_RNR_TIMER) && test_bit(MIN_RNR_TIMER_SET, &id_priv->flags))

WARNING: line length of 86 exceeds 80 columns
#127: FILE: drivers/infiniband/core/cma.c:3048:
+	u8 tos = test_bit(TOS_SET, &id_priv->flags) ? id_priv->tos : default_roce_tos;

WARNING: line length of 84 exceeds 80 columns
#136: FILE: drivers/infiniband/core/cma.c:3096:
+	route->path_rec->packet_life_time = test_bit(TIMEOUT_SET, &id_priv->flags) ?

0001-RDMA-cma-Replace-RMW-with-atomic-bit-ops.patch total: 0 errors, 4 warnings, 118 lines checked

Thanks

Re: [PATCH for-next v2] RDMA/cma: Replace RMW with atomic bit-ops

[Haakon Bugge]

> On 21 Jun 2021, at 09:10, Leon Romanovsky <leon@kernel.org> wrote:
> 
> On Thu, Jun 17, 2021 at 02:59:25PM +0200, Håkon Bugge wrote:
>> The struct rdma_id_private contains three bit-fields, tos_set,
>> timeout_set, and min_rnr_timer_set. These are set by accessor
>> functions without any synchronization. If two or all accessor
>> functions are invoked in close proximity in time, there will be
>> Read-Modify-Write from several contexts to the same variable, and the
>> result will be intermittent.
>> 
>> Replace with a flag variable and an inline function for set with
>> appropriate memory barriers and the use of test_bit().
>> 
>> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
>> Signed-off-by: Hans Westgaard Ry<hans.westgaard.ry@oracle.com>
>> 
>> ---
>> 	v1 -> v2:
>> 	   * Removed define wizardry and replaced with a set function
>>             with memory barriers. Suggested by Leon.
>> 	   * Removed zero-initialization of flags, due to kzalloc(),
>>             as suggested by Leon
>> 	   * Review comments from Stefan implicitly adapted due to
>>             first bullet above
>> 	   * Moved defines and inline function from header file to
>>             cma.c, as suggested by the undersigned
>> 	   * Renamed enum to cm_id_priv_flag_bits as suggested by the
>>             undersigned
>> ---
>> drivers/infiniband/core/cma.c      | 38 +++++++++++++++++++++++++-------------
>> drivers/infiniband/core/cma_priv.h |  4 +---
>> 2 files changed, 26 insertions(+), 16 deletions(-)
> 
> This patch generates checkpatch warnings.
> 
> ➜  kernel git:(rdma-next) git checkpatch
> WARNING: line length of 86 exceeds 80 columns
> #69: FILE: drivers/infiniband/core/cma.c:1149:
> +	if ((*qp_attr_mask & IB_QP_TIMEOUT) && test_bit(TIMEOUT_SET, &id_priv->flags))
> 
> WARNING: line length of 98 exceeds 80 columns
> #73: FILE: drivers/infiniband/core/cma.c:1152:
> +	if ((*qp_attr_mask & IB_QP_MIN_RNR_TIMER) && test_bit(MIN_RNR_TIMER_SET, &id_priv->flags))
> 
> WARNING: line length of 86 exceeds 80 columns
> #127: FILE: drivers/infiniband/core/cma.c:3048:
> +	u8 tos = test_bit(TOS_SET, &id_priv->flags) ? id_priv->tos : default_roce_tos;
> 
> WARNING: line length of 84 exceeds 80 columns
> #136: FILE: drivers/infiniband/core/cma.c:3096:
> +	route->path_rec->packet_life_time = test_bit(TIMEOUT_SET, &id_priv->flags) ?
> 
> 0001-RDMA-cma-Replace-RMW-with-atomic-bit-ops.patch total: 0 errors, 4 warnings, 118 lines checked

You're running an old checkpatch. Since commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"), the default line-length is 100. As Linus states in:

https://lkml.org/lkml/2009/12/17/229

"... But 80 characters is causing too many idiotic changes."

So, indeed, the commit passes:

$ scripts/checkpatch.pl --strict --git HEAD
total: 0 errors, 0 warnings, 0 checks, 118 lines checked

Commit d4d0dcd9513e ("RDMA/cma: Replace RMW with atomic bit-ops") has no obvious style problems and is ready for submission.



Thxs, Håkon



> 
> Thanks

Re: [PATCH for-next v2] RDMA/cma: Replace RMW with atomic bit-ops

[Leon Romanovsky]

On Mon, Jun 21, 2021 at 08:20:47AM +0000, Haakon Bugge wrote:
> 
> 
> > On 21 Jun 2021, at 09:10, Leon Romanovsky <leon@kernel.org> wrote:
> > 
> > On Thu, Jun 17, 2021 at 02:59:25PM +0200, Håkon Bugge wrote:
> >> The struct rdma_id_private contains three bit-fields, tos_set,
> >> timeout_set, and min_rnr_timer_set. These are set by accessor
> >> functions without any synchronization. If two or all accessor
> >> functions are invoked in close proximity in time, there will be
> >> Read-Modify-Write from several contexts to the same variable, and the
> >> result will be intermittent.
> >> 
> >> Replace with a flag variable and an inline function for set with
> >> appropriate memory barriers and the use of test_bit().
> >> 
> >> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> >> Signed-off-by: Hans Westgaard Ry<hans.westgaard.ry@oracle.com>
> >> 
> >> ---
> >> 	v1 -> v2:
> >> 	   * Removed define wizardry and replaced with a set function
> >>             with memory barriers. Suggested by Leon.
> >> 	   * Removed zero-initialization of flags, due to kzalloc(),
> >>             as suggested by Leon
> >> 	   * Review comments from Stefan implicitly adapted due to
> >>             first bullet above
> >> 	   * Moved defines and inline function from header file to
> >>             cma.c, as suggested by the undersigned
> >> 	   * Renamed enum to cm_id_priv_flag_bits as suggested by the
> >>             undersigned
> >> ---
> >> drivers/infiniband/core/cma.c      | 38 +++++++++++++++++++++++++-------------
> >> drivers/infiniband/core/cma_priv.h |  4 +---
> >> 2 files changed, 26 insertions(+), 16 deletions(-)
> > 
> > This patch generates checkpatch warnings.
> > 
> > ➜  kernel git:(rdma-next) git checkpatch
> > WARNING: line length of 86 exceeds 80 columns
> > #69: FILE: drivers/infiniband/core/cma.c:1149:
> > +	if ((*qp_attr_mask & IB_QP_TIMEOUT) && test_bit(TIMEOUT_SET, &id_priv->flags))
> > 
> > WARNING: line length of 98 exceeds 80 columns
> > #73: FILE: drivers/infiniband/core/cma.c:1152:
> > +	if ((*qp_attr_mask & IB_QP_MIN_RNR_TIMER) && test_bit(MIN_RNR_TIMER_SET, &id_priv->flags))
> > 
> > WARNING: line length of 86 exceeds 80 columns
> > #127: FILE: drivers/infiniband/core/cma.c:3048:
> > +	u8 tos = test_bit(TOS_SET, &id_priv->flags) ? id_priv->tos : default_roce_tos;
> > 
> > WARNING: line length of 84 exceeds 80 columns
> > #136: FILE: drivers/infiniband/core/cma.c:3096:
> > +	route->path_rec->packet_life_time = test_bit(TIMEOUT_SET, &id_priv->flags) ?
> > 
> > 0001-RDMA-cma-Replace-RMW-with-atomic-bit-ops.patch total: 0 errors, 4 warnings, 118 lines checked
> 
> You're running an old checkpatch. Since commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"), the default line-length is 100. As Linus states in:
> 
> https://lkml.org/lkml/2009/12/17/229
> 
> "... But 80 characters is causing too many idiotic changes."

I'm aware of that thread, but RDMA subsystem continues to use 80 symbols limit.

Thanks

Re: [PATCH for-next v2] RDMA/cma: Replace RMW with atomic bit-ops

[Haakon Bugge]

> On 21 Jun 2021, at 11:54, Leon Romanovsky <leon@kernel.org> wrote:
> 
> On Mon, Jun 21, 2021 at 08:20:47AM +0000, Haakon Bugge wrote:
>> 
>> 
>>> On 21 Jun 2021, at 09:10, Leon Romanovsky <leon@kernel.org> wrote:
>>> 
>>> On Thu, Jun 17, 2021 at 02:59:25PM +0200, Håkon Bugge wrote:
>>>> The struct rdma_id_private contains three bit-fields, tos_set,
>>>> timeout_set, and min_rnr_timer_set. These are set by accessor
>>>> functions without any synchronization. If two or all accessor
>>>> functions are invoked in close proximity in time, there will be
>>>> Read-Modify-Write from several contexts to the same variable, and the
>>>> result will be intermittent.
>>>> 
>>>> Replace with a flag variable and an inline function for set with
>>>> appropriate memory barriers and the use of test_bit().
>>>> 
>>>> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
>>>> Signed-off-by: Hans Westgaard Ry<hans.westgaard.ry@oracle.com>
>>>> 
>>>> ---
>>>> 	v1 -> v2:
>>>> 	   * Removed define wizardry and replaced with a set function
>>>>            with memory barriers. Suggested by Leon.
>>>> 	   * Removed zero-initialization of flags, due to kzalloc(),
>>>>            as suggested by Leon
>>>> 	   * Review comments from Stefan implicitly adapted due to
>>>>            first bullet above
>>>> 	   * Moved defines and inline function from header file to
>>>>            cma.c, as suggested by the undersigned
>>>> 	   * Renamed enum to cm_id_priv_flag_bits as suggested by the
>>>>            undersigned
>>>> ---
>>>> drivers/infiniband/core/cma.c      | 38 +++++++++++++++++++++++++-------------
>>>> drivers/infiniband/core/cma_priv.h |  4 +---
>>>> 2 files changed, 26 insertions(+), 16 deletions(-)
>>> 
>>> This patch generates checkpatch warnings.
>>> 
>>> ➜  kernel git:(rdma-next) git checkpatch
>>> WARNING: line length of 86 exceeds 80 columns
>>> #69: FILE: drivers/infiniband/core/cma.c:1149:
>>> +	if ((*qp_attr_mask & IB_QP_TIMEOUT) && test_bit(TIMEOUT_SET, &id_priv->flags))
>>> 
>>> WARNING: line length of 98 exceeds 80 columns
>>> #73: FILE: drivers/infiniband/core/cma.c:1152:
>>> +	if ((*qp_attr_mask & IB_QP_MIN_RNR_TIMER) && test_bit(MIN_RNR_TIMER_SET, &id_priv->flags))
>>> 
>>> WARNING: line length of 86 exceeds 80 columns
>>> #127: FILE: drivers/infiniband/core/cma.c:3048:
>>> +	u8 tos = test_bit(TOS_SET, &id_priv->flags) ? id_priv->tos : default_roce_tos;
>>> 
>>> WARNING: line length of 84 exceeds 80 columns
>>> #136: FILE: drivers/infiniband/core/cma.c:3096:
>>> +	route->path_rec->packet_life_time = test_bit(TIMEOUT_SET, &id_priv->flags) ?
>>> 
>>> 0001-RDMA-cma-Replace-RMW-with-atomic-bit-ops.patch total: 0 errors, 4 warnings, 118 lines checked
>> 
>> You're running an old checkpatch. Since commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"), the default line-length is 100. As Linus states in:
>> 
>> https://lkml.org/lkml/2009/12/17/229
>> 
>> "... But 80 characters is causing too many idiotic changes."
> 
> I'm aware of that thread, but RDMA subsystem continues to use 80 symbols limit.

I wasn't aware. Where is that documented? Further, it must be a limit that is not enforced. Of the last 100 commits in drivers/infiniband, there are 630 lines longer than 80.


Thxs, Håkon

> 
> Thanks

Re: [PATCH for-next v2] RDMA/cma: Replace RMW with atomic bit-ops

[Jason Gunthorpe]

On Mon, Jun 21, 2021 at 10:46:26AM +0000, Haakon Bugge wrote:

> >> You're running an old checkpatch. Since commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"), the default line-length is 100. As Linus states in:
> >> 
> >> https://lkml.org/lkml/2009/12/17/229
> >> 
> >> "... But 80 characters is causing too many idiotic changes."
> > 
> > I'm aware of that thread, but RDMA subsystem continues to use 80 symbols limit.
> 
> I wasn't aware. Where is that documented? Further, it must be a
> limit that is not enforced. Of the last 100 commits in
> drivers/infiniband, there are 630 lines longer than 80.

Linus said stick to 80 but use your best judgement if going past

It was not a blanket allowance to needless long lines all over the
place.

Jason

Re: [PATCH for-next v2] RDMA/cma: Replace RMW with atomic bit-ops

[Haakon Bugge]

> On 21 Jun 2021, at 16:37, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> On Mon, Jun 21, 2021 at 10:46:26AM +0000, Haakon Bugge wrote:
> 
>>>> You're running an old checkpatch. Since commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"), the default line-length is 100. As Linus states in:
>>>> 
>>>> https://lkml.org/lkml/2009/12/17/229
>>>> 
>>>> "... But 80 characters is causing too many idiotic changes."
>>> 
>>> I'm aware of that thread, but RDMA subsystem continues to use 80 symbols limit.
>> 
>> I wasn't aware. Where is that documented? Further, it must be a
>> limit that is not enforced. Of the last 100 commits in
>> drivers/infiniband, there are 630 lines longer than 80.
> 
> Linus said stick to 80 but use your best judgement if going past
> 
> It was not a blanket allowance to needless long lines all over the
> place.

That is not how I interpreted him:

<quote>
I don't think any kernel developers use a vt100 any more. And even if they 
do, I bet they curse the "24 lines" more than they curse the occasional 
80+ character lines.

I'd be ok with changing the warning to 132 characters, which is another 
perfectly fine historical limit. Or we can split the difference, and say 
"ok, 106 characters is too much". I don't care. But 80 characters is 
causing too many idiotic changes.
</quote>

I think his last sentence pretty much covers the line splitting I did in the v3.

And now we have to explicit add --max-line-length=80 running checkpatch, since it defaults to 100 chars.

Personally, I do not see the value of occasionally require 80 chars line-lengths.


Thxs, Håkon

Re: [PATCH for-next v2] RDMA/cma: Replace RMW with atomic bit-ops

[Jason Gunthorpe]

On Mon, Jun 21, 2021 at 02:58:46PM +0000, Haakon Bugge wrote:
> 
> 
> > On 21 Jun 2021, at 16:37, Jason Gunthorpe <jgg@nvidia.com> wrote:
> > 
> > On Mon, Jun 21, 2021 at 10:46:26AM +0000, Haakon Bugge wrote:
> > 
> >>>> You're running an old checkpatch. Since commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"), the default line-length is 100. As Linus states in:
> >>>> 
> >>>> https://lkml.org/lkml/2009/12/17/229
> >>>> 
> >>>> "... But 80 characters is causing too many idiotic changes."
> >>> 
> >>> I'm aware of that thread, but RDMA subsystem continues to use 80 symbols limit.
> >> 
> >> I wasn't aware. Where is that documented? Further, it must be a
> >> limit that is not enforced. Of the last 100 commits in
> >> drivers/infiniband, there are 630 lines longer than 80.
> > 
> > Linus said stick to 80 but use your best judgement if going past
> > 
> > It was not a blanket allowance to needless long lines all over the
> > place.
> 
> That is not how I interpreted him:

There was a much newer thread on this from Linus, 2009 is really old

Jason

Re: [PATCH for-next v2] RDMA/cma: Replace RMW with atomic bit-ops

[Haakon Bugge]

> On 21 Jun 2021, at 17:12, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> On Mon, Jun 21, 2021 at 02:58:46PM +0000, Haakon Bugge wrote:
>> 
>> 
>>> On 21 Jun 2021, at 16:37, Jason Gunthorpe <jgg@nvidia.com> wrote:
>>> 
>>> On Mon, Jun 21, 2021 at 10:46:26AM +0000, Haakon Bugge wrote:
>>> 
>>>>>> You're running an old checkpatch. Since commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"), the default line-length is 100. As Linus states in:
>>>>>> 
>>>>>> https://lkml.org/lkml/2009/12/17/229
>>>>>> 
>>>>>> "... But 80 characters is causing too many idiotic changes."
>>>>> 
>>>>> I'm aware of that thread, but RDMA subsystem continues to use 80 symbols limit.
>>>> 
>>>> I wasn't aware. Where is that documented? Further, it must be a
>>>> limit that is not enforced. Of the last 100 commits in
>>>> drivers/infiniband, there are 630 lines longer than 80.
>>> 
>>> Linus said stick to 80 but use your best judgement if going past
>>> 
>>> It was not a blanket allowance to needless long lines all over the
>>> place.
>> 
>> That is not how I interpreted him:
> 
> There was a much newer thread on this from Linus, 2009 is really old

Yes, from last year, lkml.org/lkml/2020/5/29/1038

<quote>
Excessive line breaks are BAD. They cause real and every-day problems.

They cause problems for things like "grep" both in the patterns and in
the output, since grep (and a lot of other very basic unix utilities)
is fundamentally line-based.

So the fact is, many of us have long long since skipped the whole
"80-column terminal" model, for the same reason that we have many more
lines than 25 lines visible at a time.

And honestly, I don't want to see patches that make the kernel reading
experience worse for me and likely for the vast majority of people,
based on the argument that some odd people have small terminal
windows.
</quote>

Occasionally enforcing 80-chars line lengths in the RDMA subsystem seems like a strange policy to me :-)


Thxs, Håkon


> 
> Jason

Re: [PATCH for-next v2] RDMA/cma: Replace RMW with atomic bit-ops

[Jason Gunthorpe]

On Mon, Jun 21, 2021 at 03:55:40PM +0000, Haakon Bugge wrote:
> 
> 
> > On 21 Jun 2021, at 17:12, Jason Gunthorpe <jgg@nvidia.com> wrote:
> > 
> > On Mon, Jun 21, 2021 at 02:58:46PM +0000, Haakon Bugge wrote:
> >> 
> >> 
> >>> On 21 Jun 2021, at 16:37, Jason Gunthorpe <jgg@nvidia.com> wrote:
> >>> 
> >>> On Mon, Jun 21, 2021 at 10:46:26AM +0000, Haakon Bugge wrote:
> >>> 
> >>>>>> You're running an old checkpatch. Since commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"), the default line-length is 100. As Linus states in:
> >>>>>> 
> >>>>>> https://lkml.org/lkml/2009/12/17/229
> >>>>>> 
> >>>>>> "... But 80 characters is causing too many idiotic changes."
> >>>>> 
> >>>>> I'm aware of that thread, but RDMA subsystem continues to use 80 symbols limit.
> >>>> 
> >>>> I wasn't aware. Where is that documented? Further, it must be a
> >>>> limit that is not enforced. Of the last 100 commits in
> >>>> drivers/infiniband, there are 630 lines longer than 80.
> >>> 
> >>> Linus said stick to 80 but use your best judgement if going past
> >>> 
> >>> It was not a blanket allowance to needless long lines all over the
> >>> place.
> >> 
> >> That is not how I interpreted him:
> > 
> > There was a much newer thread on this from Linus, 2009 is really old
> 
> Yes, from last year, lkml.org/lkml/2020/5/29/1038
> 
> <quote>
> Excessive line breaks are BAD. They cause real and every-day problems.
> 
> They cause problems for things like "grep" both in the patterns and in
> the output, since grep (and a lot of other very basic unix utilities)
> is fundamentally line-based.
> 
> So the fact is, many of us have long long since skipped the whole
> "80-column terminal" model, for the same reason that we have many more
> lines than 25 lines visible at a time.
> 
> And honestly, I don't want to see patches that make the kernel reading
> experience worse for me and likely for the vast majority of people,
> based on the argument that some odd people have small terminal
> windows.
> </quote>
> 
> Occasionally enforcing 80-chars line lengths in the RDMA subsystem
> seems like a strange policy to me :-)

Well, that threads from Linus seems more forceful than other threads I
recall so <shrug> Still coding-style gives the same guidance I gave
you:

 Statements longer than 80 columns should be broken into sensible chunks,
 unless exceeding 80 columns significantly increases readability and does
 not hide information.

Can't say I have anything more clever to say, other than try to follow
coding style.

Jason

Re: [PATCH for-next v2] RDMA/cma: Replace RMW with atomic bit-ops

[Leon Romanovsky]

On Mon, Jun 21, 2021 at 03:55:40PM +0000, Haakon Bugge wrote:
> 
> 
> > On 21 Jun 2021, at 17:12, Jason Gunthorpe <jgg@nvidia.com> wrote:
> > 
> > On Mon, Jun 21, 2021 at 02:58:46PM +0000, Haakon Bugge wrote:
> >> 
> >> 
> >>> On 21 Jun 2021, at 16:37, Jason Gunthorpe <jgg@nvidia.com> wrote:
> >>> 
> >>> On Mon, Jun 21, 2021 at 10:46:26AM +0000, Haakon Bugge wrote:
> >>> 
> >>>>>> You're running an old checkpatch. Since commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"), the default line-length is 100. As Linus states in:
> >>>>>> 
> >>>>>> https://lkml.org/lkml/2009/12/17/229
> >>>>>> 
> >>>>>> "... But 80 characters is causing too many idiotic changes."
> >>>>> 
> >>>>> I'm aware of that thread, but RDMA subsystem continues to use 80 symbols limit.
> >>>> 
> >>>> I wasn't aware. Where is that documented? Further, it must be a
> >>>> limit that is not enforced. Of the last 100 commits in
> >>>> drivers/infiniband, there are 630 lines longer than 80.
> >>> 
> >>> Linus said stick to 80 but use your best judgement if going past
> >>> 
> >>> It was not a blanket allowance to needless long lines all over the
> >>> place.
> >> 
> >> That is not how I interpreted him:
> > 
> > There was a much newer thread on this from Linus, 2009 is really old
> 
> Yes, from last year, lkml.org/lkml/2020/5/29/1038
> 
> <quote>
> Excessive line breaks are BAD. They cause real and every-day problems.
> 
> They cause problems for things like "grep" both in the patterns and in
> the output, since grep (and a lot of other very basic unix utilities)
> is fundamentally line-based.
> 
> So the fact is, many of us have long long since skipped the whole
> "80-column terminal" model, for the same reason that we have many more
> lines than 25 lines visible at a time.
> 
> And honestly, I don't want to see patches that make the kernel reading
> experience worse for me and likely for the vast majority of people,
> based on the argument that some odd people have small terminal
> windows.
> </quote>
> 
> Occasionally enforcing 80-chars line lengths in the RDMA subsystem seems like a strange policy to me :-)

I prefer to be strict here. We are submitting patches to different
subsystems with different reviewers.

"This adds a few pointles > 80 char lines."
https://lore.kernel.org/linux-rdma/20200907072921.GC19875@lst.de/

> 
> 
> Thxs, Håkon
> 
> 
> > 
> > Jason
> 

Re: [PATCH for-next v2] RDMA/cma: Replace RMW with atomic bit-ops

[Haakon Bugge]

> On 22 Jun 2021, at 08:16, Leon Romanovsky <leon@kernel.org> wrote:
> 
> On Mon, Jun 21, 2021 at 03:55:40PM +0000, Haakon Bugge wrote:
>> 
>> 
>>> On 21 Jun 2021, at 17:12, Jason Gunthorpe <jgg@nvidia.com> wrote:
>>> 
>>> On Mon, Jun 21, 2021 at 02:58:46PM +0000, Haakon Bugge wrote:
>>>> 
>>>> 
>>>>> On 21 Jun 2021, at 16:37, Jason Gunthorpe <jgg@nvidia.com> wrote:
>>>>> 
>>>>> On Mon, Jun 21, 2021 at 10:46:26AM +0000, Haakon Bugge wrote:
>>>>> 
>>>>>>>> You're running an old checkpatch. Since commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"), the default line-length is 100. As Linus states in:
>>>>>>>> 
>>>>>>>> https://lkml.org/lkml/2009/12/17/229
>>>>>>>> 
>>>>>>>> "... But 80 characters is causing too many idiotic changes."
>>>>>>> 
>>>>>>> I'm aware of that thread, but RDMA subsystem continues to use 80 symbols limit.
>>>>>> 
>>>>>> I wasn't aware. Where is that documented? Further, it must be a
>>>>>> limit that is not enforced. Of the last 100 commits in
>>>>>> drivers/infiniband, there are 630 lines longer than 80.
>>>>> 
>>>>> Linus said stick to 80 but use your best judgement if going past
>>>>> 
>>>>> It was not a blanket allowance to needless long lines all over the
>>>>> place.
>>>> 
>>>> That is not how I interpreted him:
>>> 
>>> There was a much newer thread on this from Linus, 2009 is really old
>> 
>> Yes, from last year, lkml.org/lkml/2020/5/29/1038
>> 
>> <quote>
>> Excessive line breaks are BAD. They cause real and every-day problems.
>> 
>> They cause problems for things like "grep" both in the patterns and in
>> the output, since grep (and a lot of other very basic unix utilities)
>> is fundamentally line-based.
>> 
>> So the fact is, many of us have long long since skipped the whole
>> "80-column terminal" model, for the same reason that we have many more
>> lines than 25 lines visible at a time.
>> 
>> And honestly, I don't want to see patches that make the kernel reading
>> experience worse for me and likely for the vast majority of people,
>> based on the argument that some odd people have small terminal
>> windows.
>> </quote>
>> 
>> Occasionally enforcing 80-chars line lengths in the RDMA subsystem seems like a strange policy to me :-)
> 
> I prefer to be strict here. We are submitting patches to different
> subsystems with different reviewers.

Indeed, a fair point. But there are plenty RDMA subsystem only commits with > 80 chars and other warnings, e.g., 

c80a0c52d85c ("RDMA/cma: Add missing error handling of listen_id")

But read me correct. I am probably one of the few here reading from left to right, and interprets c-code that way better than code having excessive line breaks.

Having:
	if (expression_a && expression_b) {
become:
	if (expression_a &&
	    expression_b) {

because the former is let's say 90 chars long, clearly reduces readability in my head. After all, the coding style also says:

<quote>
The coding style document also should not be read as an absolute law which
can never be transgressed.  If there is a good reason to go against the
style (a line which becomes far less readable if split to fit within the
80-column limit, for example), just do it.
</quote>

So, I'll end this here, just summing up my arguments: We have Linus' blessing for longer lines, checkpatch defaults to 100, readability gets better, and coding style allows exception from the 80 chars rule.


Thxs, Håkon


> 
> "This adds a few pointles > 80 char lines."
> https://lore.kernel.org/linux-rdma/20200907072921.GC19875@lst.de/
> 
>> 
>> 
>> Thxs, Håkon
>> 
>> 
>>> 
>>> Jason