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

[Mark Zhang <markzhang@nvidia.com>]

On 8/24/2021 3:30 AM, Jason Gunthorpe wrote:
> On Wed, Aug 18, 2021 at 02:24:21PM +0300, Mark Zhang wrote:
>> From: Aharon Landau <aharonl@nvidia.com>
>>
>> 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 <aharonl@nvidia.com>
>> Signed-off-by: Neta Ostrovsky <netao@nvidia.com>
>> Signed-off-by: Mark Zhang <markzhang@nvidia.com>
>>   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;
         const char *name;
         u64 value;
};

struct rdma_hw_stats {
         struct mutex    lock; /* Protect lifespan and values[] */
         unsigned long   timestamp;
         unsigned long   lifespan;
         int             num_counters;
         unsigned int    private;  // ?
         u64             flags;    // 0 or FLAG_OPTIONAL
         struct rdma_stat_desc descs[];
         //const char * const *names;
         //u64           value[]; 
 

};

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

Thanks.