2023-01-31 13:01:39

by Pietro Borrello

Subject: [PATCH v2] sched: pick_next_rt_entity(): checked list_entry

Commit 326587b84078 ("sched: fix goto retry in pick_next_task_rt()")
removed any path which could make pick_next_rt_entity() return NULL.
However, BUG_ON(!rt_se) in _pick_next_task_rt() (the only caller of
pick_next_rt_entity()) still checks the error condition, which can
never happen, since list_entry() never returns NULL.
Remove the BUG_ON check, and instead emit a warning in the only
possible error condition here: the queue being empty which should
never happen.

Fixes: 326587b84078 ("sched: fix goto retry in pick_next_task_rt()")
Signed-off-by: Pietro Borrello <[email protected]>
---
Changes in v2:
- pick_next_rt_entity(): emit warning instead of crashing
- Link to v1: https://lore.kernel.org/r/20230128-list-entry-null-check-sched-v1-1-c93085ee0055@diag.uniroma1.it
---
kernel/sched/rt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index ed2a47e4ddae..c024529d8416 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1777,6 +1777,7 @@ static struct sched_rt_entity *pick_next_rt_entity(struct rt_rq *rt_rq)
BUG_ON(idx >= MAX_RT_PRIO);

queue = array->queue + idx;
+ SCHED_WARN_ON(list_empty(queue));
next = list_entry(queue->next, struct sched_rt_entity, run_list);

return next;
@@ -1789,7 +1790,6 @@ static struct task_struct *_pick_next_task_rt(struct rq *rq)

do {
rt_se = pick_next_rt_entity(rt_rq);
- BUG_ON(!rt_se);
rt_rq = group_rt_rq(rt_se);
} while (rt_rq);


---
base-commit: 2241ab53cbb5cdb08a6b2d4688feb13971058f65
change-id: 20230128-list-entry-null-check-sched-a3f3dfd6d468

Best regards,
--
Pietro Borrello <[email protected]>

2023-02-06 16:24:15

by Steven Rostedt

Subject: Re: [PATCH v2] sched: pick_next_rt_entity(): checked list_entry

On Tue, 31 Jan 2023 13:01:16 +0000
Pietro Borrello <[email protected]> wrote:

> index ed2a47e4ddae..c024529d8416 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -1777,6 +1777,7 @@ static struct sched_rt_entity *pick_next_rt_entity(struct rt_rq *rt_rq)
> BUG_ON(idx >= MAX_RT_PRIO);
>
> queue = array->queue + idx;
> + SCHED_WARN_ON(list_empty(queue));

I wonder if we should make this:

if (SCHED_WARN_ON(list_empty(queue)))
return NULL;

> next = list_entry(queue->next, struct sched_rt_entity, run_list);
>
> return next;
> @@ -1789,7 +1790,6 @@ static struct task_struct *_pick_next_task_rt(struct rq *rq)
>
> do {
> rt_se = pick_next_rt_entity(rt_rq);
> - BUG_ON(!rt_se);

if (unlikely(!rt_se))
return NULL;

-- Steve

> rt_rq = group_rt_rq(rt_se);
> } while (rt_rq);
>

2023-02-06 16:58:32

by Phil Auld

Subject: Re: [PATCH v2] sched: pick_next_rt_entity(): checked list_entry

On Mon, Feb 06, 2023 at 11:23:42AM -0500 Steven Rostedt wrote:
> On Tue, 31 Jan 2023 13:01:16 +0000
> Pietro Borrello <[email protected]> wrote:
>
> > index ed2a47e4ddae..c024529d8416 100644
> > --- a/kernel/sched/rt.c
> > +++ b/kernel/sched/rt.c
> > @@ -1777,6 +1777,7 @@ static struct sched_rt_entity *pick_next_rt_entity(struct rt_rq *rt_rq)
> > BUG_ON(idx >= MAX_RT_PRIO);
> >
> > queue = array->queue + idx;
> > + SCHED_WARN_ON(list_empty(queue));
>
> I wonder if we should make this:
>
> if (SCHED_WARN_ON(list_empty(queue)))
> return NULL;
>
> > next = list_entry(queue->next, struct sched_rt_entity, run_list);
> >
> > return next;
> > @@ -1789,7 +1790,6 @@ static struct task_struct *_pick_next_task_rt(struct rq *rq)
> >
> > do {
> > rt_se = pick_next_rt_entity(rt_rq);
> > - BUG_ON(!rt_se);
>
> if (unlikely(!rt_se))
> return NULL;

I think that's better than taking a digger in one of the subsequent macros.


Cheers,
Phil


>
> -- Steve
>
> > rt_rq = group_rt_rq(rt_se);
> > } while (rt_rq);
> >
>



--

2023-02-06 22:38:30

by Pietro Borrello

Subject: Re: [PATCH v2] sched: pick_next_rt_entity(): checked list_entry

On Mon, 6 Feb 2023 at 17:57, Phil Auld <[email protected]> wrote:
>
> On Mon, Feb 06, 2023 at 11:23:42AM -0500 Steven Rostedt wrote:
> > On Tue, 31 Jan 2023 13:01:16 +0000
> > Pietro Borrello <[email protected]> wrote:
> >
> > > index ed2a47e4ddae..c024529d8416 100644
> > > --- a/kernel/sched/rt.c
> > > +++ b/kernel/sched/rt.c
> > > @@ -1777,6 +1777,7 @@ static struct sched_rt_entity *pick_next_rt_entity(struct rt_rq *rt_rq)
> > > BUG_ON(idx >= MAX_RT_PRIO);
> > >
> > > queue = array->queue + idx;
> > > + SCHED_WARN_ON(list_empty(queue));
> >
> > I wonder if we should make this:
> >
> > if (SCHED_WARN_ON(list_empty(queue)))
> > return NULL;
> >
> > > next = list_entry(queue->next, struct sched_rt_entity, run_list);
> > >
> > > return next;
> > > @@ -1789,7 +1790,6 @@ static struct task_struct *_pick_next_task_rt(struct rq *rq)
> > >
> > > do {
> > > rt_se = pick_next_rt_entity(rt_rq);
> > > - BUG_ON(!rt_se);
> >
> > if (unlikely(!rt_se))
> > return NULL;
>
> I think that's better than taking a digger in one of the subsequent macros.
>

Thanks for the feedback.
Fixed in v3: https://lore.kernel.org/all/20230128-list-entry-null-check-sched-v3-1-b1a71bd1ac6b@diag.uniroma1.it/T/#u

Best regards,
Pietro