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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > Signed-off-by: Neta Ostrovsky <netao@xxxxxxxxxx>
> > > Signed-off-by: Mark Zhang <markzhang@xxxxxxxxxx>
> > > 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
