Re: [PATCH rdma-next v1 05/11] RDMA/counter: Add optional counter support

On Tue, Sep 28, 2021 at 08:51:35AM -0300, Jason Gunthorpe wrote:

On Tue, Sep 28, 2021 at 05:03:24PM +0800, Mark Zhang wrote:

quoted

On 9/28/2021 1:03 AM, Jason Gunthorpe wrote:

quoted

On Wed, Sep 15, 2021 at 02:07:24AM +0300, Leon Romanovsky wrote:

quoted

+int rdma_counter_modify(struct ib_device *dev, u32 port, int index, bool enable)
+{
+	struct rdma_hw_stats *stats;
+	int ret;
+
+	if (!dev->ops.modify_hw_stat)
+		return -EOPNOTSUPP;
+
+	stats = ib_get_hw_stats_port(dev, port);
+	if (!stats)
+		return -EINVAL;
+
+	mutex_lock(&stats->lock);
+	ret = dev->ops.modify_hw_stat(dev, port, index, enable);
+	if (!ret)
+		enable ? clear_bit(index, stats->is_disabled) :
+			set_bit(index, stats->is_disabled);

This is not a kernel coding style write out the if, use success
oriented flow

Also, shouldn't this logic protect the driver from being called on
non-optional counters?

We leave it to driver, driver would return failure if modify is not
supported. Is it good?

I think the core code should do it

quoted

quoted

quoted

  	for (i = 0; i < data->stats->num_counters; i++) {
-		attr = &data->attrs[i];
+		if (data->stats->descs[i].flags & IB_STAT_FLAG_OPTIONAL)
+			continue;
+		attr = &data->attrs[pos];
  		sysfs_attr_init(&attr->attr.attr);
  		attr->attr.attr.name = data->stats->descs[i].name;
  		attr->attr.attr.mode = 0444;
  		attr->attr.show = hw_stat_device_show;
  		attr->show = show_hw_stats;
-		data->group.attrs[i] = &attr->attr.attr;
+		data->group.attrs[pos] = &attr->attr.attr;
+		pos++;
  	}

This isn't OK, the hw_stat_device_show() computes the stat index like
this:

	return stat_attr->show(ibdev, ibdev->hw_stats_data->stats,
			       stat_attr - ibdev->hw_stats_data->attrs, 0, buf);

Which assumes the stats are packed contiguously. This only works
because mlx5 is always putting the optional stats at the end.

Yes you are right, thanks. Maybe we can add an "index" field in struct
hw_stats_device/port_attribute, then set it in setup and use it in show.

You could just add a WARN_ON check that optional stats are at the end
I suppose

Everyone adds their counters to the end, last example is bnxt_re
9a381f7e5aa2 ("RDMA/bnxt_re: Add extended statistics counters")

Thanks

Jason