> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Sunday, November 25, 2018 9:19 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 Sat, Nov 24, 2018 at 02:27:53PM +0000, Parav Pandit wrote:
>
> > > > +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.
>
> Where is the snapshot?
>
> Reading a u64 is not possible atomically on 32 bit platforms, you get garbage
> if there is a race with a writer.
>
Yes. Didn't consider 32-bit.
Additionally, I am not using this_cpu_inc() variant so it is not preemption safe given that we update these counters from thread context too.
So atomic64_t is better choice that addresses both the issues.
> Jason
