On Tue, 2024-01-23 at 14:27 +0100, Jan Beulich wrote: > On 23.01.2024 13:18, Oleksii wrote: > > On Tue, 2024-01-23 at 11:28 +0100, Jan Beulich wrote: > > > On 23.01.2024 11:15, Oleksii wrote: > > > > On Mon, 2024-01-22 at 17:27 +0100, Jan Beulich wrote: > > > > > On 22.12.2023 16:12, Oleksii Kurochko wrote: > > > > > > +static inline unsigned long __xchg(volatile void *ptr, > > > > > > unsigned > > > > > > long x, int size) > > > > > > +{ > > > > > > + switch (size) { > > > > > > + case 1: > > > > > > + return __cmpxchg_case_1(ptr, (uint32_t)-1, x); > > > > > > + case 2: > > > > > > + return __cmpxchg_case_2(ptr, (uint32_t)-1, x); > > > > > > > > > > How are these going to work? You'll compare against ~0, and > > > > > if > > > > > the > > > > > value > > > > > in memory isn't ~0, memory won't be updated; you will only > > > > > (correctly) > > > > > return the value found in memory. > > > > > > > > > > Or wait - looking at __cmpxchg_case_{1,2}() far further down, > > > > > you > > > > > ignore > > > > > "old" there. Which apparently means they'll work for the use > > > > > here, > > > > > but > > > > > not for the use in __cmpxchg(). > > > > Yes, the trick is that old is ignored and is read in > > > > __emulate_cmpxchg_case1_2() before __cmpxchg_case_4 is called: > > > > do > > > > { > > > > read_val = > > > > read_func(aligned_ptr); > > > > swapped_new = read_val & > > > > ~mask; > > > > swapped_new |= > > > > masked_new; > > > > ret = cmpxchg_func(aligned_ptr, read_val, > > > > swapped_new); > > > > } while ( ret != read_val > > > > ); > > > > read_val it is 'old'. > > > > > > > > But now I am not 100% sure that it is correct for __cmpxchg... > > > > > > It just can't be correct - you can't ignore "old" there. I think > > > you > > > want simple cmpxchg primitives, which xchg then uses in a loop > > > (while > > > cmpxchg uses them plainly). > > But xchg doesn't require 'old' value, so it should be ignored in > > some > > way by cmpxchg. > > Well, no. If you have only cmpxchg, I think your only choice is - as > said - to read the old value and then loop over cmpxchg until that > succeeds. Not really different from other operations which need > emulating using cmpxchg. Then it looks like the main error in __emulate_cmpxchg_case1_2 is that I read the value each time, so read_val = read_func(aligned_ptr); should be before the do {...} while(). Also, it would be better to rename it to old_val or just old.
> > > > > > > +static always_inline unsigned short > > > > > > __cmpxchg_case_2(volatile > > > > > > uint32_t *ptr, > > > > > > + > > > > > > uint32_t > > > > > > old, > > > > > > + > > > > > > uint32_t > > > > > > new) > > > > > > +{ > > > > > > + (void) old; > > > > > > + > > > > > > + if (((unsigned long)ptr & 3) == 3) > > > > > > + { > > > > > > +#ifdef CONFIG_64BIT > > > > > > + return __emulate_cmpxchg_case1_2((uint64_t *)ptr, > > > > > > new, > > > > > > + readq, > > > > > > __cmpxchg_case_8, > > > > > > 0xffffU); > > > > > > > > > > What if ((unsigned long)ptr & 7) == 7 (which is a sub-case of > > > > > what > > > > > the > > > > > if() above checks for? Isn't it more reasonable to require > > > > > aligned > > > > > 16-bit quantities here? Or if mis-aligned addresses are okay, > > > > > you > > > > > could > > > > > as well emulate using __cmpxchg_case_4(). > > > > Yes, it will be more reasonable. I'll use IS_ALIGNED instead. > > > > > > Not sure I get your use of "instead" here correctly. There's more > > > to change here than just the if() condition. > > I meant something like: > > > > if ( IS_ALIGNED(ptr, 16) ) > > __emulate_cmpxchg_case1_2(...); > > else > > assert_failed("ptr isn't aligned\n"); > > Except that you'd better not use assert_failed() directly anywhere, > and the above is easier as > > ASSERT(IS_ALIGNED(ptr, 16)); > __emulate_cmpxchg_case1_2(...); > > anyway (leaving aside that I guess you mean 2, not 16). Yeah, it should be 2. Thanks. ~ Oleksii