From: Julien Grall <julien@xen.org>
To: "Oleksii K." <oleksii.kurochko@gmail.com>,
xen-devel@lists.xenproject.org
Cc: "Roger Pau Monné" <roger.pau@citrix.com>,
"Ross Lagerwall" <ross.lagerwall@citrix.com>,
"Stefano Stabellini" <sstabellini@kernel.org>,
"Bertrand Marquis" <bertrand.marquis@arm.com>,
"Michal Orzel" <michal.orzel@amd.com>,
"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
"Andrew Cooper" <andrew.cooper3@citrix.com>,
"George Dunlap" <george.dunlap@citrix.com>,
"Jan Beulich" <jbeulich@suse.com>,
"Shawn Anastasio" <sanastasio@raptorengineering.com>
Subject: Re: [PATCH v10 02/14] xen: introduce generic non-atomic test_*bit()
Date: Fri, 24 May 2024 10:17:21 +0100 [thread overview]
Message-ID: <555a80ba-3981-4b3e-877d-77ff172ac186@xen.org> (raw)
In-Reply-To: <82b8828112ffb05170472310d7d510ceb4edc555.camel@gmail.com>
Hi Oleksii,
On 24/05/2024 09:58, Oleksii K. wrote:
> On Thu, 2024-05-23 at 15:33 +0100, Julien Grall wrote:
>>>>> #include <asm/bitops.h>
>>>>>
>>>>> +/**
>>>>> + * generic__test_and_set_bit - Set a bit and return its old
>>>>> value
>>>>> + * @nr: Bit to set
>>>>> + * @addr: Address to count from
>>>>> + *
>>>>> + * This operation is non-atomic and can be reordered.
>>>>> + * If two examples of this operation race, one can appear to
>>>>> succeed
>>>>> + * but actually fail. You must protect multiple accesses with
>>>>> a
>>>>> lock.
>>>>> + */
>>>>
>>>> Sorry for only mentioning this on v10. I think this comment
>>>> should be
>>>> duplicated (or moved to) on top of test_bit() because this is
>>>> what
>>>> everyone will use. This will avoid the developper to follow the
>>>> function
>>>> calls and only notice the x86 version which says "This function
>>>> is
>>>> atomic and may not be reordered." and would be wrong for all the
>>>> other arch.
>>> It makes sense to add this comment on top of test_bit(), but I am
>>> curious if it is needed to mention that for x86 arch_test_bit() "is
>>> atomic and may not be reordered":
>>
>> I would say no because any developper modifying common code can't
>> relying it.
> But won't then be confusion that if not generic implementation of
> test_bit() is chosen then test_bit() can be " atomic and cannot be
> reordered " ( as it is in case of x86 )?
I don't understand what confusion could arise. A developper cannot rely
on the x86 behavior in common code. They have to write code that works
for every arch.
The first thing a developer will do is to check test_bit(). The comment
on top will say they can't relying on ordering. And that what most of
the developper needs to rely on.
If someone wants to write more optimized code for x86, they are free to
look at the implementation. But I don't think this should be detailed on
top of the common wrapper (the x86 implementation would still be
compatible with the comment).
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2024-05-24 9:17 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-17 13:54 [PATCH v10 00/14] Enable build of full Xen for RISC-V Oleksii Kurochko
2024-05-17 13:54 ` [PATCH v10 01/14] xen/riscv: disable unnecessary configs Oleksii Kurochko
2024-05-17 13:54 ` [PATCH v10 02/14] xen: introduce generic non-atomic test_*bit() Oleksii Kurochko
2024-05-21 11:10 ` Jan Beulich
2024-05-23 13:00 ` Julien Grall
2024-05-23 14:11 ` Oleksii K.
2024-05-23 14:33 ` Julien Grall
2024-05-23 16:40 ` Oleksii K.
2024-05-24 6:48 ` Jan Beulich
2024-05-24 7:25 ` Oleksii K.
2024-05-24 7:35 ` Jan Beulich
2024-05-24 8:06 ` Oleksii K.
2024-05-24 8:58 ` Oleksii K.
2024-05-24 9:17 ` Julien Grall [this message]
2024-05-17 13:54 ` [PATCH v10 03/14] xen/bitops: implement fls{l}() in common logic Oleksii Kurochko
2024-05-21 11:18 ` Jan Beulich
2024-05-22 7:37 ` Oleksii K.
2024-05-22 8:15 ` Jan Beulich
2024-05-23 13:21 ` Julien Grall
2024-05-17 13:54 ` [PATCH v10 04/14] xen/riscv: introduce bitops.h Oleksii Kurochko
2024-05-17 13:54 ` [PATCH v10 05/14] xen/riscv: introduce cmpxchg.h Oleksii Kurochko
2024-05-17 13:54 ` [PATCH v10 06/14] xen/riscv: introduce atomic.h Oleksii Kurochko
2024-05-17 13:54 ` [PATCH v10 07/14] xen/riscv: introduce monitor.h Oleksii Kurochko
2024-05-18 1:11 ` Tamas K Lengyel
2024-05-17 13:54 ` [PATCH v10 08/14] xen/riscv: add definition of __read_mostly Oleksii Kurochko
2024-05-17 13:54 ` [PATCH v10 09/14] xen/riscv: add required things to current.h Oleksii Kurochko
2024-05-17 13:54 ` [PATCH v10 10/14] xen/riscv: add minimal stuff to mm.h to build full Xen Oleksii Kurochko
2024-05-17 13:55 ` [PATCH v10 11/14] xen/riscv: introduce vm_event_*() functions Oleksii Kurochko
2024-05-18 1:12 ` Tamas K Lengyel
2024-05-17 13:55 ` [PATCH v10 12/14] xen/riscv: add minimal amount of stubs to build full Xen Oleksii Kurochko
2024-05-17 13:55 ` [PATCH v10 13/14] xen/riscv: enable full Xen build Oleksii Kurochko
2024-05-17 13:55 ` [PATCH v10 14/14] xen/README: add compiler and binutils versions for RISC-V64 Oleksii Kurochko
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=555a80ba-3981-4b3e-877d-77ff172ac186@xen.org \
--to=julien@xen.org \
--cc=Volodymyr_Babchuk@epam.com \
--cc=andrew.cooper3@citrix.com \
--cc=bertrand.marquis@arm.com \
--cc=george.dunlap@citrix.com \
--cc=jbeulich@suse.com \
--cc=michal.orzel@amd.com \
--cc=oleksii.kurochko@gmail.com \
--cc=roger.pau@citrix.com \
--cc=ross.lagerwall@citrix.com \
--cc=sanastasio@raptorengineering.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.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).