On Mon, 2016-11-28 at 23:02 +0100, Lino Sanfilippo wrote:

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)"


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.
 */