Messages in this thread | | | Date | Tue, 28 Jul 2020 20:53:21 +0100 | From | Vincent Donnefort <> | Subject | Re: [PATCH] sched/fair: provide u64 read for 32-bits arch helper |
| |
Hi,
On Tue, Jul 28, 2020 at 02:00:27PM +0200, peterz@infradead.org wrote: > On Tue, Jul 28, 2020 at 01:13:02PM +0200, peterz@infradead.org wrote: > > On Mon, Jul 27, 2020 at 04:23:03PM +0100, Vincent Donnefort wrote: > > > > > For 32-bit architectures, both min_vruntime and last_update_time are using > > > similar access. This patch is simply an attempt to unify their usage by > > > introducing two macros to rely on when accessing those. At the same time, it > > > brings a comment regarding the barriers usage, as per the kernel policy. So > > > overall this is just a clean-up without any functional changes. > > > > Ah, I though there was perhaps the idea to make use of armv7-lpae > > instructions. > > > > Aside of that, I think we need to spend a little time bike-shedding the > > API/naming here: > > > > > +# define u64_32read(val, copy) (val) > > > +# define u64_32read_set_copy(val, copy) do { } while (0) > > > > 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.
-- Vincent
| |