Re: [PATCH 1/2] x86/memsharing: use an atomic add instead of a cmpxchg loop

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Feb 22, 2024 at 5:06 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 22.02.2024 10:05, Roger Pau Monne wrote:
> > The usage of a cmpxchg loop in get_next_handle() is unnecessary, as the same
> > can be achieved with an atomic increment, which is both simpler to read, and
> > avoid any need for a loop.
> >
> > The cmpxchg usage is likely a remnant of 32bit support, which didn't have an
> > instruction to do an atomic 64bit add, and instead a cmpxchg had to be used.
> >
> > Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Signed-of-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> albeit ...
>
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -179,13 +179,7 @@ static void mem_sharing_page_unlock(struct page_info 
> > *pg)
> >
> >  static shr_handle_t get_next_handle(void)
> >  {
> > -    /* Get the next handle get_page style */
> > -    uint64_t x, y = next_handle;
> > -    do {
> > -        x = y;
> > -    }
> > -    while ( (y = cmpxchg(&next_handle, x, x + 1)) != x );
> > -    return x + 1;
> > +    return arch_fetch_and_add(&next_handle, 1) + 1;
> >  }
>
> ... the adding of 1 here is a little odd when taken together with
> next_handle's initializer. Tamas, you've not written that code, but do
> you have any thoughts towards the possible removal of either the
> initializer or the adding here? Plus that variable of course could
> very well do with moving into this function.

I have to say I find the existing logic here hard to parse but by the
looks I don't think we need the + 1 once we switch to
arch_fetch_and_add. Also could go without initializing next_handle to
1. Moving it into the function would not really accomplish anything
other than style AFAICT?

Tamas