From: Alan Huang <mmpgouride@gmail.com>
To: paulmck@kernel.org
Cc: Akira Yokosawa <akiyks@gmail.com>, perfbook@vger.kernel.org
Subject: Re: Question: Why does the modification of seq in write_seqlock not require WRITE_ONCE?
Date: Wed, 26 Apr 2023 19:19:02 +0800 [thread overview]
Message-ID: <50A530B8-254C-4D88-81ED-096684A3D377@gmail.com> (raw)
In-Reply-To: <2f93807e-5137-4ed2-9267-ab3dab24ac57@paulmck-laptop>
> 2023年4月26日 01:50，Paul E. McKenney <paulmck@kernel.org> 写道：
>
> On Wed, Apr 26, 2023 at 01:06:57AM +0800, Alan Huang wrote:
>> Hi Akira,
>>
>>> 2023年4月25日 23:33，Akira Yokosawa <akiyks@gmail.com> 写道：
>>>
>>> Hi Alan,
>>>
>>> On Tue, 25 Apr 2023 21:52:48 +0800, Alan Huang wrote:
>>>> Hi,
>>>>
>>>> I noticed that the modifications of seq in write_seqlock and write_sequnlock of Listing 9.10 use plain ++ operation.
>>>> But as Chapter 4 (especially 4.3.4.4) says, there will be store tearing since there are concurrent readers.
>>>>
>>>> However, the kernel implementation of sequence lock also uses plain ++ operation.
>>>>
>>>> I’m somewhat confused.
>>>>
>>>> Why does the modification of seq in write_seqlock not require WRITE_ONCE?
>>>
>>> Good question...
>>>
>>> I don't have a straight answer, but the history of include/linux/seqlock.h
>>> says those plain increments predate the introduction of ACCESS_ONCE
>>> (predecessor to READ_ONCE/WRITE_ONCE) to the Linux kernel.
>>>
>>> The code at the time (pre v2.6.0) looked like this:
>>>
>>> static inline void write_seqlock(seqlock_t *sl)
>>> {
>>> spin_lock(&sl->lock);
>>> ++sl->sequence;
>>> smp_wmb();
>>> }
>>>
>>> static inline void write_sequnlock(seqlock_t *sl)
>>> {
>>> smp_wmb();
>>> sl->sequence++;
>>> spin_unlock(&sl->lock);
>>> }
>>>
>>> static inline int write_tryseqlock(seqlock_t *sl)
>>> {
>>> int ret = spin_trylock(&sl->lock);
>>>
>>> if (ret) {
>>> ++sl->sequence;
>>> smp_wmb();
>>> }
>>> return ret;
>>> }
>>>
>>> The purpose of WRITE_ONCE() would be to suppress compiler optimization
>>> of write accesses. In the code above, smp_wmb(), spin_lock(),
>>> spin_unlock(), and spin_trylock() all imply compiler barriers.
>>> So, there is not much room for compilers to optimize the store part
>>> of increments.
>>
>> But unsigned long is at least 32 bits, here is the COMPILER BARRIER section of Documentation/memory-barriers.txt:
>>
>> For example, given an architecture having 16-bit store instructions with 7-bit immediate fields, the compiler
>> might be tempted to use two 16-bit store-immediate instructions to implement the following 32-bit store:
>> p = 0x00010002;
>>
>> The kernel defines sequence as unsigned, which is at least 16 bits, I don’t know if there exists an architecture having 16-bit store instructions with 32-bit unsigned…
>
> There have been CPUs and compilers that would use a pair of 16-bit
> store-immediate instructions to store a 32-bit constant. I don't know
> of any situation where a compiler would use 16-bit loads or stores for
> a 32-bit increment, but there is no law forbidding the compiler from
> doing so.
>
> To answer the higher-level question, once we had the Linux-kernel memory
> model in place, I focused more on explaining it and clearly not enough
> on bringing the perfook code samples into line with it.
>
> So I would welcome a patch making sequence locking use READ_ONCE()
> and WRITE_ONCE() to do the increments. The Linux kernel might not
> be so excited about the corresponding patch because it could result
> in slightly worse code being generated, for example, a++ might use
> an x86 add-to-memory instruction while the READ_ONCE()/WRITE_ONCE()
> counterpart would load, increment, and store. Though I hear rumors that
> some compilers might start better optimizing this.
I see. Let’s keep the sample code consistent with kernel’s.
Thank you both for the explanation!
Thanks,
Alan
>
> Thanx, Paul
>
>> Thanks,
>> Alan
>>
>>>
>>> I think those plain-looking increments are safe as far as current
>>> compilers are concerned.
>>>
>>> Does this explanation help you?
>>>
>>> Paul, please chime in if I'm missing something.
>>>
>>> Thanks, Akira
>>>
>>>>
>>>> Thanks,
>>>> Alan
prev parent reply other threads:[~2023-04-26 11:19 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-25 13:52 Question: Why does the modification of seq in write_seqlock not require WRITE_ONCE? Alan Huang
2023-04-25 15:33 ` Akira Yokosawa
2023-04-25 17:06 ` Alan Huang
2023-04-25 17:50 ` Paul E. McKenney
2023-04-26 11:19 ` Alan Huang [this message]
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=50A530B8-254C-4D88-81ED-096684A3D377@gmail.com \
--to=mmpgouride@gmail.com \
--cc=akiyks@gmail.com \
--cc=paulmck@kernel.org \
--cc=perfbook@vger.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
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 read-only IMAP folder(s) and NNTP newsgroup(s).