Re: [PATCH] sched/fair: provide u64 read for 32-bits arch helper
Hi Vincent, On 7/27/20 11:59 AM, vincent.donnef...@arm.com wrote: From: Vincent Donnefort Introducing two macro helpers u64_32read() and u64_32read_set_copy() to factorize the u64 vminruntime and last_update_time read on a 32-bits architecture. Those new helpers encapsulate smp_rmb() and smp_wmb() synchronization and therefore, have a small penalty in set_task_rq_fair() and init_cfs_rq(). The choice of using a macro over an inline function is driven by the conditional u64 variable copy declarations. #ifndef CONFIG_64BIT u64 [vminruntime|last_update_time]_copy; #endif Signed-off-by: Vincent Donnefort I've run it on 32-bit ARM big.LITTLE platform (odroid xu3) with the CONFIG_FAIR_GROUP_SCHED and CONFIG_CGROUP_SCHED. I haven't observed any issues while running hackbench or booting the kernel, the performance of affected function set_task_rq_fair() is the same. Function profile results after hackbench test: w/ Vincent's patch + performance cpufreq gov root@odroid:/sys/kernel/debug/tracing# cat trace_stat/function? Function HitTimeAvg s^2 ------ --- set_task_rq_fair 40683753.185 us 0.922 us1.239 us Function HitTimeAvg s^2 ------ --- set_task_rq_fair 44924180.133 us 0.930 us2.318 us Function HitTimeAvg s^2 ------ --- set_task_rq_fair 42083991.386 us 0.948 us13.552 us Function HitTimeAvg s^2 ------ --- set_task_rq_fair 47534432.231 us 0.932 us5.875 us Function HitTimeAvg s^2 ------ --- set_task_rq_fair 79805037.096 us 0.631 us1.690 us Function HitTimeAvg s^2 ------ --- set_task_rq_fair 81435078.515 us 0.623 us2.930 us Function HitTimeAvg s^2 ------ --- set_task_rq_fair 97216477.904 us 0.666 us2.425 us Function HitTimeAvg s^2 ------ --- set_task_rq_fair 77434896.002 us 0.632 us1.188 us ---w/o Vincent patch, performance cpufreq gov root@odroid:/sys/kernel/debug/tracing# cat trace_stat/function? Function HitTimeAvg s^2 ------ --- set_task_rq_fair 65255830.450 us 0.893 us3.213 us Function HitTimeAvg s^2 ------ --- set_task_rq_fair 66466069.444 us 0.913 us9.651 us Function HitTimeAvg s^2 ------ --- set_task_rq_fair 59885646.133 us 0.942 us7.685 us Function HitTimeAvg s^2 ------ --- set_task_rq_fair 58835390.103 us 0.916 us29.283 us Function HitTimeAvg s^2 ------ --- set_task_rq_fair 38442561.186 us 0.666 us0.933 us Function HitTimeAvg s^2 ------ --- set_task_rq_fair 55153491.011 us 0.633 us9.845 us Function HitTimeAvg s^2 ------ --- set_task_rq_fair 69474808.522 us 0.692 us5.822 us Function
Re: [PATCH] sched/fair: provide u64 read for 32-bits arch helper
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); \ val; }) #define u64_u32_store(name, val)\ do {\ typeof(val) __val = (val); \ name = __val; \ smp_wmb(); \ name##_copy = __val;\ } while (0) #endif
Re: [PATCH] sched/fair: provide u64 read for 32-bits arch helper
On Tue, Jul 28, 2020 at 01:13:02PM +0200, pet...@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.
Re: [PATCH] sched/fair: provide u64 read for 32-bits arch helper
Hi, On Tue, Jul 28, 2020 at 02:00:27PM +0200, pet...@infradead.org wrote: > On Tue, Jul 28, 2020 at 01:13:02PM +0200, pet...@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
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
Re: [PATCH] sched/fair: provide u64 read for 32-bits arch helper
* vincent.donnef...@arm.com wrote: > From: Vincent Donnefort > > Introducing two macro helpers u64_32read() and u64_32read_set_copy() to > factorize the u64 vminruntime and last_update_time read on a 32-bits > architecture. Those new helpers encapsulate smp_rmb() and smp_wmb() > synchronization and therefore, have a small penalty in set_task_rq_fair() > and init_cfs_rq(). > > The choice of using a macro over an inline function is driven by the > conditional u64 variable copy declarations. Could you please explain how "conditional u64 variable copy declarations" prevents us to use an inline function for this: > +/* > + * u64_32read() / u64_32read_set_copy() > + * > + * Use the copied u64 value to protect against data race. This is only > + * applicable for 32-bits architectures. > + */ > +#if !defined(CONFIG_64BIT) && defined(CONFIG_SMP) > +# define u64_32read(val, copy) > \ > +({ \ > + u64 _val; \ > + u64 _val_copy; \ > + \ > + do {\ > + _val_copy = copy; \ > + /* \ > + * paired with u64_32read_set_copy, ordering access \ > + * to val and copy. \ > + */ \ > + smp_rmb(); \ > + _val = val; \ > + } while (_val != _val_copy);\ > + \ > + _val; \ > +}) > +# define u64_32read_set_copy(val, copy) > \ > +do { \ > + /* paired with u64_32read, ordering access to val and copy */ \ > + smp_wmb(); \ > + copy = val; \ > +} while (0) > +#else > +# define u64_32read(val, copy) (val) > +# define u64_32read_set_copy(val, copy) do { } while (0) > +#endif > + Thanks, Ingo
Re: [PATCH] sched/fair: provide u64 read for 32-bits arch helper
Hi, On Mon, Jul 27, 2020 at 01:24:54PM +0200, Ingo Molnar wrote: > > * vincent.donnef...@arm.com wrote: > > > From: Vincent Donnefort > > > > Introducing two macro helpers u64_32read() and u64_32read_set_copy() to > > factorize the u64 vminruntime and last_update_time read on a 32-bits > > architecture. Those new helpers encapsulate smp_rmb() and smp_wmb() > > synchronization and therefore, have a small penalty in set_task_rq_fair() > > and init_cfs_rq(). > > > > The choice of using a macro over an inline function is driven by the > > conditional u64 variable copy declarations. > > Could you please explain how "conditional u64 variable copy > declarations" prevents us to use an inline function for this I meant, as the "copy" variable [vminruntime|last_update_time]_copy, is declared only in the !CONFIG_64BIT case, a function call would fail when CONFIG_64BIT is set. u64_32read(cfs_rq->min_vruntime, cfs_rq->min_vruntime_copy); ^ not declared I could rephrase this part to something more understandable ? > > > +/* > > + * u64_32read() / u64_32read_set_copy() > > + * > > + * Use the copied u64 value to protect against data race. This is only > > + * applicable for 32-bits architectures. > > + */ > > +#if !defined(CONFIG_64BIT) && defined(CONFIG_SMP) > > +# define u64_32read(val, copy) > > \ > > +({ \ > > + u64 _val; \ > > + u64 _val_copy; \ > > + \ > > + do {\ > > + _val_copy = copy; \ > > + /* \ > > +* paired with u64_32read_set_copy, ordering access \ > > +* to val and copy. \ > > +*/ \ > > + smp_rmb(); \ > > + _val = val; \ > > + } while (_val != _val_copy);\ > > + \ > > + _val; \ > > +}) > > +# define u64_32read_set_copy(val, copy) > > \ > > +do { > > \ > > + /* paired with u64_32read, ordering access to val and copy */ \ > > + smp_wmb(); \ > > + copy = val; \ > > +} while (0) > > +#else > > +# define u64_32read(val, copy) (val) > > +# define u64_32read_set_copy(val, copy) do { } while (0) > > +#endif > > + > > Thanks, > > Ingo -- Vincent
Re: [PATCH] sched/fair: provide u64 read for 32-bits arch helper
On Mon, Jul 27, 2020 at 11:59:24AM +0100, vincent.donnef...@arm.com wrote: > From: Vincent Donnefort > > Introducing two macro helpers u64_32read() and u64_32read_set_copy() to > factorize the u64 vminruntime and last_update_time read on a 32-bits > architecture. Those new helpers encapsulate smp_rmb() and smp_wmb() > synchronization and therefore, have a small penalty in set_task_rq_fair() > and init_cfs_rq(). > > The choice of using a macro over an inline function is driven by the > conditional u64 variable copy declarations. > > #ifndef CONFIG_64BIT > u64 [vminruntime|last_update_time]_copy; > #endif This lacks a *why*... why did you get up this morning and wrote us this patch.
Re: [PATCH] sched/fair: provide u64 read for 32-bits arch helper
On Mon, Jul 27, 2020 at 02:38:01PM +0200, pet...@infradead.org wrote: > On Mon, Jul 27, 2020 at 11:59:24AM +0100, vincent.donnef...@arm.com wrote: > > From: Vincent Donnefort > > > > Introducing two macro helpers u64_32read() and u64_32read_set_copy() to > > factorize the u64 vminruntime and last_update_time read on a 32-bits > > architecture. Those new helpers encapsulate smp_rmb() and smp_wmb() > > synchronization and therefore, have a small penalty in set_task_rq_fair() > > and init_cfs_rq(). > > > > The choice of using a macro over an inline function is driven by the > > conditional u64 variable copy declarations. > > > > #ifndef CONFIG_64BIT > > u64 [vminruntime|last_update_time]_copy; > > #endif > > This lacks a *why*... why did you get up this morning and wrote us this > patch. > > 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. -- Vincent.