Re: [PATCH for-next] RDMA/nldev: Add copy-on-fork attribute to get sys command

[Gal Pressman <galpress@amazon.com>]

On 05/04/2021 14:57, Leon Romanovsky wrote:
> On Mon, Apr 05, 2021 at 02:47:21PM +0300, Gal Pressman wrote:
>> The new attribute indicates that the kernel copies DMA pages on fork,
>> hence libibverbs' fork support through madvise and MADV_DONTFORK is not
>> needed.
>>
>> The introduced attribute is always reported as supported since the
>> kernel has the patch that added the copy-on-fork behavior. This allows
>> the userspace library to identify older vs newer kernel versions.
>> Extra care should be taken when backporting this patch as it relies on
>> the fact that the copy-on-fork patch is merged, hence no check for
>> support is added.
> 
> Please be more specific, add SHA-1 of that patch and wrote the same
> comment near "err = nla_put_u8(msg, RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK,
> 1);" line.
> 
> Thanks

Should I put the original commit here? There were quite a lot of bug fixes and
followups that are required.

>>
>> Copy-on-fork attribute is read-only, trying to change it through the set
>> sys command will result in an error.
>>
>> Signed-off-by: Gal Pressman <galpress@amazon.com>
>> ---
>> PR was sent:
>> https://github.com/linux-rdma/rdma-core/pull/975
>> ---
>>  drivers/infiniband/core/nldev.c  | 19 ++++++++++++++-----
>>  include/uapi/rdma/rdma_netlink.h |  2 ++
>>  2 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
>> index b8dc002a2478..87c68301c25b 100644
>> --- a/drivers/infiniband/core/nldev.c
>> +++ b/drivers/infiniband/core/nldev.c
>> @@ -146,6 +146,7 @@ static const struct nla_policy nldev_policy[RDMA_NLDEV_ATTR_MAX] = {
>>       [RDMA_NLDEV_ATTR_UVERBS_DRIVER_ID]      = { .type = NLA_U32 },
>>       [RDMA_NLDEV_NET_NS_FD]                  = { .type = NLA_U32 },
>>       [RDMA_NLDEV_SYS_ATTR_NETNS_MODE]        = { .type = NLA_U8 },
>> +     [RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]      = { .type = NLA_U8 },
>>  };
>>
>>  static int put_driver_name_print_type(struct sk_buff *msg, const char *name,
>> @@ -1693,12 +1694,19 @@ static int nldev_sys_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
>>
>>       err = nla_put_u8(msg, RDMA_NLDEV_SYS_ATTR_NETNS_MODE,
>>                        (u8)ib_devices_shared_netns);
>> -     if (err) {
>> -             nlmsg_free(msg);
>> -             return err;
>> -     }
>> +     if (err)
>> +             goto err_nlmsg_free;
>> +
>> +     err = nla_put_u8(msg, RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK, 1);
>> +     if (err)
>> +             goto err_nlmsg_free;
> 
> Is it important to have an ability to fail here? Can we simply ignore
> failure?

Should be fine.

Thanks