Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Mon, 26 Jun 2023 15:52:17 +0800 | From | Chen Yu <> | Subject | Re: [PATCH v2] sched/task_group: Re-layout structure to reduce false sharing |
| |
On 2023-06-26 at 13:47:56 +0800, Aaron Lu wrote: > On Wed, Jun 21, 2023 at 04:14:25PM +0800, Deng Pan wrote: > > When running UnixBench/Pipe-based Context Switching case, we observed > > high false sharing for accessing ‘load_avg’ against rt_se and rt_rq > > when config CONFIG_RT_GROUP_SCHED is turned on. > > > > Pipe-based Context Switching case is a typical sleep/wakeup scenario, > > in which load_avg is frequenly loaded and stored, at the meantime, rt_se > > and rt_rq are frequently loaded. Unfortunately, they are in the same > > cacheline. > > > > This change re-layouts the structure: > > 1. Move rt_se and rt_rq to a 2nd cacheline. > > 2. Keep ‘parent’ field in the 2nd cacheline since it's also accessed > > very often when cgroups are nested, thanks Tim Chen for providing the > > insight. > > > > Tested on Intel Icelake 2 sockets 80C/160T platform, based on v6.4-rc5. > > > > With this change, Pipe-based Context Switching 160 parallel score is > > improved ~9.6%, perf record tool reports rt_se and rt_rq access cycles > > are reduced from ~14.5% to ~0.3%, perf c2c tool shows the false-sharing > > is resolved as expected: > > I also give it a run on an Icelake and saw similar things when > CONFIG_RT_GROUP_SCHED is on. > > For hackbench/pipe/thread, set_task_cpu() dropped from 1.67% to 0.51% > according to perf cycle; for netperf/nr_client=nr_cpu/UDP_RR, > set_task_cpu() dropped from 5.06% to 1.08%. > > The patch looks good to me, just a nit below. > I also saw overall throughput improvements of netperf on Sapphire Rapids with CONFIG_RT_GROUP_SCHED set, as this platform suffers from C2C so this patch helps a lot.
netperf ======= case load baseline(std%) compare%( std%) TCP_RR 56-threads 1.00 ( 1.61) +2.20 ( 1.39) TCP_RR 112-threads 1.00 ( 2.71) -0.75 ( 2.29) TCP_RR 168-threads 1.00 ( 4.39) -14.26 ( 4.99) TCP_RR 224-threads 1.00 ( 4.21) -5.52 ( 5.07) TCP_RR 280-threads 1.00 ( 1.89) +246.41 ( 61.31) TCP_RR 336-threads 1.00 ( 53.49) +164.89 ( 21.45) TCP_RR 392-threads 1.00 ( 42.46) +162.16 ( 31.33) TCP_RR 448-threads 1.00 ( 44.61) +113.64 ( 41.74) UDP_RR 56-threads 1.00 ( 3.63) -1.27 ( 3.73) UDP_RR 112-threads 1.00 ( 7.83) -4.16 ( 16.57) UDP_RR 168-threads 1.00 ( 18.08) -16.54 ( 17.27) UDP_RR 224-threads 1.00 ( 12.60) -5.77 ( 12.79) UDP_RR 280-threads 1.00 ( 9.37) -0.57 ( 15.75) UDP_RR 336-threads 1.00 ( 14.87) +200.81 ( 34.90) UDP_RR 392-threads 1.00 ( 38.85) -10.15 ( 46.04) UDP_RR 448-threads 1.00 ( 35.06) -8.93 ( 55.56)
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h > > index ec7b3e0a2b20..4fbd4b3a4bdd 100644 > > --- a/kernel/sched/sched.h > > +++ b/kernel/sched/sched.h > > @@ -389,6 +389,19 @@ struct task_group { > > #endif > > #endif > > > > + struct rcu_head rcu; > > + struct list_head list; > > + > > + struct list_head siblings; > > + struct list_head children; > > + > > + /* > > + * To reduce false sharing, current layout is optimized to make > > + * sure load_avg is in a different cacheline from parent, rt_se > > + * and rt_rq. > > + */ > > + struct task_group *parent; > > + > > I wonder if we can simply do: > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h > index ec7b3e0a2b20..31b73e8d9568 100644 > --- a/kernel/sched/sched.h > +++ b/kernel/sched/sched.h > @@ -385,7 +385,9 @@ struct task_group { > * it in its own cacheline separated from the fields above which > * will also be accessed at each tick. > */ > - atomic_long_t load_avg ____cacheline_aligned; > + struct { > + atomic_long_t load_avg; > + } ____cacheline_aligned_in_smp; > #endif > #endif > > This way it can make sure there is no false sharing with load_avg no > matter how the layout of this structure changes in the future. > > Your patch has the advantage of not adding any more padding, thus saves > some space; the example code above has the advantage of no need to worry > about future changes that might break the expected alignment, but it > does make the structure size a little larger(704 -> 768). > Looks reasonable that we don't need to adjust the layout in the future. Besides the cache line alignment, if the task is not a rt one, why do we have to touch that, I wonder if the following change can avoid that:
thanks, Chenyu diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index ec7b3e0a2b20..067f1310bad2 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -1958,8 +1958,10 @@ static inline void set_task_rq(struct task_struct *p, unsigned int cpu) #endif #ifdef CONFIG_RT_GROUP_SCHED - p->rt.rt_rq = tg->rt_rq[cpu]; - p->rt.parent = tg->rt_se[cpu]; + if (p->sched_class = &rt_sched_class) { + p->rt.rt_rq = tg->rt_rq[cpu]; + p->rt.parent = tg->rt_se[cpu]; + } #endif } -- 2.25.1 > Thanks, > Aaron > > > #ifdef CONFIG_RT_GROUP_SCHED > > struct sched_rt_entity **rt_se; > > struct rt_rq **rt_rq; > > @@ -396,13 +409,6 @@ struct task_group { > > struct rt_bandwidth rt_bandwidth; > > #endif > > > > - struct rcu_head rcu; > > - struct list_head list; > > - > > - struct task_group *parent; > > - struct list_head siblings; > > - struct list_head children; > > - > > #ifdef CONFIG_SCHED_AUTOGROUP > > struct autogroup *autogroup; > > #endif > > -- > > 2.39.3 > >
| ![\](/images/icornerr.gif) |