Re: [PATCH v4 01/24] Documentation: locking: Describe seqlock design and usage

[Ahmed S. Darwish]

On Mon, Jul 20, 2020 at 09:51:15PM -0400, Steven Rostedt wrote:
> On Mon, 20 Jul 2020 21:44:00 -0400
> Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> > > - * This is not as cache friendly as brlock. Also, this may not work well
> > > - * for data that contains pointers, because any writer could
> > > - * invalidate a pointer that a reader was following.
> > > + * See Documentation/locking/seqlock.rst
> >
> > I absolutely hate it when I see this.
> >
> > I much rather have the documentation next to the code. Because
> > honestly, I trust that comments next to the code will get updated if
> > the code changes much more likely than comments buried in the
> > Documentation directory.
> >
> > It's also more likely that I wont even bother looking at the doc
> > (because I wont trust it to be up to date) and just read the code and
> > try to figure it out. Or look at how others have used it.
>
> Never mind,
>
> I see that kerneldoc is added in patch 5, which helps.
>

Even looking at the current patch #1 *in isolation*, no relevant
comments were removed from seqlock.h at all. They were just moved closer
to the seqcount_t and seqlock_t structure definitions.

The original comments were horrible. They intermingled seqcount_t and
seqlock_t descriptions in a *very* ambiguous, sometimes even in the same
sentence. It was misleading, as the usage constrains for each data type
(especially on the write side) are vastly different.

Even the new KCSAN comment, which was freshly merged this release cycle,
got tainted by such already-existing incoherence. It kept talking about
the "seqlock interface" while it actually meant the seqcount_t
interface.  Then at the end, it realized the semantical ambiguity and
noted how the "seqlock interface" does *not* cover seqlock_t.

Moreover, this seqcount_t/seqlock_t documentation intermingling led to
the critical usage constraint of disabling preemption before entering a
seqcount_t write side critical section never getting explicitly
mentioned. This has (naturally) led to a considerable number of buggy
call sites, and the fixes are now merged.

I've tried to fix the problem at the roots, and basically identified
three major problems:

  1. The seqcount_t/seqlock_t intermingling is confusing everyone. That
     is, both of call-site developers *and* core kernel engineers.

  2. The big picture of seqlock.h was never expressed in a sufficient
     detail.

  3. The usage constraints for the exported seqlock.h APIs were never
     explicitly mentioned.. leading to buggy call sites.

To fix #1, I've changed the all of the existing seqlock.h comments to
explicitly mention either "seqcount_t" or "seqlock_t". Then, divided the
the top seqlock.h comment into a seqcount_t part and a seqlock_t one.
Afterwards, the whole seqlock.h header file code was grouped into
"sections", as seen in patch #4.

To fix #2, Documentation/locking/seqlock.rst was introduced and boldly
referenced at the header-file top comment.

To fix #3, kernel-doc was added for all of the seqlock.h exported APIs.

@Peter, kindly note that all of the above *is exactly why* I've resisted
hiding the new seqcount_LOCKTYPE_t APIs introduced by this patch series
inside generative macros. All the parts that will be referenced by
call-site developers are kept both explicit and kernel-doc commented.

Thanks!

--
Ahmed S. Darwish
Linutronix GmbH