Re: [PATCH 1/2] sched/rt: Use 'back' only if CONFIG_RT_GROUP_SCHED is enabled

[Yajun Deng]

June 4, 2024 at 9:22 PM, "Steven Rostedt" <rostedt@xxxxxxxxxxx> wrote:



> 
> On Mon, 3 Jun 2024 20:50:01 +0800
> 
> Yajun Deng <yajun.deng@xxxxxxxxx> wrote:
> 
> > 
> > The 'back' member in struct sched_rt_entity only related to
> > 
> >  CONFIG_RT_GROUP_SCHED, so there is no need to init it in dequeue_rt_stack.
> > 
> >  
> > 
> >  Init the 'back' member in init_tg_rt_entry and use it only if
> > 
> >  CONFIG_RT_GROUP_SCHED is enabled.
> > 
> >  
> > 
> >  Signed-off-by: Yajun Deng <yajun.deng@xxxxxxxxx>
> > 
> >  ---
> > 
> >  kernel/sched/rt.c | 15 ++++++++++-----
> > 
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> > 
> >  
> > 
> >  diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> > 
> >  index 63e49c8ffc4d..44bb9087cfe5 100644
> > 
> >  --- a/kernel/sched/rt.c
> > 
> >  +++ b/kernel/sched/rt.c
> > 
> >  @@ -232,8 +232,10 @@ void init_tg_rt_entry(struct task_group *tg, struct rt_rq *rt_rq,
> > 
> >  
> > 
> >  if (!parent)
> > 
> >  rt_se->rt_rq = &rq->rt;
> > 
> >  - else
> > 
> >  + else {
> > 
> >  rt_se->rt_rq = parent->my_q;
> > 
> >  + parent->back = rt_se;
> > 
> >  + }
> > 
> 
> If one block of an if/else requires brackets, then both do.
> 

Got it.
> > 
> > rt_se->my_q = rt_rq;
> > 
> >  rt_se->parent = parent;
> > 
> >  @@ -1428,16 +1430,19 @@ static void dequeue_rt_stack(struct sched_rt_entity *rt_se, unsigned int flags)
> > 
> >  struct sched_rt_entity *back = NULL;
> > 
> >  unsigned int rt_nr_running;
> > 
> >  
> > 
> >  - for_each_sched_rt_entity(rt_se) {
> > 
> >  - rt_se->back = back;
> > 
> >  + for_each_sched_rt_entity(rt_se)
> > 
> >  back = rt_se;
> > 
> >  - }
> > 
> >  
> > 
> >  rt_nr_running = rt_rq_of_se(back)->rt_nr_running;
> > 
> >  
> > 
> >  - for (rt_se = back; rt_se; rt_se = rt_se->back) {
> > 
> >  + for (rt_se = back; rt_se; ) {
> > 
> >  if (on_rt_rq(rt_se))
> > 
> >  __dequeue_rt_entity(rt_se, flags);
> > 
> >  +#ifdef CONFIG_RT_GROUP_SCHED
> > 
> >  + rt_se = rt_se->back;
> > 
> >  +#else
> > 
> >  + rt_se = NULL;
> > 
> >  +#endif
> > 
> 
> Perhaps make a helper function to get rid of the #ifdef in the code here.
> 
>  for (rt_se = back, rt_se; rt_se = rt_se_back(rt_se)) {
> 
> Where rt_se_back() above can be defined in the #ifdef blocks for
> 
> CONFIG_RT_GROUP_SCHED as either:
> 
> static struct sched_rt_entity *rt_se_back(struct sched_rt_entity *se)
> 
> {
> 
>  return se->back;
> 
> }
> 
> Or
> 
> static inline struct sched_rt_entity *rt_se_back(struct sched_rt_entity *se)
> 
> {
> 
>  return NULL;
> 
> }
> 

Okay, thanks.

> -- Steve
> 
> > 
> > }
> > 
> >  
> > 
> >  dequeue_top_rt_rq(rt_rq_of_se(back), rt_nr_running);
> >
>