[PATCH 0/2] sched/rt: Move sched_rt_entity::back to CONFIG_RT_GROUP_SCHED

[PATCH 0/2] sched/rt: Move sched_rt_entity::back to CONFIG_RT_GROUP_SCHED

[Yajun Deng]

The member back in struct sched_rt_entity only related to RT_GROUP_SCHED,
it should not place out of RT_GROUP_SCHED, move back to RT_GROUP_SCHED.
It will save a few bytes.

Yajun Deng (2):
  sched/rt: Use 'back' only if CONFIG_RT_GROUP_SCHED is enabled
  sched/headers: Move sched_rt_entity::back to under the
    CONFIG_RT_GROUP_SCHED block

 include/linux/sched.h |  2 +-
 kernel/sched/rt.c     | 15 ++++++++++-----
 2 files changed, 11 insertions(+), 6 deletions(-)

-- 
2.25.1

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

[Yajun Deng]

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@linux.dev>
---
 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;
+	}
 
 	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
 	}
 
 	dequeue_top_rt_rq(rt_rq_of_se(back), rt_nr_running);
-- 
2.25.1

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

[Steven Rostedt]

On Mon,  3 Jun 2024 20:50:01 +0800
Yajun Deng <yajun.deng@linux.dev> 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@linux.dev>
> ---
>  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.


>  
>  	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;
}

-- Steve


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

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@goodmis.org> wrote:



> 
> On Mon, 3 Jun 2024 20:50:01 +0800
> 
> Yajun Deng <yajun.deng@linux.dev> 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@linux.dev>
> > 
> >  ---
> > 
> >  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);
> >
>

[PATCH 2/2] sched/headers: Move sched_rt_entity::back to under the CONFIG_RT_GROUP_SCHED block

[Yajun Deng]

We already remove the call to the 'back' member when CONFIG_RT_GROUP_SCHED
is disabled.

It's safe to move sched_rt_entity::back to under the
CONFIG_RT_GROUP_SCHED block, it would save a few bytes.

Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
---
 include/linux/sched.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 61591ac6eab6..3af89a67fb61 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -582,8 +582,8 @@ struct sched_rt_entity {
 	unsigned short			on_rq;
 	unsigned short			on_list;
 
-	struct sched_rt_entity		*back;
 #ifdef CONFIG_RT_GROUP_SCHED
+	struct sched_rt_entity		*back;
 	struct sched_rt_entity		*parent;
 	/* rq on which this entity is (to be) queued: */
 	struct rt_rq			*rt_rq;
-- 
2.25.1