netdev - RE: Aw: Re: [PATCH] mlx4: give precise rx/tx bytes/packets counters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 30 Nov 2016 15:28:02 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Eric Dumazet' <eric.dumazet@...il.com>,
        Lino Sanfilippo <LinoSanfilippo@....de>
CC:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Tariq Toukan <tariqt@...lanox.com>
Subject: RE: Aw: Re: [PATCH] mlx4: give precise rx/tx bytes/packets counters

From: Eric Dumazet
> Sent: 29 November 2016 00:19
> On Mon, 2016-11-28 at 23:02 +0100, Lino Sanfilippo wrote:
> > Hi Eric,
> >
> > On 25.11.2016 20:19, Eric Dumazet wrote:
> > > On Fri, 2016-11-25 at 17:30 +0100, Lino Sanfilippo wrote:
> > >> Hi,
> > >>
> > >>
> > >> >
> > >> > The READ_ONCE() are documenting the fact that no lock is taken to fetch
> > >> > the stats, while another cpus might being changing them.
> > >> >
> > >> > I had no answer yet from https://patchwork.ozlabs.org/patch/698449/
> > >> >
> > >> > So I thought it was not needed to explain this in the changelog, given
> > >> > that it apparently is one of the few things that can block someone to
> > >> > understand one of my changes :/
> > >> >
> > >> > Apparently nobody really understands READ_ONCE() purpose, it is really a
> > >> > pity we have to explain this over and over.
> > >> >
> > >>
> > >> Even at the risk of showing once more a lack of understanding for
> > >> READ_ONCE():
> > >> Does not a READ_ONCE() have to e paired with some kind of
> > >> WRITE_ONCE()?
> > >
> > > You are right.
> > >
> > > Although in this case, the producers are using a lock, and do
> > >
> > > ring->packets++;
> > >
> > > We hopefully have compilers/cpus that do not put intermediate garbage in
> > > ring->packets while doing the increment.
> > >
> > > One problem with :
> > >
> > > WRITE_ONCE(ring->packets, ring->packets + 1);
> > >
> > > is that gcc no longer uses an INC instruction.
> >
> > I see. So we would have to do something like
> >
> > tmp = ring->packets;
> > tmp++;
> > WRITE_ONCE(ring->packets, tmp);
> 
> 
> Well, gcc will generate a code with more instructions than a mere
> 
> "inc  offset(%register)"

Are you sure??
Last I looked gcc seemed to convert 'foo++' to 'foo = foo + 1' before
generating any code.
It might then optimise that back to a memory increment, but that would
also happen if you'd coded the latter form.

> Which is kind of unfortunate, given it is the fast path.
> 
> Better add a comment, like :
> 
> /* We should use WRITE_ONCE() to pair with the READ_ONCE() found in xxxx()
>  * But gcc would generate non optimal code.
>  */

Actually while READ_ONCE() is generally useful - to get a snapshot of a changing value.

WRITE_ONCE() isn't a pairing - the compiler is highly unlikely to write a
location twice.
You might want an annotation to ensure is doesn't assume it can read the value
back (write through volatile pointer). But that has nothing to do with how readers behave.

	David