| Message ID | 20210818112428.209111-4-markzhang@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Optional counter statistics support | expand |
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. Jason
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.
On Tue, Aug 24, 2021 at 02:22:52PM +0800, Mark Zhang wrote: > 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; 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
diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c index df9e6c5e4ddf..b8b6db98bfdf 100644 --- a/drivers/infiniband/core/counters.c +++ 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; + + mutex_init(&port_counter->opstats->lock); + } } return; @@ -618,6 +627,11 @@ void rdma_counter_init(struct ib_device *dev) fail: for (i = port; i >= rdma_start_port(dev); i--) { port_counter = &dev->port_data[port].port_counter; + if (port_counter && port_counter->opstats) { + mutex_destroy(&port_counter->opstats->lock); + kfree(port_counter->opstats); + port_counter->opstats = NULL; + } kfree(port_counter->hstats); port_counter->hstats = NULL; mutex_destroy(&port_counter->lock); @@ -631,6 +645,10 @@ void rdma_counter_release(struct ib_device *dev) rdma_for_each_port(dev, port) { port_counter = &dev->port_data[port].port_counter; + if (port_counter && port_counter->opstats) { + mutex_destroy(&port_counter->opstats->lock); + kfree(port_counter->opstats); + } kfree(port_counter->hstats); mutex_destroy(&port_counter->lock); } diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index f4814bb7f082..23e1ae50b2e4 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -2597,6 +2597,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops) SET_DEVICE_OP(dev_ops, alloc_mr); SET_DEVICE_OP(dev_ops, alloc_mr_integrity); SET_DEVICE_OP(dev_ops, alloc_mw); + SET_DEVICE_OP(dev_ops, alloc_op_port_stats); SET_DEVICE_OP(dev_ops, alloc_pd); SET_DEVICE_OP(dev_ops, alloc_rdma_netdev); SET_DEVICE_OP(dev_ops, alloc_ucontext); diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index aa7806335cba..e8763d336df1 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -598,6 +598,28 @@ static inline struct rdma_hw_stats *rdma_alloc_hw_stats_struct( return stats; } +/** + * struct rdma_op_counter + * @name - The name of the counter + * @value - The value of the counter + */ +struct rdma_op_counter { + const char *name; + u64 value; +}; + +/** + * struct rdma_op_stats + * @lock - Mutex to protect parallel write access to opstats of counters + * @num_opcounters - How many optional counters there are + * @opcounters - Array of optional counters that are filled in by the drivers + */ +struct rdma_op_stats { + /* Hold this mutex when accessing optional counter */ + struct mutex lock; + int num_opcounters; + struct rdma_op_counter opcounters[]; +}; /* Define bits for the various functionality this port needs to be supported by * the core. @@ -2568,6 +2590,8 @@ struct ib_device_ops { */ int (*get_hw_stats)(struct ib_device *device, struct rdma_hw_stats *stats, u32 port, int index); + struct rdma_op_stats *(*alloc_op_port_stats)(struct ib_device *device, + u32 port_num); /** * Allows rdma drivers to add their own restrack attributes. diff --git a/include/rdma/rdma_counter.h b/include/rdma/rdma_counter.h index 0295b22cd1cd..3531c5061718 100644 --- a/include/rdma/rdma_counter.h +++ b/include/rdma/rdma_counter.h @@ -29,6 +29,7 @@ struct rdma_port_counter { struct rdma_counter_mode mode; struct rdma_hw_stats *hstats; unsigned int num_counters; + struct rdma_op_stats *opstats; struct mutex lock; };