On Wed, May 22, 2019 at 02:10:42PM -0300, Jason Gunthorpe wrote:
> On Mon, Apr 29, 2019 at 11:34:49AM +0300, Leon Romanovsky wrote:
> > From: Mark Zhang <markz@xxxxxxxxxxxx>
> >
> > Since a QP can only be bound to one counter, then if it is bound to a
> > separate counter, for backward compatibility purpose, the statistic
> > value must be:
> > * stat of default counter
> > + stat of all running allocated counters
> > + stat of all deallocated counters (history stats)
> >
> > Signed-off-by: Mark Zhang <markz@xxxxxxxxxxxx>
> > Reviewed-by: Majd Dibbiny <majd@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > drivers/infiniband/core/counters.c | 99 +++++++++++++++++++++++++++++-
> > drivers/infiniband/core/device.c | 8 ++-
> > drivers/infiniband/core/sysfs.c | 10 ++-
> > include/rdma/rdma_counter.h | 5 +-
> > 4 files changed, 113 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c
> > index 36cd9eca1e46..f598b1cdb241 100644
> > +++ b/drivers/infiniband/core/counters.c
> > @@ -146,6 +146,20 @@ static int __rdma_counter_bind_qp(struct rdma_counter *counter,
> > return ret;
> > }
> >
> > +static void counter_history_stat_update(const struct rdma_counter *counter)
> > +{
> > + struct ib_device *dev = counter->device;
> > + struct rdma_port_counter *port_counter;
> > + int i;
> > +
> > + port_counter = &dev->port_data[counter->port].port_counter;
> > + if (!port_counter->hstats)
> > + return;
> > +
> > + for (i = 0; i < counter->stats->num_counters; i++)
> > + port_counter->hstats->value[i] += counter->stats->value[i];
> > +}
> > +
> > static int __rdma_counter_unbind_qp(struct ib_qp *qp, bool force)
> > {
> > struct rdma_counter *counter = qp->counter;
> > @@ -285,8 +299,10 @@ int rdma_counter_unbind_qp(struct ib_qp *qp, bool force)
> > return ret;
> >
> > rdma_restrack_put(&counter->res);
> > - if (atomic_dec_and_test(&counter->usecnt))
> > + if (atomic_dec_and_test(&counter->usecnt)) {
> > + counter_history_stat_update(counter);
> > rdma_counter_dealloc(counter);
> > + }
> >
> > return 0;
> > }
> > @@ -307,21 +323,98 @@ int rdma_counter_query_stats(struct rdma_counter *counter)
> > return ret;
> > }
> >
> > -void rdma_counter_init(struct ib_device *dev)
> > +static u64 get_running_counters_hwstat_sum(struct ib_device *dev,
> > + u8 port, u32 index)
> > +{
> > + struct rdma_restrack_entry *res;
> > + struct rdma_restrack_root *rt;
> > + struct rdma_counter *counter;
> > + unsigned long id = 0;
> > + u64 sum = 0;
> > +
> > + rt = &dev->res[RDMA_RESTRACK_COUNTER];
> > + xa_lock(&rt->xa);
> > + xa_for_each(&rt->xa, id, res) {
> > + if (!rdma_restrack_get(res))
> > + continue;
>
> Why do we need to get refcounts if we are holding the xa_lock?
Don't we need to protect an entry itself from disappearing?
>
> > +
> > + counter = container_of(res, struct rdma_counter, res);
> > + if ((counter->device != dev) || (counter->port != port))
> > + goto next;
> > +
> > + if (rdma_counter_query_stats(counter))
> > + goto next;
>
> And rdma_counter_query_stats does
>
> + mutex_lock(&counter->lock);
>
> So this was never tested as it will insta-crash with lockdep.
>
> Presumably this is why it is using xa_for_each and restrack_get - but
> it needs to drop the lock after successful get.
>
> This sort of comment applies to nearly evey place in this series that
> uses xa_for_each.
>
> This needs to be tested with lockdep.
I use LOCKDEP.
>
> Jason
