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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> On 21 Jun 2021, at 11:54, Leon Romanovsky <leon@xxxxxxxxxx> wrote:
> 
> On Mon, Jun 21, 2021 at 08:20:47AM +0000, Haakon Bugge wrote:
>> 
>> 
>>> On 21 Jun 2021, at 09:10, Leon Romanovsky <leon@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>> Signed-off-by: Hans Westgaard Ry<hans.westgaard.ry@xxxxxxxxxx>
>>>> 
>>>> ---
>>>> 	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





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux