Messages in this thread | | | Date | Tue, 18 Aug 2020 19:11:20 +0100 | From | Vincent Donnefort <> | Subject | Re: [PATCH] sched/fair: provide u64 read for 32-bits arch helper |
| |
Hi Peter,
[...]
> > > > > > How about something like: > > > > > > #ifdef CONFIG_64BIT > > > > > > #define DEFINE_U64_U32(name) u64 name > > > #define u64_u32_load(name) name > > > #define u64_u32_store(name, val)name = val > > > > > > #else > > > > > > #define DEFINE_U64_U32(name) \ > > > struct { \ > > > u64 name; \ > > > u64 name##_copy; \ > > > } > > > > > > #define u64_u32_load(name) \ > > > ({ \ > > > u64 val, copy; \ > > > do { \ > > > val = name; \ > > > smb_rmb(); \ > > > copy = name##_copy; \ > > > } while (val != copy); \ > > > > wrong order there; we should first read _copy and then the regular one > > of course. > > > > > val; > > > }) > > > > > > #define u64_u32_store(name, val) \ > > > do { \ > > > typeof(val) __val = (val); \ > > > name = __val; \ > > > smp_wmb(); \ > > > name##_copy = __val; \ > > > } while (0) > > > > > > #endif > > > > The other approach is making it a full type and inline functions I > > suppose. > > I didn't pick this approach originally, as I thought it would be way too > invasive. If the API looks way cleaner, it nonetheless doesn't match > last_update_time usage. The variable is declared in sched_avg while its copy > is in struct cfs_rq. > > Moving the copy in sched_avg would mean: > > * Setting the copy for all struct sched_avg in ___update_load_sum(), instead > of just the cfs_rq.avg in update_cfs_rq_load_avg(). > > * Having the DEFINE_U64_U32 declaration in include/linux/sched.h to cover > struct sched_avg.
Gentle ping
| |