Messages in this thread | | | Date | Mon, 17 Jan 2022 19:42:04 +0000 | From | Vincent Donnefort <> | Subject | Re: [PATCH v2 1/7] sched/fair: Provide u64 read for 32-bits arch helper |
| |
[...]
> > +/* > > + * u64_u32_load/u64_u32_store > > + * > > + * Use a copy of a u64 value to protect against data race. This is only > > + * applicable for 32-bits architectures. > > + */ > > +#ifdef CONFIG_64BIT > > +# define u64_u32_load_copy(var, copy) var > > +# define u64_u32_store_copy(var, copy, val) (var = val) > > +#else > > +# define u64_u32_load_copy(var, copy) \ > > +({ \ > > + u64 __val, __val_copy; \ > > + do { \ > > + __val_copy = copy; \ > > + /* \ > > + * paired with u64_u32_store, ordering access \ > > + * to var and copy. \ > > + */ \ > > + smp_rmb(); \ > > + __val = var; \ > > + } while (__val != __val_copy); \ > > + __val; \ > > +}) > > +# define u64_u32_store_copy(var, copy, val) \ > > +do { \ > > + typeof(val) __val = (val); \ > > + var = __val; \ > > + /* \ > > + * paired with u64_u32_load, ordering access to var and \ > > + * copy. \ > > + */ \ > > + smp_wmb(); \ > > + copy = __val; \ > > +} while (0) > > Code stay there some time from me. Just from my crude review; > The above macro need a variable to load @var temporarily for > later store; that means the @copy value is from @var not @val. > > # define u64_u32_store_copy(var, copy, val) \ > do { \ > typeof(val) __val = (val), __var = (var); \ > var = __val; \ > /* \ > * paired with u64_u32_load, ordering access to var and \ > * copy. \ > */ \ > smp_wmb(); \ > copy = __var; \ > } while (0)
Hi Tao,
__var would then contain the previous value of @var, wouldn't it? We need both @var and @copy to be equal to @val.
> > > > Thanks, > Tao
| |