Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()

[Julien Grall <julien@xxxxxxx>]

Hi Oleksii,

On 28/05/2024 09:37, Oleksii K. wrote:
> On Tue, 2024-05-28 at 08:20 +0200, Jan Beulich wrote:
> > On 24.05.2024 13:08, 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
> > >
> > > These functions and macros can be useful for architectures
> > > that don't have corresponding arch-specific instructions.
> > >
> > > 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
> > >
> > > BITS_PER_BYTE was moved to xen/bitops.h as BITS_PER_BYTE is the
> > > same
> > > across architectures.
> > >
> > > The following approach was chosen for generic*() and arch*() bit
> > > operation functions:
> > > If the bit operation function that is going to be generic starts
> > > with the prefix "__", then the corresponding generic/arch function
> > > will also contain the "__" prefix. For example:
> > >   * test_bit() will be defined using arch_test_bit() and
> > >     generic_test_bit().
> > >   * __test_and_set_bit() will be defined using
> > >     arch__test_and_set_bit() and generic__test_and_set_bit().
> > >
> > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > > ---
> > >   Reviewed-by: Jan Beulich jbeulich@xxxxxxxx? Jan gave his R-by for
> > > the previous
> > >   version of the patch, but some changes were done, so I wasn't sure
> > > if I could
> > >   use the R-by in this patch version.
> > That really depends on the nature of the changes. Seeing the pretty
> > long list below, I think it was appropriate to drop the R-b.
> >
> > > ---
> > > Changes in V11:
> > >   - fix identation in generic_test_bit() function.
> > >   - move definition of BITS_PER_BYTE from <arch>/bitops.h to
> > > xen/bitops.h
> > I realize you were asked to do this, but I'm not overly happy with
> > it.
> > BITS_PER_BYTE is far more similar to BITS_PER_LONG than to
> > BITOP_BITS_PER_WORD. Plus in any event that change is entirely
> > unrelated
> > here.
> So where then it should be introduced? x86 introduces that in config.h,
> Arm in asm/bitops.h.
I would be fine if it is defined in config.h for Arm.

> I am okay to revert this change and make a separate patch.
[...]


> > Also did Julien(?) really ask that these comments be reproduced on
> > both the functions supposed to be used throughout the codebase _and_
> > the generic_*() ones (being merely internal helpers/fallbacks)?
> At least, I understood that in this way.
I suggested to duplicate. But I also proposed an alternative to move the 
comment:
"I think this comment should be duplicated (or moved to) on top of"

I don't have a strong preference which one is used.

> > > +/**
> > > + * generic_test_bit - Determine whether a bit is set
> > > + * @nr: bit number to test
> > > + * @addr: Address to start counting 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.
> > > + */
> > You got carried away updating comments - there's no raciness for
> > simple test_bit(). As is also expressed by its name not having those
> > double underscores that the others have.
> Then it is true for every function in this header. Based on the naming
> the conclusion can be done if it is atomic/npn-atomic and can/can't be
> reordered. 
So let me start with that my only request is to keep the existing 
comments as you move it. It looks like there were none of test_bit() before.
> So the comments aren't seem very useful.

The comments *are* useful. We had several instances where non-Arm folks 
thought all the operations were atomic on Arm as well. And the usual 
suggestion is to add barriers in the Arm version which is a no-go.
Cheers,

--
Julien Grall