Messages in this thread | | | Date | Tue, 27 Jun 2023 16:05:47 +0800 | From | Chen Yu <> | Subject | Re: [PATCH v2] sched/task_group: Re-layout structure to reduce false sharing |
| |
On 2023-06-26 at 20:53:35 +0800, Aaron Lu wrote: > On Mon, Jun 26, 2023 at 03:52:17PM +0800, Chen Yu wrote: > > 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: > > > > 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) { > == :-) > Yes. > > + p->rt.rt_rq = tg->rt_rq[cpu]; > > + p->rt.parent = tg->rt_se[cpu]; > > + } > > #endif > > } > > If a task starts life as a SCHED_NORMAL one and then after some time > it's changed to a RT one, then during its next ttwu(), if it didn't > migrate, then set_task_rq() will not be called and p->rt.rt_rq will > keep as NULL which will cause problem when this task gets enqueued as > a rt one. > Yeah, this case should be covered to avoid the NULL pointer issue. It seems that set_task_rq() is widely used to assign task's rt field irrelevant of what context the task is in to avoid extra check. > The follow diff seems to cure this issue: > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index c7db597e8175..8c57148e668c 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -7801,6 +7801,20 @@ static int __sched_setscheduler(struct task_struct *p, > } > __setscheduler_uclamp(p, attr); > > +#ifdef CONFIG_RT_GROUP_SCHED > + /* > + * Make sure when this task becomes a rt one, > + * its rt fields have valid value. > + */ > + if (rt_prio(newprio)) { > + struct task_group *tg = task_group(p); > + int cpu = cpu_of(rq); > + > + p->rt.rt_rq = tg->rt_rq[cpu]; > + p->rt.parent = tg->rt_se[cpu]; > + } > +#endif > + > if (queued) { > /* > * We enqueue to tail when the priority of a task is > > But I'm not sure if it's worth the trouble. > Yeah, cache alignment change seems to be simpler.
thanks, Chenyu > Thanks, > Aaron
| |