Hi Jason,
> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Friday, November 23, 2018 1:20 PM
> To: Parav Pandit <parav@xxxxxxxxxxxx>
> Cc: linux-rdma@xxxxxxxxxxxxxxx; dledford@xxxxxxxxxx; Moni Shoua
> <monis@xxxxxxxxxxxx>
> Subject: Re: [PATCH] IB/rxe: Make counters thread safe
>
> On Fri, Nov 23, 2018 at 11:08:47AM -0600, Parav Pandit wrote:
> > Current rxe device counters are not thread safe.
> > When multiple QPs are used, they can be racy.
> > Make them thread safe by making it per cpu.
> >
> > Fixes: 0b1e5b99a48b ("IB/rxe: Add port protocol stats")
> > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > drivers/infiniband/sw/rxe/rxe.c | 17 +++++++++++++++++
> > drivers/infiniband/sw/rxe/rxe_hw_counters.c | 17 +++++++++++++++--
> > drivers/infiniband/sw/rxe/rxe_verbs.h | 14 ++++++++++----
> > 3 files changed, 42 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/infiniband/sw/rxe/rxe.c
> > b/drivers/infiniband/sw/rxe/rxe.c index 383e65c..722aca9 100644
> > +++ b/drivers/infiniband/sw/rxe/rxe.c
> > @@ -39,6 +39,17 @@
> > MODULE_DESCRIPTION("Soft RDMA transport"); MODULE_LICENSE("Dual
> > BSD/GPL");
> >
> > +static int rxe_init_stats(struct rxe_dev *rxe) {
> > + rxe->pcpu_stats = alloc_percpu(struct rxe_stats);
> > + return rxe->pcpu_stats ? 0 : -ENOMEM; }
> > +
> > +static void rxe_cleanup_stats(struct rxe_dev *rxe) {
> > + free_percpu(rxe->pcpu_stats);
> > +}
> > +
> > /* free resources for all ports on a device */ static void
> > rxe_cleanup_ports(struct rxe_dev *rxe) { @@ -64,6 +75,7 @@ static
> > void rxe_cleanup(struct rxe_dev *rxe)
> > rxe_pool_cleanup(&rxe->mc_elem_pool);
> >
> > rxe_cleanup_ports(rxe);
> > + rxe_cleanup_stats(rxe);
> >
> > crypto_free_shash(rxe->tfm);
> > }
> > @@ -267,6 +279,10 @@ static int rxe_init(struct rxe_dev *rxe)
> > /* init default device parameters */
> > rxe_init_device_param(rxe);
> >
> > + err = rxe_init_stats(rxe);
> > + if (err)
> > + return err;
> > +
> > err = rxe_init_ports(rxe);
> > if (err)
> > goto err1;
> > @@ -288,6 +304,7 @@ static int rxe_init(struct rxe_dev *rxe)
> > err2:
> > rxe_cleanup_ports(rxe);
> > err1:
> > + rxe_cleanup_stats(rxe);
> > return err;
> > }
> >
> > diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.c
> > b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
> > index 6aeb7a1..0fb567a 100644
> > +++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
> > @@ -48,6 +48,19 @@
> > [RXE_CNT_SEND_ERR] = "send_err",
> > };
> >
> > +static u64 rxe_stats_read_by_index(const struct rxe_dev *dev, int
> > +index) {
> > + struct rxe_stats *stats;
> > + u64 sum = 0;
> > + int cpu;
> > +
> > + for_each_possible_cpu(cpu) {
> > + stats = per_cpu_ptr(dev->pcpu_stats, cpu);
> > + sum += stats->counters[index];
> > + }
>
> This is still racy with a writer on another CPU.
>
I didn't follow you. If other cpu is updating counter for given index, sum will be updated to user in next read cycle whenever user does it.
Counters are always a snapshot when read.
Idea here is to have right sum when multiple cpu writers are writing, which was not the case before this patch.
> Jason
