LKML Archive on lore.kernel.org
 help / color / Atom feed
From: "Ahmed S. Darwish" <a.darwish@linutronix.de>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Jonathan Corbet <corbet@lwn.net>,
	John Hubbard <jhubbard@nvidia.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"Sebastian A. Siewior" <bigeasy@linutronix.de>
Subject: Re: [PATCH -tip v1 3/3] seqlock: kernel-doc: Specify when preemption is automatically altered
Date: Tue, 8 Dec 2020 15:31:39 +0100
Message-ID: <X8+OS/K0+9ibIEGz@lx-t490> (raw)
In-Reply-To: <20201207204316.GF552508@nvidia.com>

Hi Jason,

On Mon, Dec 07, 2020 at 04:43:16PM -0400, Jason Gunthorpe wrote:
...
>
> The thing that was confusing is if it was appropriate to use a
> seqcount in case where write side preemption was not disabled - which
> is safe only if the read side doesn't spin.
>

No, that's not correct.

What was confusing was that *everyone* in that mm/ thread, including
yourself, thought that write_seqcount_begin() will automatically disable
preemption for plain seqcount_t:

  https://lkml.kernel.org/r/20201030235121.GQ2620339@nvidia.com

Quoting Peter Xu: "My understanding is that we used
raw_write_seqcount_t_begin() because we're with spin lock so assuming we
disabled preemption already."

Quoting you: "write_seqcount_begin - Enforces preemption off".

And that's why this patch explicitly adds to the kernel-doc: "Preemption
will be automatically disabled if and only if the seqcount write
serialization lock is associated, and preemptible."

Honestly, I should've added that statement anyway in my original
"sequence counters with associated locks" submission. I take the blame
for the resulting confusion, and I'm sorry...

~~~~~~~~~~~~~~~~~~~~

Now regarding the other point, where the seqcount_t write side does not
need to disable preemption if the reader does not spin. We've already
discussed that (at length) last time.

There comes a point where you decide what level of documentation to add,
and what level to skip.

Because in the end, you don't want to confuse "Joe, the generic driver
developer" with too much details that's not relevant to their task at
hand.

You want to keep the general case simple, but the special case do-able.
And you also want to encourage people to use the standard write side
critical section entry and exit functions, as much as possible.

Because, oh, look at that:

  [PATCH v2 0/6] seqlock: seqcount_t call sites bugfixes
  https://lkml.kernel.org/r/20200603144949.1122421-1-a.darwish@linutronix.de

Sequence counters are already hard to get right. And driver developers
get things wrong, a lot -- you don't want to confuse them even further.

For developers who're advanced enough to know the difference, they don't
need the kernel-doc anyway. And that's why I've kindly asked to add the
following to your mm/ patch (which you did, thanks):

	/*
	 * Disabling preemption is not needed for the write side, as
	 * the read side does not spin, but goes to mmap_lock.
	 * ...
	 */

And IMHO, that should be enough. Developers of such special cases are
already assumed to know what they're doing.

Thanks a lot,

--
Ahmed S. Darwish
Linutronix GmbH

  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-06 16:21 [PATCH -tip v1 0/3] seqlock: assorted cleanups Ahmed S. Darwish
2020-12-06 16:21 ` [PATCH -tip v1 1/3] Documentation: seqlock: s/LOCKTYPE/LOCKNAME/g Ahmed S. Darwish
2020-12-09 18:38   ` [tip: locking/core] " tip-bot2 for Ahmed S. Darwish
2020-12-06 16:21 ` [PATCH -tip v1 2/3] seqlock: Prefix internal seqcount_t-only macros with a "do_" Ahmed S. Darwish
2020-12-07 20:39   ` Jason Gunthorpe
2020-12-06 16:21 ` [PATCH -tip v1 3/3] seqlock: kernel-doc: Specify when preemption is automatically altered Ahmed S. Darwish
2020-12-07 20:43   ` Jason Gunthorpe
2020-12-08 14:31     ` Ahmed S. Darwish [this message]
2020-12-08 15:46       ` Jason Gunthorpe
2020-12-07 14:07 ` [PATCH -tip v1 0/3] seqlock: assorted cleanups Peter Zijlstra

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=X8+OS/K0+9ibIEGz@lx-t490 \
    --to=a.darwish@linutronix.de \
    --cc=bigeasy@linutronix.de \
    --cc=corbet@lwn.net \
    --cc=jgg@nvidia.com \
    --cc=jhubbard@nvidia.com \
    --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=torvalds@linux-foundation.org \
    --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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

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

Example config snippet for mirrors

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


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