Re: [PATCH v17 02/20] netlink: add new family to manage ULP_DDP enablement and stats

[Aurelien Aptel <aaptel@nvidia.com>]

Hi Jiri,

Jiri Pirko <jiri@resnulli.us> writes:
>>+definitions:
>>+  -
>>+    type: enum
>>+    name: cap
>>+    entries:
>>+      - nvme-tcp
>>+      - nvme-tcp-ddgst-rx
>>+
>>+uapi-header: linux/ulp_ddp_nl.h
>
> Not needed.
> Hmm, Jakub, why this is not only allowed in genetlink-legacy?

Ok, we will remove it and use the default location.

>>+
>>+attribute-sets:
>>+  -
>>+    name: stat
>
> "stats"?

Sure.

>>+    attributes:
>>+      -
>>+        name: ifindex
>>+        doc: interface index of the net device.
>>+        type: u32
>>+      -
>>+        name: pad
>>+        type: pad
>>+      -
>>+        name: rx-nvmeotcp-sk-add
>>+        doc: Sockets successfully configured for NVMeTCP offloading.
>>+        type: u64
>>+      -
>>+        name: rx-nvmeotcp-sk-add-fail
>
> "nvmeotcp" should stand for "nvme over tcp"? Why not to name it just
> "rx-nvem-tcp-x"?

Ok, we will rename them.

>>+  -
>>+    name: dev
>
> If this is attribute set for "caps-get"/"caps-set" only, why it is not
> called "caps"?

Ok, we will rename it.

>>+    attributes:
>>+      -
>>+        name: ifindex
>>+        doc: interface index of the net device.
>>+        type: u32
>>+      -
>>+        name: hw
>>+        doc: bitmask of the capabilities supported by the device.
>>+        type: u64
>>+        enum: cap
>>+        enum-as-flags: true
>>+      -
>>+        name: active
>>+        doc: bitmask of the capabilities currently enabled on the device.
>>+        type: u64
>>+        enum: cap
>>+        enum-as-flags: true
>>+      -
>>+        name: wanted
>
> For all caps related attrs, either put "caps" into the name or do that
> and put it in a caps nest

We will rename the attribute-set to "caps" and leave hw, active, wanted as-is.

>>+operations:
>>+  list:
>>+    -
>>+      name: get
>>+      doc: Get ULP DDP capabilities.
>
> This is for capabalities only, nothing else?
> If yes, why not to name the op "caps-get"/"caps-set"?
> If not and this is related to "dev", name it perhaps "dev-get"?
> I mean, you should be able to align the op name and attribute set name.

Yes it is for capabilities. We will rename the operations to caps-get/caps-set.

>>+      attribute-set: dev
>>+      do:
>>+        request:
>>+          attributes:
>>+            - ifindex
>>+        reply: &dev-all
>>+          attributes:
>>+            - ifindex
>>+            - hw
>>+            - active
>>+        pre: ulp_ddp_get_netdev
>>+        post: ulp_ddp_put_netdev
>>+      dump:
>>+        reply: *dev-all
>>+    -
>>+      name: stats
>
> "stats-get" ?

Ok

>>+++ b/net/core/ulp_ddp_nl.c
>>@@ -0,0 +1,388 @@
>>+// SPDX-License-Identifier: GPL-2.0
>>+/*
>>+ * ulp_ddp.c
>>+ *    Author: Aurelien Aptel <aaptel@nvidia.com>
>>+ *    Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES.  All rights reserved.
>>+ */
>>+#include <net/ulp_ddp.h>
>>+#include "ulp_ddp_gen_nl.h"
>>+
>>+#define ULP_DDP_STATS_CNT (sizeof(struct netlink_ulp_ddp_stats) / sizeof(u64))
>>+
>>+struct reply_data {
>
> Some sane prefix for structs and fuctions would be nice. That applies to
> the whole code.

Sure, we will add ulp_ddp prefix to all symbols. 

> What's "data"? Reading the code, this sounds very vague. Try to be more
> precise in struct and functions naming.

It's the data used to write the response message.
We will rename it to ulp_ddp_reply_context.

>>+
>>+/* pre_doit */
>>+int ulp_ddp_get_netdev(const struct genl_split_ops *ops,
>>+                     struct sk_buff *skb, struct genl_info *info)
>
> Could you perhaps check ulp_ddp_caps here instead of doing it on multiple
> places over and over. Of course fill-up a proper extack message in case
> the check fails.

Sounds good.

>>+      if (!data->dev) {
>>+              kfree(data);
>>+              NL_SET_BAD_ATTR(info->extack,
>>+                              info->attrs[ULP_DDP_A_DEV_IFINDEX]);
>
> Fill-up a meaningful extack message as well please using
> NL_SET_ERR_MSG()

Ok

>>+              return -ENOENT;
>
>                 return -ENODEV;
> Maybe?

Ok

>>+      struct reply_data *data = info->user_ptr[0];
>>+
>>+      if (data) {
>
> How "data" could be NULL here?
>
>>+              if (data->dev)
>
> How "data->dev" could be NULL here?

It can't, we will remove all the checks.

>>+              for (i = 0; i < ULP_DDP_STATS_CNT; i++, attr++)
>>+                      if (nla_put_u64_64bit(rsp, attr, val[i],
>
> This rely on a struct layout is dangerous and may easily lead in future
> to put attrs which are not define in enum. Please properly put each stat
> by using attr enum value.

Sure, we will explicitely set all stats attributes.

>>+
>>+      /* if (req_mask & ~all_bits) */
>>+      bitmap_fill(all_bits, ULP_DDP_C_COUNT);
>>+      bitmap_andnot(tmp, req_mask, all_bits, ULP_DDP_C_COUNT);
>>+      if (!bitmap_empty(tmp, ULP_DDP_C_COUNT))
>
> Please make sure you always fillup a proper extack message, always.

Noted. This check was redundant so we removed it (bits beyond
ULP_DDP_C_COUNT are not checked).

>>+      notify = !!ret;
>>+      ret = prepare_data(data, ULP_DDP_CMD_SET);
>
> Why you send it back for set? (leaving notify aside)

We send back the final caps so userspace can see and compare what was
requested vs the result of what the driver could enable.
This is following the convention of ethtool features.

>>+int ulp_ddp_nl_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>>+{
>>+      struct net *net = sock_net(skb->sk);
>>+      struct net_device *netdev;
>>+      struct reply_data data;
>>+      int err = 0;
>>+
>>+      rtnl_lock();
>>+      for_each_netdev_dump(net, netdev, cb->args[0]) {
>
> Is this function necessary? I mean, do you have usecase to dump the
> the caps for all devices in the netns? If no, remove this dump op.
> If yes, could you please do it without holding rtnl? You don't need rtnl
> for do ops either.

This is not necessary, we will remove the dump ops.

>>+static int __init ulp_ddp_init(void)
>>+{
>>+      int err;
>>+
>>+      err = genl_register_family(&ulp_ddp_nl_family);
>>+      if (err)
>>+              return err;
>>+
>>+      return 0;
>
> The whole function reduces just to:
>         return genl_register_family(&ulp_ddp_nl_family);

Ok

Thanks