From: Eric Dumazet

Sent: 29 November 2016 00:19
On Mon, 2016-11-28 at 23:02 +0100, Lino Sanfilippo wrote:

quoted

Hi Eric,

On 25.11.2016 20:19, Eric Dumazet wrote:

quoted

On Fri, 2016-11-25 at 17:30 +0100, Lino Sanfilippo wrote:

quoted

Hi,

quoted

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