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