netdev - Re: [PATCH mlx5-next v1] RDMA/mlx5: Cleanup the synchronize

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <549b337b-b51e-c984-a4d8-72f9f738be9c@nvidia.com>
Date:   Sun, 31 Jan 2021 15:24:50 +0200
From:   Yishai Hadas <yishaih@...dia.com>
To:     Saeed Mahameed <saeed@...nel.org>,
        Leon Romanovsky <leon@...nel.org>,
        "Doug Ledford" <dledford@...hat.com>,
        Jason Gunthorpe <jgg@...dia.com>
CC:     Jakub Kicinski <kuba@...nel.org>, <linux-rdma@...r.kernel.org>,
        <netdev@...r.kernel.org>, Yishai Hadas <yishaih@...dia.com>
Subject: Re: [PATCH mlx5-next v1] RDMA/mlx5: Cleanup the synchronize_srcu()
 from the ODP flow

On 1/29/2021 2:23 PM, Saeed Mahameed wrote:
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/mr.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/mr.c
>> index 9eb51f06d3ae..50af84e76fb6 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/mr.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/mr.c
>> @@ -56,6 +56,7 @@ int mlx5_core_create_mkey(struct mlx5_core_dev
>> *dev,
>>          mkey->size = MLX5_GET64(mkc, mkc, len);
>>          mkey->key |= mlx5_idx_to_mkey(mkey_index);
>>          mkey->pd = MLX5_GET(mkc, mkc, pd);
>> +       init_waitqueue_head(&mkey->wait);
>>
>>          mlx5_core_dbg(dev, "out 0x%x, mkey 0x%x\n", mkey_index, mkey-
>>> key);
>>          return 0;
>> diff --git a/include/linux/mlx5/driver.h
>> b/include/linux/mlx5/driver.h
>> index 4901b4fadabb..f9e7036ae5a5 100644
>> --- a/include/linux/mlx5/driver.h
>> +++ b/include/linux/mlx5/driver.h
>> @@ -373,6 +373,8 @@ struct mlx5_core_mkey {
>>          u32                     key;
>>          u32                     pd;
>>          u32                     type;
>> +       struct wait_queue_head wait;
>> +       refcount_t usecount;
> mlx5_core_mkey is used everywhere in mlx5_core and we don't care about
> odp complexity, i would like to keep the core simple and primitive as
> it is today.
> please keep the layer separation and find a way to manage refcount and
> wait queue in mlx5_ib driver..
>
The alternative could really be to come with some wrapped mlx5_ib 
structure that will hold 'mlx5_core_mkey' and will add those two fields.

However,

As ODP is a data path flow we need to minimize any locking scope and 
reduce branches, having the above stuff on 'mlx5_core_mkey' allows 
direct access from any type of mlx5_ib object that uses it.
Having it per object (e.g. mlx5_ib_mr, mlx5_ib_mw, mlx5_ib_devx_mr) 
increasing locking scope and branches on data path to find the refcount 
field per its 'type'.  (see mlx5_core_mkey->type).

Specifically talking, see pagefault_single_data_segment() [1], its mkey 
can be from type MR, MW or DEVX, with current patch having the refcount 
on the core we increase it immediacy and free the lock rather than do 
some lookup based on type and only then increase refcount and  free the 
lock.

In addition,

Wrapping 'mlx5_core_mkey' for this might hit other data path flows as of 
UMR, this may require extra memory access to get the 
'mlx5_core_mkey->key' field upon building the WR, we prefer to avoid it.
See mlx5_ib_create_xlt_wr [2].

So, it seems reasonable to have those properties on the raw mkey 
structure, usage of the refcount / wait is done in mlx5 ib, so no impact 
should be for other users as of that.

[1] 
https://elixir.bootlin.com/linux/v5.11-rc5/source/drivers/infiniband/hw/mlx5/odp.c#L893

[2] 
https://elixir.bootlin.com/linux/v5.11-rc5/source/drivers/infiniband/hw/mlx5/mr.c#L1092

Yishai