On Fri, Jul 26, 2019 at 04:14:05PM -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 26 Jul 2019 14:02:01 -0400
> Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> escreveu:
>
> > On Fri, Jul 26, 2019 at 09:51:35AM -0300, Mauro Carvalho Chehab wrote:
> > [snip]
> > > +| until the assignment to ``gp``, by which time both fields are fully |
> > > +| initialized. So reordering the assignments to ``p->a`` and ``p->b`` |
> > > +| cannot possibly cause any problems. |
> > > ++-----------------------------------------------------------------------+
> > > +
> > > +It is tempting to assume that the reader need not do anything special to
> > > +control its accesses to the RCU-protected data, as shown in
> > > +``do_something_gp_buggy()`` below:
> > > +
> > > + ::
> > > +
> > > + 1 bool do_something_gp_buggy(void)
> > > + 2 {
> > > + 3 rcu_read_lock();
> > > + 4 p = gp; /* OPTIMIZATIONS GALORE!!! */
> > > + 5 if (p) {
> > > + 6 do_something(p->a, p->b);
> > > + 7 rcu_read_unlock();
> > > + 8 return true;
> > > + 9 }
> > > + 10 rcu_read_unlock();
> > > + 11 return false;
> > > + 12 }
> > > +
> > > +However, this temptation must be resisted because there are a
> > > +surprisingly large number of ways that the compiler (to say nothing of
> > > +`DEC Alpha CPUs <https://h71000.www7.hp.com/wizard/wiz_2637.html>`__)
> > > +can trip this code up. For but one example, if the compiler were short
> > > +of registers, it might choose to refetch from ``gp`` rather than keeping
> > > +a separate copy in ``p`` as follows:
> > > +
> > > + ::
> > > +
> > > + 1 bool do_something_gp_buggy_optimized(void)
> > > + 2 {
> > > + 3 rcu_read_lock();
> > > + 4 if (gp) { /* OPTIMIZATIONS GALORE!!! */
> > > + 5 do_something(gp->a, gp->b);
> > > + 6 rcu_read_unlock();
> > > + 7 return true;
> > > + 8 }
> > > + 9 rcu_read_unlock();
> > > + 10 return false;
> > > + 11 }
> > > +
> > > +If this function ran concurrently with a series of updates that replaced
> > > +the current structure with a new one, the fetches of ``gp->a`` and
> > > +``gp->b`` might well come from two different structures, which could
> > > +cause serious confusion. To prevent this (and much else besides),
> > > +``do_something_gp()`` uses ``rcu_dereference()`` to fetch from ``gp``:
> > > +
> > > + ::
> > > +
> > > + 1 bool do_something_gp(void)
> > > + 2 {
> > > + 3 rcu_read_lock();
> > > + 4 p = rcu_dereference(gp);
> > > + 5 if (p) {
> > > + 6 do_something(p->a, p->b);
> > > + 7 rcu_read_unlock();
> > > + 8 return true;
> > > + 9 }
> > > + 10 rcu_read_unlock();
> > > + 11 return false;
> > > + 12 }
> > > +
> > > +The ``rcu_dereference()`` uses volatile casts and (for DEC Alpha) memory
> > > +barriers in the Linux kernel. Should a `high-quality implementation of
> > > +C11 ``memory_order_consume``
> > > +[PDF] <http://www.rdrop.com/users/paulmck/RCU/consume.2015.07.13a.pdf>`__
> > > +ever appear, then ``rcu_dereference()`` could be implemented as a
> > > +``memory_order_consume`` load. Regardless of the exact implementation, a
> > > +pointer fetched by ``rcu_dereference()`` may not be used outside of the
> > > +outermost RCU read-side critical section containing that
> > > +``rcu_dereference()``, unless protection of the corresponding data
> > > +element has been passed from RCU to some other synchronization
> > > +mechanism, most commonly locking or `reference
> > > +counting <https://www.kernel.org/doc/Documentation/RCU/rcuref.txt>`__.
> >
> > From the make htmldocs output, this appears very poorly for me, I get
> > something like this in the browser:
> >
> > The rcu_dereference() uses volatile casts and (for DEC Alpha) memory barriers
> > in the Linux kernel. Should a high-quality implementation of C11
> > ``memory_order_consume` [PDF]
> > <http://www.rdrop.com/users/paulmck/RCU/consume.2015.07.13a.pdf>`__ ever
> > appear, then rcu_dereference() could be implemented as a memory_order_consume
> > load.
> >
> > Is there a syntax issue here?
>
> Maybe. I tested those with Sphinx 2.0.1. Didn't test with older versions.
>
> I'll do some tests with Sphinx 1.7.9 (with is the current minimal
> recommended version) and do some cleanup on those references.
Ok, one more thing is broken, clicking links such as "Parallelism Facts of
Life" does not jump to the corresponding section.
Would you mind fixing this, add the description of changes you made (which
you shared in an earlier reply), fixing the above Sphinx issue, and then
resend it?
Otherwise, I believe it looks sane.
> > One more feedback,
> > the image under "RCU read-side critical section that started before the current
> > grace period:" should probably be blown up a bit.
>
> Unfortunately, the Kernel Sphinx image extension doesn't allow image scaling.
The current scale appears fine to me, it is not a big deal since it is clear.
> We had to add our own image extension at the Kernel, as otherwise,
> for every image, we would need to add one parser for PDF and another
> one for SVG.
>
> We would also need an extra parser for DOT.
>
> Markus solved all the 3 image formats with a single extension, but
> it currently doesn't allow passing the image size.
Cool.
