Re: [PATCH rdma-next 03/10] RDMA/counters: Support to allocate per-port optional counter statistics

On Tue, Aug 24, 2021 at 02:22:52PM +0800, Mark Zhang wrote:

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

quoted

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

quoted

From: Aharon Landau <redacted>

Add an alloc_op_port_stats() API, as well as related structures, to support
per-port op_stats allocation during counter module initialization.

Signed-off-by: Aharon Landau <redacted>
Signed-off-by: Neta Ostrovsky <redacted>
Signed-off-by: Mark Zhang <redacted>
  drivers/infiniband/core/counters.c | 18 ++++++++++++++++++
  drivers/infiniband/core/device.c   |  1 +
  include/rdma/ib_verbs.h            | 24 ++++++++++++++++++++++++
  include/rdma/rdma_counter.h        |  1 +
  4 files changed, 44 insertions(+)

diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c
index df9e6c5e4ddf..b8b6db98bfdf 100644
+++ b/drivers/infiniband/core/counters.c

@@ -611,6 +611,15 @@ void rdma_counter_init(struct ib_device *dev)
  		port_counter->hstats = dev->ops.alloc_hw_port_stats(dev, port);
  		if (!port_counter->hstats)
  			goto fail;
+
+		if (dev->ops.alloc_op_port_stats) {
+			port_counter->opstats =
+				dev->ops.alloc_op_port_stats(dev, port);
+			if (!port_counter->opstats)
+				goto fail;

It would be nicer to change the normal stats to have more detailed
meta information instead of adding an entire parallel interface like
this.

struct rdma_hw_stats {
	struct mutex	lock;
	unsigned long	timestamp;
	unsigned long	lifespan;
	const char * const *names;

Change the names to a struct

  const struct rdma_hw_stat_desc *descs;

struct rdma_hw_stat_desc {
    const char *name;
    unsigned int flags;
    unsigned int private;
}

and then define a FLAG_OPTIONAL.

Then alot of this oddness goes away.

You might also need a small allocated bitmap to store the
enabled/disabled state

Then the series basically boils down to adding some 'modify counter'
driver op that flips the enable/disable flag

And the netlink patches to expose the additional information.

Maybe it can be defined like this:

struct rdma_stat_desc {
        bool enabled;

This isn't quite a nice because the definition array can't be setup as
a static const inside the driver code. You'd have to memory copy to
set it up.

What does the "private" field mean? Driver-specific? Aren't all counters
driver-specific?

The other patch had used it to identify the counter

Jason