* [PATCH] sched/deadline: Skip overflow check if 0 capacity @ 2024-11-08 4:29 Waiman Long 2024-11-08 13:25 ` Juri Lelli 0 siblings, 1 reply; 6+ messages in thread From: Waiman Long @ 2024-11-08 4:29 UTC (permalink / raw) To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider Cc: linux-kernel, Phil Auld, Joel Fernandes, Waiman Long By properly setting up a 1-cpu sched domain (partition) with no task, it was found that offlining that particular CPU failed because dl_bw_check_overflow() in cpuset_cpu_inactive() returned -EBUSY. This is due to the fact that dl_bw_capacity() return 0 as the sched domain has no active CPU causing a false positive in the overflow check. Fix this corner case by skipping the __dl_overflow() check in dl_bw_manage() when the returned capacity is 0. Signed-off-by: Waiman Long <longman@redhat.com> --- kernel/sched/deadline.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c index be1b917dc8ce..0195f350d6d3 100644 --- a/kernel/sched/deadline.c +++ b/kernel/sched/deadline.c @@ -3479,7 +3479,13 @@ static int dl_bw_manage(enum dl_bw_request req, int cpu, u64 dl_bw) } else { unsigned long cap = dl_bw_capacity(cpu); - overflow = __dl_overflow(dl_b, cap, 0, dl_bw); + /* + * In the unlikely case of 0 capacity (e.g. a sched domain + * with no active CPUs), skip the overflow check as it will + * always return a false positive. + */ + if (likely(cap)) + overflow = __dl_overflow(dl_b, cap, 0, dl_bw); if (req == dl_bw_req_alloc && !overflow) { /* -- 2.47.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/deadline: Skip overflow check if 0 capacity 2024-11-08 4:29 [PATCH] sched/deadline: Skip overflow check if 0 capacity Waiman Long @ 2024-11-08 13:25 ` Juri Lelli 2024-11-08 13:39 ` Waiman Long 0 siblings, 1 reply; 6+ messages in thread From: Juri Lelli @ 2024-11-08 13:25 UTC (permalink / raw) To: Waiman Long Cc: Ingo Molnar, Peter Zijlstra, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, linux-kernel, Phil Auld, Joel Fernandes On 07/11/24 23:29, Waiman Long wrote: > By properly setting up a 1-cpu sched domain (partition) with no > task, it was found that offlining that particular CPU failed because > dl_bw_check_overflow() in cpuset_cpu_inactive() returned -EBUSY. This > is due to the fact that dl_bw_capacity() return 0 as the sched domain > has no active CPU causing a false positive in the overflow check. > > Fix this corner case by skipping the __dl_overflow() check in > dl_bw_manage() when the returned capacity is 0. > > Signed-off-by: Waiman Long <longman@redhat.com> > --- > kernel/sched/deadline.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c > index be1b917dc8ce..0195f350d6d3 100644 > --- a/kernel/sched/deadline.c > +++ b/kernel/sched/deadline.c > @@ -3479,7 +3479,13 @@ static int dl_bw_manage(enum dl_bw_request req, int cpu, u64 dl_bw) > } else { > unsigned long cap = dl_bw_capacity(cpu); > > - overflow = __dl_overflow(dl_b, cap, 0, dl_bw); > + /* > + * In the unlikely case of 0 capacity (e.g. a sched domain > + * with no active CPUs), skip the overflow check as it will > + * always return a false positive. > + */ > + if (likely(cap)) > + overflow = __dl_overflow(dl_b, cap, 0, dl_bw); The remaining total_bw that make this check fail should be the one relative to the dl_server on the cpu that is going offline. Wonder if we shouldn't rather clean that up (remove dl_server contribution) before we get to this point during an hotplug operation. Need to think about it a little more. Thanks, Juri ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/deadline: Skip overflow check if 0 capacity 2024-11-08 13:25 ` Juri Lelli @ 2024-11-08 13:39 ` Waiman Long 2024-11-08 16:31 ` Juri Lelli 0 siblings, 1 reply; 6+ messages in thread From: Waiman Long @ 2024-11-08 13:39 UTC (permalink / raw) To: Juri Lelli Cc: Ingo Molnar, Peter Zijlstra, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, linux-kernel, Phil Auld, Joel Fernandes On 11/8/24 8:25 AM, Juri Lelli wrote: > On 07/11/24 23:29, Waiman Long wrote: >> By properly setting up a 1-cpu sched domain (partition) with no >> task, it was found that offlining that particular CPU failed because >> dl_bw_check_overflow() in cpuset_cpu_inactive() returned -EBUSY. This >> is due to the fact that dl_bw_capacity() return 0 as the sched domain >> has no active CPU causing a false positive in the overflow check. >> >> Fix this corner case by skipping the __dl_overflow() check in >> dl_bw_manage() when the returned capacity is 0. >> >> Signed-off-by: Waiman Long <longman@redhat.com> >> --- >> kernel/sched/deadline.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c >> index be1b917dc8ce..0195f350d6d3 100644 >> --- a/kernel/sched/deadline.c >> +++ b/kernel/sched/deadline.c >> @@ -3479,7 +3479,13 @@ static int dl_bw_manage(enum dl_bw_request req, int cpu, u64 dl_bw) >> } else { >> unsigned long cap = dl_bw_capacity(cpu); >> >> - overflow = __dl_overflow(dl_b, cap, 0, dl_bw); >> + /* >> + * In the unlikely case of 0 capacity (e.g. a sched domain >> + * with no active CPUs), skip the overflow check as it will >> + * always return a false positive. >> + */ >> + if (likely(cap)) >> + overflow = __dl_overflow(dl_b, cap, 0, dl_bw); > The remaining total_bw that make this check fail should be the one > relative to the dl_server on the cpu that is going offline. Wonder if we > shouldn't rather clean that up (remove dl_server contribution) before we > get to this point during an hotplug operation. Need to think about it a > little more. static inline bool __dl_overflow(struct dl_bw *dl_b, unsigned long cap, u64 old_bw, u64 new_bw) { return dl_b->bw != -1 && cap_scale(dl_b->bw, cap) < dl_b->total_bw - old_bw + new_bw; } With a 0 cap, cap_scale(dl_b->bw, cap) will always be 0. As long as total_bw isn't 0 and bw isn't -1, the condition will be true. Cheers, Longman ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/deadline: Skip overflow check if 0 capacity 2024-11-08 13:39 ` Waiman Long @ 2024-11-08 16:31 ` Juri Lelli 2024-11-08 18:41 ` Waiman Long 2024-11-08 18:47 ` Waiman Long 0 siblings, 2 replies; 6+ messages in thread From: Juri Lelli @ 2024-11-08 16:31 UTC (permalink / raw) To: Waiman Long Cc: Ingo Molnar, Peter Zijlstra, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, linux-kernel, Phil Auld, Joel Fernandes On 08/11/24 08:39, Waiman Long wrote: > On 11/8/24 8:25 AM, Juri Lelli wrote: > > On 07/11/24 23:29, Waiman Long wrote: > > > By properly setting up a 1-cpu sched domain (partition) with no > > > task, it was found that offlining that particular CPU failed because > > > dl_bw_check_overflow() in cpuset_cpu_inactive() returned -EBUSY. This > > > is due to the fact that dl_bw_capacity() return 0 as the sched domain > > > has no active CPU causing a false positive in the overflow check. > > > > > > Fix this corner case by skipping the __dl_overflow() check in > > > dl_bw_manage() when the returned capacity is 0. > > > > > > Signed-off-by: Waiman Long <longman@redhat.com> > > > --- > > > kernel/sched/deadline.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c > > > index be1b917dc8ce..0195f350d6d3 100644 > > > --- a/kernel/sched/deadline.c > > > +++ b/kernel/sched/deadline.c > > > @@ -3479,7 +3479,13 @@ static int dl_bw_manage(enum dl_bw_request req, int cpu, u64 dl_bw) > > > } else { > > > unsigned long cap = dl_bw_capacity(cpu); > > > - overflow = __dl_overflow(dl_b, cap, 0, dl_bw); > > > + /* > > > + * In the unlikely case of 0 capacity (e.g. a sched domain > > > + * with no active CPUs), skip the overflow check as it will > > > + * always return a false positive. > > > + */ > > > + if (likely(cap)) > > > + overflow = __dl_overflow(dl_b, cap, 0, dl_bw); > > The remaining total_bw that make this check fail should be the one > > relative to the dl_server on the cpu that is going offline. Wonder if we > > shouldn't rather clean that up (remove dl_server contribution) before we > > get to this point during an hotplug operation. Need to think about it a > > little more. > static inline bool > __dl_overflow(struct dl_bw *dl_b, unsigned long cap, u64 old_bw, u64 new_bw) > { > return dl_b->bw != -1 && > cap_scale(dl_b->bw, cap) < dl_b->total_bw - old_bw + new_bw; > } > > With a 0 cap, cap_scale(dl_b->bw, cap) will always be 0. As long as total_bw > isn't 0 and bw isn't -1, the condition will be true. Right, but I fear that by hiding the special corner case we would also miss the cases where we have DEADLINE tasks with bandwidth on that single CPU and then ignore it. So, maybe something like the below? --- kernel/sched/deadline.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c index be1b917dc8ce..7884e566557c 100644 --- a/kernel/sched/deadline.c +++ b/kernel/sched/deadline.c @@ -130,11 +130,24 @@ static inline int dl_bw_cpus(int i) if (cpumask_subset(rd->span, cpu_active_mask)) return cpumask_weight(rd->span); + /* + * Hotplug extreme case in which the last remaining online CPU in a + * root domain is going offline. We get here after that cpus has been + * cleared from cpu_active_mask, so the loop below would return 0 + * CPUs, which of course doesn't make much sense. Return at least 1 + * CPU. + */ + + if (cpumask_weight(rd->span) == 1) + return 1; + cpus = 0; for_each_cpu_and(i, rd->span, cpu_active_mask) cpus++; + WARN_ON(!cpus); + return cpus; } --- That said though, I believe I just found an additional issue. With the above the system doesn't crash (it did w/o it), but happily moves DEADLINE tasks out of a domain with a single CPU going offline. Last time I looked at this we were properly checking and failing the hotplug operation, but it was indeed a while ago, so not sure yet what changed. More staring. Oh, so broken, yay. :) Best, Juri ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/deadline: Skip overflow check if 0 capacity 2024-11-08 16:31 ` Juri Lelli @ 2024-11-08 18:41 ` Waiman Long 2024-11-08 18:47 ` Waiman Long 1 sibling, 0 replies; 6+ messages in thread From: Waiman Long @ 2024-11-08 18:41 UTC (permalink / raw) To: Juri Lelli, Waiman Long Cc: Ingo Molnar, Peter Zijlstra, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, linux-kernel, Phil Auld, Joel Fernandes On 11/8/24 11:31 AM, Juri Lelli wrote: > On 08/11/24 08:39, Waiman Long wrote: >> On 11/8/24 8:25 AM, Juri Lelli wrote: >>> On 07/11/24 23:29, Waiman Long wrote: >>>> By properly setting up a 1-cpu sched domain (partition) with no >>>> task, it was found that offlining that particular CPU failed because >>>> dl_bw_check_overflow() in cpuset_cpu_inactive() returned -EBUSY. This >>>> is due to the fact that dl_bw_capacity() return 0 as the sched domain >>>> has no active CPU causing a false positive in the overflow check. >>>> >>>> Fix this corner case by skipping the __dl_overflow() check in >>>> dl_bw_manage() when the returned capacity is 0. >>>> >>>> Signed-off-by: Waiman Long <longman@redhat.com> >>>> --- >>>> kernel/sched/deadline.c | 8 +++++++- >>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c >>>> index be1b917dc8ce..0195f350d6d3 100644 >>>> --- a/kernel/sched/deadline.c >>>> +++ b/kernel/sched/deadline.c >>>> @@ -3479,7 +3479,13 @@ static int dl_bw_manage(enum dl_bw_request req, int cpu, u64 dl_bw) >>>> } else { >>>> unsigned long cap = dl_bw_capacity(cpu); >>>> - overflow = __dl_overflow(dl_b, cap, 0, dl_bw); >>>> + /* >>>> + * In the unlikely case of 0 capacity (e.g. a sched domain >>>> + * with no active CPUs), skip the overflow check as it will >>>> + * always return a false positive. >>>> + */ >>>> + if (likely(cap)) >>>> + overflow = __dl_overflow(dl_b, cap, 0, dl_bw); >>> The remaining total_bw that make this check fail should be the one >>> relative to the dl_server on the cpu that is going offline. Wonder if we >>> shouldn't rather clean that up (remove dl_server contribution) before we >>> get to this point during an hotplug operation. Need to think about it a >>> little more. >> static inline bool >> __dl_overflow(struct dl_bw *dl_b, unsigned long cap, u64 old_bw, u64 new_bw) >> { >> return dl_b->bw != -1 && >> cap_scale(dl_b->bw, cap) < dl_b->total_bw - old_bw + new_bw; >> } >> >> With a 0 cap, cap_scale(dl_b->bw, cap) will always be 0. As long as total_bw >> isn't 0 and bw isn't -1, the condition will be true. > Right, but I fear that by hiding the special corner case we would also > miss the cases where we have DEADLINE tasks with bandwidth on that > single CPU and then ignore it. > > So, maybe something like the below? > > --- > kernel/sched/deadline.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c > index be1b917dc8ce..7884e566557c 100644 > --- a/kernel/sched/deadline.c > +++ b/kernel/sched/deadline.c > @@ -130,11 +130,24 @@ static inline int dl_bw_cpus(int i) > if (cpumask_subset(rd->span, cpu_active_mask)) > return cpumask_weight(rd->span); > > + /* > + * Hotplug extreme case in which the last remaining online CPU in a > + * root domain is going offline. We get here after that cpus has been > + * cleared from cpu_active_mask, so the loop below would return 0 > + * CPUs, which of course doesn't make much sense. Return at least 1 > + * CPU. > + */ > + > + if (cpumask_weight(rd->span) == 1) > + return 1; > + > cpus = 0; > > for_each_cpu_and(i, rd->span, cpu_active_mask) > cpus++; > > + WARN_ON(!cpus); > + > return cpus; > } > --- This patch looks good to me. With this patch and my cpuset patches applied, the test_cpuset_prs.sh test passed without any error. You can add the following tags if you send this patch out. Reported-by: Waiman Long <longman@redhat.com> Tested-by: Waiman Long <longman@redhat.com > That said though, I believe I just found an additional issue. With the > above the system doesn't crash (it did w/o it), but happily moves > DEADLINE tasks out of a domain with a single CPU going offline. Last > time I looked at this we were properly checking and failing the hotplug > operation, but it was indeed a while ago, so not sure yet what changed. > More staring. > > Oh, so broken, yay. :) The cpuset code will move tasks in a partition with no effective CPUs to its parent. Cheers, Longman ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/deadline: Skip overflow check if 0 capacity 2024-11-08 16:31 ` Juri Lelli 2024-11-08 18:41 ` Waiman Long @ 2024-11-08 18:47 ` Waiman Long 1 sibling, 0 replies; 6+ messages in thread From: Waiman Long @ 2024-11-08 18:47 UTC (permalink / raw) To: Juri Lelli, Waiman Long Cc: Ingo Molnar, Peter Zijlstra, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, linux-kernel, Phil Auld, Joel Fernandes On 11/8/24 11:31 AM, Juri Lelli wrote: > That said though, I believe I just found an additional issue. With the > above the system doesn't crash (it did w/o it), but happily moves > DEADLINE tasks out of a domain with a single CPU going offline. Last > time I looked at this we were properly checking and failing the hotplug > operation, but it was indeed a while ago, so not sure yet what changed. > More staring. > > Oh, so broken, yay. 🙂 Is it the case that the null total_bw bug let the cpu offline succeeds in a 1-cpu partition with a DL task and cause it to crash? I am also OK if you adds the check for the presence of a DL task in the cpu and fails the offline check in this case. Thanks, Longman ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-11-08 18:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-11-08 4:29 [PATCH] sched/deadline: Skip overflow check if 0 capacity Waiman Long 2024-11-08 13:25 ` Juri Lelli 2024-11-08 13:39 ` Waiman Long 2024-11-08 16:31 ` Juri Lelli 2024-11-08 18:41 ` Waiman Long 2024-11-08 18:47 ` Waiman Long
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.