[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/2] x86/memsharing: use an atomic add instead of a cmpxchg loop
On 22.02.2024 19:03, Tamas K Lengyel wrote: > 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? Well, limiting scope of things can be viewed as purely style, but I think it's more than that: It makes intentions more clear and reduces the chance of abuse (deliberate or unintentional). Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |