Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> > On Tue, May 19, 2020 at 11:45:31PM +0200, Ahmed S. Darwish wrote:
> > > diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
> > > index d35be7709403..2a4af746b1da 100644
> > > --- a/include/linux/seqlock.h
> > > +++ b/include/linux/seqlock.h
> > > @@ -1,36 +1,15 @@
> > > /* SPDX-License-Identifier: GPL-2.0 */
> > > #ifndef __LINUX_SEQLOCK_H
> > > #define __LINUX_SEQLOCK_H
> > > +
> > > /*
> > > - * Reader/writer consistent mechanism without starving writers. This type of
> > > - * lock for data where the reader wants a consistent set of information
> > > - * and is willing to retry if the information changes. There are two types
> > > - * of readers:
> > > - * 1. Sequence readers which never block a writer but they may have to retry
> > > - * if a writer is in progress by detecting change in sequence number.
> > > - * Writers do not wait for a sequence reader.
> > > - * 2. Locking readers which will wait if a writer or another locking reader
> > > - * is in progress. A locking reader in progress will also block a writer
> > > - * from going forward. Unlike the regular rwlock, the read lock here is
> > > - * exclusive so that only one locking reader can get it.
> > > + * seqcount_t / seqlock_t - a reader-writer consistency mechanism with
> > > + * lockless readers (read-only retry loops), and no writer starvation.
> > > *
> > > - * 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 for full description.
> >
> > So I really really hate that... I _much_ prefer code comments to crappy
> > documents.
>
> Agreed. Comments are much less likely to bitrot than documents. The
> farther away the documentation is from the code, the quicker it becomes
> stale.
>
> It's fine to add "See Documentation/..." but please don't *ever* remove
> comments that's next to the actual code.
>
This patch was unfairly cut at the hunk above :)
If you follow the rest of it, you see that the documentation has just
moved 3 lines below:
/*
- * Version using sequence counter only.
- * This can be used when code has its own mutex protecting the
- * updating starting before the write_seqcountbeqin() and ending
- * after the write_seqcount_end().
+ * Sequence counters (seqcount_t)
+ *
+ * The raw counting mechanism without any writer protection. Write side
+ * critical sections must be serialized and readers on the same CPU
+ * (e.g. through preemption or interrupts) must be excluded.
+ *
+ * If it's desired to automatically handle the sequence counter writer
+ * serialization and non-preemptibility requirements, use a sequential
+ * lock (seqlock_t) instead.
+ *
+ * See Documentation/locking/seqlock.rst
*/
+
typedef struct seqcount {
and:
+/*
+ * Sequential locks (seqlock_t)
+ *
+ * Sequence counters with an embedded spinlock for writer serialization
+ * and non-preemptibility.
+ *
+ * See Documentation/locking/seqlock.rst
+ */
+
typedef struct {
struct seqcount seqcount;
spinlock_t lock;
} seqlock_t;
This was done because, as said in the commit log, documentation of
seqcount_t and seqlock_t was originally intermingled. This is incorrect
and confusing since the usage constrains for each type are vastly
different.
Then, the brlock comment:
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.
was removed not because it's moved to Documentation/locking/seqlock.rst,
but because it's obsolete: 0f6ed63b1707 ("no need to keep brlock macros
anymore...").
Thanks,
--
Ahmed S. Darwish
Linutronix GmbH
