From: "Oleksii K." <oleksii.kurochko@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>,
"Ross Lagerwall" <ross.lagerwall@citrix.com>,
"Stefano Stabellini" <sstabellini@kernel.org>,
"Julien Grall" <julien@xen.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>,
"Shawn Anastasio" <sanastasio@raptorengineering.com>,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v9 02/15] xen: introduce generic non-atomic test_*bit()
Date: Thu, 16 May 2024 12:34:14 +0200 [thread overview]
Message-ID: <c8bd47e8f8558d88b4d5a4a09ea10728006fd4d3.camel@gmail.com> (raw)
In-Reply-To: <77c6e497-631d-4f92-bd38-8ab7dba4841d@suse.com>
On Thu, 2024-05-16 at 09:04 +0200, Jan Beulich wrote:
> On 15.05.2024 19:03, Oleksii K. wrote:
> > On Wed, 2024-05-15 at 17:41 +0200, Jan Beulich wrote:
> > > On 15.05.2024 17:29, Oleksii K. wrote:
> > > > On Wed, 2024-05-15 at 10:52 +0200, Jan Beulich wrote:
> > > > > On 06.05.2024 12:15, Oleksii Kurochko wrote:
> > > > > > The following generic functions were introduced:
> > > > > > * test_bit
> > > > > > * generic__test_and_set_bit
> > > > > > * generic__test_and_clear_bit
> > > > > > * generic__test_and_change_bit
> > > > > >
> > > > > > Also, the patch introduces the following generics which are
> > > > > > used by the functions mentioned above:
> > > > > > * BITOP_BITS_PER_WORD
> > > > > > * BITOP_MASK
> > > > > > * BITOP_WORD
> > > > > > * BITOP_TYPE
> > > > > >
> > > > > > These functions and macros can be useful for architectures
> > > > > > that don't have corresponding arch-specific instructions.
> > > > >
> > > > > Logically this paragraph may better move ahead of the BITOP_*
> > > > > one.
> > > > >
> > > > > > Because of that x86 has the following check in the macros
> > > > > > test_bit(),
> > > > > > __test_and_set_bit(), __test_and_clear_bit(),
> > > > > > __test_and_change_bit():
> > > > > > if ( bitop_bad_size(addr) ) __bitop_bad_size();
> > > > > > It was necessary to make bitop bad size check generic too,
> > > > > > so
> > > > > > arch_check_bitop_size() was introduced.
> > > > >
> > > > > Not anymore, with the most recent adjustments? There's
> > > > > nothing
> > > > > arch-
> > > > > specific anymore in the checking.
> > > > >
> > > > > > @@ -183,7 +180,7 @@ static inline int test_and_set_bit(int
> > > > > > nr,
> > > > > > volatile void *addr)
> > > > > > * If two examples of this operation race, one can appear
> > > > > > to
> > > > > > succeed
> > > > > > * but actually fail. You must protect multiple accesses
> > > > > > with
> > > > > > a
> > > > > > lock.
> > > > > > */
> > > > > > -static inline int __test_and_set_bit(int nr, void *addr)
> > > > > > +static inline int arch__test_and_set_bit(int nr, volatile
> > > > > > void
> > > > > > *addr)
> > > > >
> > > > > I think I raised this point before: Why arch__ here, ...
> > > > >
> > > > > > @@ -232,7 +226,7 @@ static inline int
> > > > > > test_and_clear_bit(int
> > > > > > nr,
> > > > > > volatile void *addr)
> > > > > > * If two examples of this operation race, one can appear
> > > > > > to
> > > > > > succeed
> > > > > > * but actually fail. You must protect multiple accesses
> > > > > > with
> > > > > > a
> > > > > > lock.
> > > > > > */
> > > > > > -static inline int __test_and_clear_bit(int nr, void *addr)
> > > > > > +static inline int arch__test_and_clear_bit(int nr,
> > > > > > volatile
> > > > > > void
> > > > > > *addr)
> > > > >
> > > > > ... here, ...
> > > > >
> > > > > > @@ -243,13 +237,10 @@ static inline int
> > > > > > __test_and_clear_bit(int
> > > > > > nr, void *addr)
> > > > > >
> > > > > > return oldbit;
> > > > > > }
> > > > > > -#define __test_and_clear_bit(nr, addr) ({ \
> > > > > > - if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> > > > > > - __test_and_clear_bit(nr, addr); \
> > > > > > -})
> > > > > > +#define arch__test_and_clear_bit arch__test_and_clear_bit
> > > > > >
> > > > > > /* WARNING: non atomic and it can be reordered! */
> > > > > > -static inline int __test_and_change_bit(int nr, void
> > > > > > *addr)
> > > > > > +static inline int arch__test_and_change_bit(int nr,
> > > > > > volatile
> > > > > > void
> > > > > > *addr)
> > > > >
> > > > > ... and here, while ...
> > > > >
> > > > > > @@ -307,8 +295,7 @@ static inline int variable_test_bit(int
> > > > > > nr,
> > > > > > const volatile void *addr)
> > > > > > return oldbit;
> > > > > > }
> > > > > >
> > > > > > -#define test_bit(nr, addr) ({ \
> > > > > > - if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> > > > > > +#define arch_test_bit(nr, addr) ({ \
> > > > >
> > > > > ... just arch_ here? I don't like the double underscore
> > > > > infixes
> > > > > very
> > > > > much, but I'm okay with them as long as they're applied
> > > > > consistently.
> > > >
> > > > Common code and x86 use __test_and_clear_bit(), and this patch
> > > > provides
> > > > a generic version of __test_and_clear_bit(). To emphasize that
> > > > generic__test_and_clear_bit() is a common implementation of
> > > > __test_and_clear_bit(), the double underscore was retained.
> > > > Also,
> > > > test_and_clear_bit() exists and if one day it will be needed to
> > > > provide
> > > > a generic version of it, then it will be needed to have
> > > > generic__test_and_clear_bit() and generic_test_and_clear_bit()
> > > >
> > > > A similar logic was chosen for test_bit.
> > >
> > > Right, but in all of your reply arch_ doesn't appear at all.
> > I am a little confused here. According to my logic, should it be
> > arch___test_and_set_bit() and generic___test_and_set_bit()?
>
> Why 3 underscores in a row? I'm clearly not following.
>
> > If you are asking why there is no generic implementation for
> > test_and_clear_bit() without the double underscores, the reason is
> > that
> > Arm, PPC, and x86 don't use generic code and rely on specific
> > instructions for this operation. Therefore, I didn't see much sense
> > in
> > providing a generic version of test_and_clear_bit(), at least, for
> > now.
>
> No, there was no question in that direction. And hence ...
>
> > > Yet the
> > > question was: Why then not arch__test_bit(), to match the other
> > > arch
> > > helpers?
> > Because no one uses __test_bit(). Everywhere is used test_bit().
>
> ... this seems unrelated (constrained by my earlier lack of following
> you).
>
> (Later) Wait, maybe I've finally figured it: You use
> arch__test_and_*()
> because those underlie __test_and_*(), but arch_test_bit() because
> there's
> solely test_bit() (same for the generic_* naming).
Yes, that what I meant.
> I guess I can accept
> that then, despite the slight anomaly you point out, resulting in the
> question towards 3 underscores in a row. To clarify, my thinking was
> more
> towards there not possibly being generic forms of test_and_*() (i.e.
> no
> possible set of arch_test_and_*() or generic_test_and_*()), thus
> using
> double inner underscores in arch__test_*() and generic__test_*() to
> signify that those are purely internal functions, which aren't
> supposed to
> be called directly.
I understand your point regarding functions that start with "__".
For example, __test_and_clear_bit() is used not only internally (in
terms of architecture code) but also in common code, so it is not
strictly internal. I may have misunderstood what "internal function"
means in this context.
I thought that, at least for bit operations, "__bit_operation" means
that the bit operation is non-atomic and can be reordered, which
implies that it's not a good idea to use it in common code without
additional steps.
Anyway, I am not sure I understand which approach I should use in this
patch. You mentioned that possibly test_and_() can't have a generic
form, meaning it won't be a set of arch_test_and_() functions.
So, can I rename arch__test_() and generic__test_() to arch_test_() and
generic_test_(), respectively, and use the renamed functions in
_test_and*() in xen/bitops.h? Is my understanding correct?
If my understanding is correct, I am happy to apply mentioned changes
in the next patch version.
~ Oleksii
>
> Jan
next prev parent reply other threads:[~2024-05-16 10:34 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-06 10:15 [PATCH v9 00/15] Enable build of full Xen for RISC-V Oleksii Kurochko
2024-05-06 10:15 ` [PATCH v9 01/15] xen/riscv: disable unnecessary configs Oleksii Kurochko
2024-05-06 10:15 ` [PATCH v9 02/15] xen: introduce generic non-atomic test_*bit() Oleksii Kurochko
2024-05-15 8:52 ` Jan Beulich
2024-05-15 15:29 ` Oleksii K.
2024-05-15 15:41 ` Jan Beulich
2024-05-15 17:03 ` Oleksii K.
2024-05-16 7:04 ` Jan Beulich
2024-05-16 10:34 ` Oleksii K. [this message]
2024-05-16 10:49 ` Jan Beulich
2024-05-17 8:42 ` Oleksii K.
2024-05-06 10:15 ` [PATCH v9 03/15] xen/bitops: implement fls{l}() in common logic Oleksii Kurochko
2024-05-15 9:09 ` Jan Beulich
2024-05-15 13:55 ` Oleksii K.
2024-05-15 14:07 ` Jan Beulich
2024-05-15 15:31 ` Oleksii K.
2024-05-17 9:06 ` Oleksii K.
2024-05-17 9:52 ` Jan Beulich
2024-05-06 10:15 ` [PATCH v9 04/15] xen/bitops: put __ffs() into linux compatible header Oleksii Kurochko
2024-05-15 14:34 ` Michal Orzel
2024-05-15 15:08 ` Rahul Singh
2024-05-06 10:15 ` [PATCH v9 05/15] xen/riscv: introduce bitops.h Oleksii Kurochko
2024-05-15 9:29 ` Jan Beulich
2024-05-06 10:15 ` [PATCH v9 06/15] xen/riscv: introduce cmpxchg.h Oleksii Kurochko
2024-05-15 9:38 ` Jan Beulich
2024-05-06 10:15 ` [PATCH v9 07/15] xen/riscv: introduce atomic.h Oleksii Kurochko
2024-05-15 9:49 ` Jan Beulich
2024-05-15 13:59 ` Oleksii K.
2024-05-06 10:15 ` [PATCH v9 08/15] xen/riscv: introduce monitor.h Oleksii Kurochko
2024-05-06 10:15 ` [PATCH v9 09/15] xen/riscv: add definition of __read_mostly Oleksii Kurochko
2024-05-06 10:15 ` [PATCH v9 10/15] xen/riscv: add required things to current.h Oleksii Kurochko
2024-05-06 10:15 ` [PATCH v9 11/15] xen/riscv: add minimal stuff to mm.h to build full Xen Oleksii Kurochko
2024-05-06 10:15 ` [PATCH v9 12/15] xen/riscv: introduce vm_event_*() functions Oleksii Kurochko
2024-05-06 10:15 ` [PATCH v9 13/15] xen/riscv: add minimal amount of stubs to build full Xen Oleksii Kurochko
2024-05-06 10:15 ` [PATCH v9 14/15] xen/riscv: enable full Xen build Oleksii Kurochko
2024-05-06 10:15 ` [PATCH v9 15/15] 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=c8bd47e8f8558d88b4d5a4a09ea10728006fd4d3.camel@gmail.com \
--to=oleksii.kurochko@gmail.com \
--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=julien@xen.org \
--cc=michal.orzel@amd.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).