From: "Ahmed S. Darwish" <firstname.lastname@example.org>
To: Jason Gunthorpe <email@example.com>
Cc: Peter Zijlstra <firstname.lastname@example.org>,
Ingo Molnar <email@example.com>, Will Deacon <firstname.lastname@example.org>,
Linus Torvalds <email@example.com>,
Thomas Gleixner <firstname.lastname@example.org>,
"Paul E. McKenney" <email@example.com>,
Steven Rostedt <firstname.lastname@example.org>,
Jonathan Corbet <email@example.com>,
John Hubbard <firstname.lastname@example.org>,
"Sebastian A. Siewior" <email@example.com>
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 [thread overview]
Message-ID: <X8+OS/K0+9ibIEGz@lx-t490> (raw)
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:
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
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
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
next prev parent reply other threads:[~2020-12-08 14:32 UTC|newest]
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
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:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).