Linux-Doc Archive on lore.kernel.org
 help / color / Atom feed
From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
To: Steven Rostedt <rostedt@goodmis.org>,
	Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	"Sebastian A. Siewior" <bigeasy@linutronix.de>,
	LKML <linux-kernel@vger.kernel.org>,
	Jonathan Corbet <corbet@lwn.net>,
	linux-doc@vger.kernel.org
Subject: Re: [PATCH v4 01/24] Documentation: locking: Describe seqlock design and usage
Date: Tue, 21 Jul 2020 09:15:11 +0200
Message-ID: <20200721071510.GA1175122@debian-buster-darwi.lab.linutronix.de> (raw)
In-Reply-To: <20200720215115.4c5276db@oasis.local.home>

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@goodmis.org> 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

  reply index

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-19 21:45 [PATCH v1 00/25] seqlock: Extend seqcount API with associated locks Ahmed S. Darwish
2020-05-19 21:45 ` [PATCH v1 09/25] Documentation: locking: Describe seqlock design and usage Ahmed S. Darwish
2020-05-22 18:01   ` Peter Zijlstra
2020-05-22 22:24     ` Steven Rostedt
2020-05-25 10:50       ` Ahmed S. Darwish
2020-05-25 11:02         ` Ahmed S. Darwish
2020-05-19 21:45 ` [PATCH v1 10/25] seqlock: Add RST directives to kernel-doc code samples and notes Ahmed S. Darwish
2020-05-22 18:02   ` Peter Zijlstra
2020-05-22 18:03     ` Peter Zijlstra
2020-05-22 18:26       ` Thomas Gleixner
2020-05-22 18:32         ` Peter Zijlstra
2020-05-25  9:36           ` Ahmed S. Darwish
2020-05-25 13:44             ` Peter Zijlstra
2020-05-25 14:07               ` Peter Zijlstra
2020-05-19 21:45 ` [PATCH v1 11/25] seqlock: Add missing kernel-doc annotations Ahmed S. Darwish
2020-05-19 21:45 ` [PATCH v1 12/25] seqlock: Extend seqcount API with associated locks Ahmed S. Darwish
2020-06-08  0:57 ` [PATCH v2 00/18] " Ahmed S. Darwish
2020-06-08  0:57   ` [PATCH v2 01/18] Documentation: locking: Describe seqlock design and usage Ahmed S. Darwish
2020-06-08  0:57   ` [PATCH v2 02/18] seqlock: Properly format kernel-doc code samples Ahmed S. Darwish
2020-06-08  0:57   ` [PATCH v2 03/18] seqlock: Add missing kernel-doc annotations Ahmed S. Darwish
2020-06-30  5:44 ` [PATCH v3 00/20] seqlock: Extend seqcount API with associated locks Ahmed S. Darwish
2020-06-30  5:44   ` [PATCH v3 01/20] Documentation: locking: Describe seqlock design and usage Ahmed S. Darwish
2020-07-06 21:04     ` Peter Zijlstra
2020-07-06 21:12       ` Jonathan Corbet
2020-07-06 21:16       ` Peter Zijlstra
2020-07-07 10:12       ` Ahmed S. Darwish
2020-07-07 12:47         ` Peter Zijlstra
2020-06-30  5:44   ` [PATCH v3 02/20] seqlock: Properly format kernel-doc code samples Ahmed S. Darwish
2020-06-30  5:44   ` [PATCH v3 03/20] seqlock: Add missing kernel-doc annotations Ahmed S. Darwish
2020-07-20 15:55 ` [PATCH v4 00/24] seqlock: Extend seqcount API with associated locks Ahmed S. Darwish
2020-07-20 15:55   ` [PATCH v4 01/24] Documentation: locking: Describe seqlock design and usage Ahmed S. Darwish
2020-07-21  1:35     ` Steven Rostedt
2020-07-21  1:37       ` Steven Rostedt
2020-07-21  5:34       ` Ahmed S. Darwish
2020-07-21  1:44     ` Steven Rostedt
2020-07-21  1:51       ` Steven Rostedt
2020-07-21  7:15         ` Ahmed S. Darwish [this message]
2020-07-20 15:55   ` [PATCH v4 02/24] seqlock: Properly format kernel-doc code samples Ahmed S. Darwish
2020-07-20 15:55   ` [PATCH v4 03/24] seqlock: seqcount_t latch: End read sections with read_seqcount_retry() Ahmed S. Darwish
2020-07-20 15:55   ` [PATCH v4 05/24] seqlock: Add kernel-doc for seqcount_t and seqlock_t APIs Ahmed S. Darwish
2020-07-20 16:49   ` [PATCH v4 00/24] seqlock: Extend seqcount API with associated locks Eric Biggers
2020-07-20 17:33     ` Ahmed S. Darwish

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200721071510.GA1175122@debian-buster-darwi.lab.linutronix.de \
    --to=a.darwish@linutronix.de \
    --cc=bigeasy@linutronix.de \
    --cc=corbet@lwn.net \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Doc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-doc/0 linux-doc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-doc linux-doc/ https://lore.kernel.org/linux-doc \
		linux-doc@vger.kernel.org
	public-inbox-index linux-doc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-doc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git