Re: [PATCH rdma-next 06/10] RDMA/nldev: Add support to add and remove optional counters

On 8/24/2021 3:42 AM, Jason Gunthorpe wrote:

On Wed, Aug 18, 2021 at 02:24:24PM +0300, Mark Zhang wrote:

quoted

From: Aharon Landau <redacted>

This patch adds the ability to add/remove optional counter to a link
through RDMA netlink. Limit it to users with ADMIN capability only.

Examples:
$ sudo rdma statistic add link rocep8s0f0/1 optional-set cc_rx_ce_pkts
$ sudo rdma statistic remove link rocep8s0f0/1 optional-set cc_rx_ce_pkts

Signed-off-by: Aharon Landau <redacted>
Signed-off-by: Neta Ostrovsky <redacted>
Signed-off-by: Mark Zhang <redacted>
  drivers/infiniband/core/counters.c | 50 ++++++++++++++++
  drivers/infiniband/core/device.c   |  2 +
  drivers/infiniband/core/nldev.c    | 93 ++++++++++++++++++++++++++++++
  include/rdma/ib_verbs.h            |  7 +++
  include/rdma/rdma_counter.h        |  4 ++
  include/rdma/rdma_netlink.h        |  1 +
  include/uapi/rdma/rdma_netlink.h   |  9 +++
  7 files changed, 166 insertions(+)

diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c

...

quoted

+static int nldev_stat_set_op_stat(struct sk_buff *skb,
+				  struct nlmsghdr *nlh,
+				  struct netlink_ext_ack *extack,
+				  bool cmd_add)
+{

...

quoted

+
+	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
+			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
+					 (cmd_add ?
+					  RDMA_NLDEV_CMD_STAT_ADD_OPCOUNTER :
+					  RDMA_NLDEV_CMD_STAT_REMOVE_OPCOUNTER)),
+			0, 0);
+
+	if (cmd_add)
+		ret = rdma_opcounter_add(device, port, opcounter);
+	else
+		ret = rdma_opcounter_remove(device, port, opcounter);
+	if (ret)
+		goto err_msg;
+
+	nlmsg_end(msg, nlh);

Shouldn't the netlink message for a 'set' always return the current
value of the thing being set on return? Eg the same output that GET
would generate?

May I ask why can't just return an error code?

quoted

+static int nldev_stat_add_op_stat_doit(struct sk_buff *skb,
+				       struct nlmsghdr *nlh,
+				       struct netlink_ext_ack *extack)
+{
+	return nldev_stat_set_op_stat(skb, nlh, extack, true);
+}
+
+static int nldev_stat_remove_op_stat_doit(struct sk_buff *skb,
+					  struct nlmsghdr *nlh,
+					  struct netlink_ext_ack *extack)
+{
+	return nldev_stat_set_op_stat(skb, nlh, extack, false);
+}
+
  static int nldev_stat_set_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
  			       struct netlink_ext_ack *extack)
  {

@@ -2342,6 +2427,14 @@ static const struct rdma_nl_cbs nldev_cb_table[RDMA_NLDEV_NUM_OPS] = {
  		.dump = nldev_res_get_mr_raw_dumpit,
  		.flags = RDMA_NL_ADMIN_PERM,
  	},
+	[RDMA_NLDEV_CMD_STAT_ADD_OPCOUNTER] = {
+		.doit = nldev_stat_add_op_stat_doit,
+		.flags = RDMA_NL_ADMIN_PERM,
+	},
+	[RDMA_NLDEV_CMD_STAT_REMOVE_OPCOUNTER] = {
+		.doit = nldev_stat_remove_op_stat_doit,
+		.flags = RDMA_NL_ADMIN_PERM,
+	},
  };

And here I wonder if this is the cannonical way to manipulate lists of
strings in netlink? I'm trying to think of another case like this, did
you reference something?

For add/remove, we only support one op-counter at one time (for simplicity), so it's just a string, not a list of string.

This is supported:
#  rdma stat add link mlx5_0/1 optional-set cc_rx_ce_pkts

This is not supported:
# rdma stat add link mlx5_0/1 optional-set cc_rx_ce_pkts cc_tx_cnp_pkts

Are you sure this shouldn't be done via some set on some counter
object?

Currently we don't support do on a counter object, just per-port.

Jason