* [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up @ 2018-01-08 6:14 Byungchul Park 2018-01-08 6:14 ` [RESEND PATCH v3 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value Byungchul Park 2018-01-11 9:07 ` [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up Peter Zijlstra 0 siblings, 2 replies; 17+ messages in thread From: Byungchul Park @ 2018-01-08 6:14 UTC (permalink / raw) To: peterz, mingo, rostedt Cc: tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team Changes from v2 - Run spellchecker over the text and fix typos - Add acked-by Daniel Changes from v1 - Enhance commit msg - Prevent WARN in cpumask_test_cpu() in cpudl_find() when best_cpu == -1 -----8<----- >From 7735382d07ae6a61d740ae39ba2ecf169d43b8a2 Mon Sep 17 00:00:00 2001 From: Byungchul Park <byungchul.park@lge.com> Date: Wed, 22 Mar 2017 14:25:56 +0900 Subject: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up Current code uses cpudl_maximum() to get the root node's cpu, while it directly accesses the root node like 'cp->elements[0].dl' to get the root node's dl. It would be more readable to add a function for the dl, as well. Added it. Signed-off-by: Byungchul Park <byungchul.park@lge.com> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org> Acked-by: Daniel Bristot de Oliveira <bristot@redhat.com> --- kernel/sched/cpudeadline.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c index 8d9562d..9f02035 100644 --- a/kernel/sched/cpudeadline.c +++ b/kernel/sched/cpudeadline.c @@ -108,11 +108,16 @@ static void cpudl_heapify(struct cpudl *cp, int idx) cpudl_heapify_down(cp, idx); } -static inline int cpudl_maximum(struct cpudl *cp) +static inline int cpudl_maximum_cpu(struct cpudl *cp) { return cp->elements[0].cpu; } +static inline u64 cpudl_maximum_dl(struct cpudl *cp) +{ + return cp->elements[0].dl; +} + /* * cpudl_find - find the best (later-dl) CPU in the system * @cp: the cpudl max-heap context @@ -130,11 +135,11 @@ int cpudl_find(struct cpudl *cp, struct task_struct *p, cpumask_and(later_mask, cp->free_cpus, &p->cpus_allowed)) { return 1; } else { - int best_cpu = cpudl_maximum(cp); + int best_cpu = cpudl_maximum_cpu(cp); WARN_ON(best_cpu != -1 && !cpu_present(best_cpu)); if (cpumask_test_cpu(best_cpu, &p->cpus_allowed) && - dl_time_before(dl_se->deadline, cp->elements[0].dl)) { + dl_time_before(dl_se->deadline, cpudl_maximum_dl(cp))) { if (later_mask) cpumask_set_cpu(best_cpu, later_mask); -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [RESEND PATCH v3 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value 2018-01-08 6:14 [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up Byungchul Park @ 2018-01-08 6:14 ` Byungchul Park 2018-06-01 6:12 ` [RESEND, v3, " Joel Fernandes 2018-01-11 9:07 ` [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up Peter Zijlstra 1 sibling, 1 reply; 17+ messages in thread From: Byungchul Park @ 2018-01-08 6:14 UTC (permalink / raw) To: peterz, mingo, rostedt Cc: tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team Currently, migrating tasks to cpu0 unconditionally happens when the heap is empty, since cp->elements[].cpu was initialized to 0(=cpu0). We have to distinguish between the empty case and cpu0 to avoid the unnecessary migrations. Therefore, it has to return an invalid value e.i. -1 in that case. Signed-off-by: Byungchul Park <byungchul.park@lge.com> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org> Acked-by: Daniel Bristot de Oliveira <bristot@redhat.com> --- kernel/sched/cpudeadline.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c index 9f02035..bcf903f 100644 --- a/kernel/sched/cpudeadline.c +++ b/kernel/sched/cpudeadline.c @@ -138,6 +138,12 @@ int cpudl_find(struct cpudl *cp, struct task_struct *p, int best_cpu = cpudl_maximum_cpu(cp); WARN_ON(best_cpu != -1 && !cpu_present(best_cpu)); + /* + * The heap tree is empty for now, just return. + */ + if (best_cpu == -1) + return 0; + if (cpumask_test_cpu(best_cpu, &p->cpus_allowed) && dl_time_before(dl_se->deadline, cpudl_maximum_dl(cp))) { if (later_mask) @@ -265,8 +271,10 @@ int cpudl_init(struct cpudl *cp) return -ENOMEM; } - for_each_possible_cpu(i) + for_each_possible_cpu(i) { + cp->elements[i].cpu = -1; cp->elements[i].idx = IDX_INVALID; + } return 0; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [RESEND, v3, 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value 2018-01-08 6:14 ` [RESEND PATCH v3 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value Byungchul Park @ 2018-06-01 6:12 ` Joel Fernandes 2018-06-01 6:18 ` Joel Fernandes 0 siblings, 1 reply; 17+ messages in thread From: Joel Fernandes @ 2018-06-01 6:12 UTC (permalink / raw) To: byungchul park Cc: peterz, mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team On Mon, Jan 08, 2018 at 03:14:41PM +0900, byungchul park wrote: > Currently, migrating tasks to cpu0 unconditionally happens when the > heap is empty, since cp->elements[].cpu was initialized to 0(=cpu0). > We have to distinguish between the empty case and cpu0 to avoid the > unnecessary migrations. Therefore, it has to return an invalid value > e.i. -1 in that case. > > Signed-off-by: Byungchul Park <byungchul.park@lge.com> > Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > Acked-by: Daniel Bristot de Oliveira <bristot@redhat.com> > --- > kernel/sched/cpudeadline.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c > index 9f02035..bcf903f 100644 > --- a/kernel/sched/cpudeadline.c > +++ b/kernel/sched/cpudeadline.c > @@ -138,6 +138,12 @@ int cpudl_find(struct cpudl *cp, struct task_struct *p, > int best_cpu = cpudl_maximum_cpu(cp); > WARN_ON(best_cpu != -1 && !cpu_present(best_cpu)); > > + /* > + * The heap tree is empty for now, just return. > + */ > + if (best_cpu == -1) > + return 0; > + > if (cpumask_test_cpu(best_cpu, &p->cpus_allowed) && > dl_time_before(dl_se->deadline, cpudl_maximum_dl(cp))) { > if (later_mask) > @@ -265,8 +271,10 @@ int cpudl_init(struct cpudl *cp) > return -ENOMEM; > } > > - for_each_possible_cpu(i) > + for_each_possible_cpu(i) { > + cp->elements[i].cpu = -1; > cp->elements[i].idx = IDX_INVALID; Shouldn't you also set cp->elements[cpu].cpu to -1 in cpudl_clear (when you set cp->elements[cpu].cpu to IDX_INVALID there)? thanks, - Joel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND, v3, 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value 2018-06-01 6:12 ` [RESEND, v3, " Joel Fernandes @ 2018-06-01 6:18 ` Joel Fernandes 2018-06-01 7:10 ` Byungchul Park 0 siblings, 1 reply; 17+ messages in thread From: Joel Fernandes @ 2018-06-01 6:18 UTC (permalink / raw) To: byungchul park Cc: peterz, mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team On Thu, May 31, 2018 at 11:12:55PM -0700, Joel Fernandes wrote: > On Mon, Jan 08, 2018 at 03:14:41PM +0900, byungchul park wrote: > > Currently, migrating tasks to cpu0 unconditionally happens when the > > heap is empty, since cp->elements[].cpu was initialized to 0(=cpu0). > > We have to distinguish between the empty case and cpu0 to avoid the > > unnecessary migrations. Therefore, it has to return an invalid value > > e.i. -1 in that case. > > > > Signed-off-by: Byungchul Park <byungchul.park@lge.com> > > Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > > Acked-by: Daniel Bristot de Oliveira <bristot@redhat.com> > > --- > > kernel/sched/cpudeadline.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c > > index 9f02035..bcf903f 100644 > > --- a/kernel/sched/cpudeadline.c > > +++ b/kernel/sched/cpudeadline.c > > @@ -138,6 +138,12 @@ int cpudl_find(struct cpudl *cp, struct task_struct *p, > > int best_cpu = cpudl_maximum_cpu(cp); > > WARN_ON(best_cpu != -1 && !cpu_present(best_cpu)); > > > > + /* > > + * The heap tree is empty for now, just return. > > + */ > > + if (best_cpu == -1) > > + return 0; > > + > > if (cpumask_test_cpu(best_cpu, &p->cpus_allowed) && > > dl_time_before(dl_se->deadline, cpudl_maximum_dl(cp))) { > > if (later_mask) > > @@ -265,8 +271,10 @@ int cpudl_init(struct cpudl *cp) > > return -ENOMEM; > > } > > > > - for_each_possible_cpu(i) > > + for_each_possible_cpu(i) { > > + cp->elements[i].cpu = -1; > > cp->elements[i].idx = IDX_INVALID; > > Shouldn't you also set cp->elements[cpu].cpu to -1 in cpudl_clear (when you > set cp->elements[cpu].cpu to IDX_INVALID there)? I messed up my words, I meant : "when setting cp->elements[cpu].idx to IDX_INVALID there". Which means I need to call it a day :-) - Joel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND, v3, 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value 2018-06-01 6:18 ` Joel Fernandes @ 2018-06-01 7:10 ` Byungchul Park 2018-06-01 15:52 ` Joel Fernandes 0 siblings, 1 reply; 17+ messages in thread From: Byungchul Park @ 2018-06-01 7:10 UTC (permalink / raw) To: Joel Fernandes Cc: peterz, mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team On 2018-06-01 15:18, Joel Fernandes wrote: > On Thu, May 31, 2018 at 11:12:55PM -0700, Joel Fernandes wrote: >> On Mon, Jan 08, 2018 at 03:14:41PM +0900, byungchul park wrote: >>> Currently, migrating tasks to cpu0 unconditionally happens when the >>> heap is empty, since cp->elements[].cpu was initialized to 0(=cpu0). >>> We have to distinguish between the empty case and cpu0 to avoid the >>> unnecessary migrations. Therefore, it has to return an invalid value >>> e.i. -1 in that case. >>> >>> Signed-off-by: Byungchul Park <byungchul.park@lge.com> >>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org> >>> Acked-by: Daniel Bristot de Oliveira <bristot@redhat.com> >>> --- >>> kernel/sched/cpudeadline.c | 10 +++++++++- >>> 1 file changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c >>> index 9f02035..bcf903f 100644 >>> --- a/kernel/sched/cpudeadline.c >>> +++ b/kernel/sched/cpudeadline.c >>> @@ -138,6 +138,12 @@ int cpudl_find(struct cpudl *cp, struct task_struct *p, >>> int best_cpu = cpudl_maximum_cpu(cp); >>> WARN_ON(best_cpu != -1 && !cpu_present(best_cpu)); >>> >>> + /* >>> + * The heap tree is empty for now, just return. >>> + */ >>> + if (best_cpu == -1) >>> + return 0; >>> + >>> if (cpumask_test_cpu(best_cpu, &p->cpus_allowed) && >>> dl_time_before(dl_se->deadline, cpudl_maximum_dl(cp))) { >>> if (later_mask) >>> @@ -265,8 +271,10 @@ int cpudl_init(struct cpudl *cp) >>> return -ENOMEM; >>> } >>> >>> - for_each_possible_cpu(i) >>> + for_each_possible_cpu(i) { >>> + cp->elements[i].cpu = -1; >>> cp->elements[i].idx = IDX_INVALID; >> >> Shouldn't you also set cp->elements[cpu].cpu to -1 in cpudl_clear (when you >> set cp->elements[cpu].cpu to IDX_INVALID there)? > > I messed up my words, I meant : "when setting cp->elements[cpu].idx to > IDX_INVALID there". Which means I need to call it a day :-) Ah.. I agree with you. It might be a problem when removing the last element.. Then I think the following change should also be applied additionally. Right? --- diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c index 8d9562d..44d4c88 100644 --- a/kernel/sched/cpudeadline.c +++ b/kernel/sched/cpudeadline.c @@ -172,12 +172,14 @@ void cpudl_clear(struct cpudl *cp, int cpu) } else { new_cpu = cp->elements[cp->size - 1].cpu; cp->elements[old_idx].dl = cp->elements[cp->size - 1].dl; - cp->elements[old_idx].cpu = new_cpu; + cp->elements[old_idx].cpu = (new_cpu == cpu) ? -1 : new_cpu; cp->size--; - cp->elements[new_cpu].idx = old_idx; cp->elements[cpu].idx = IDX_INVALID; - cpudl_heapify(cp, old_idx); + if (new_cpu != cpu) { + cp->elements[new_cpu].idx = old_idx; + cpudl_heapify(cp, old_idx); + } cpumask_set_cpu(cpu, cp->free_cpus); } raw_spin_unlock_irqrestore(&cp->lock, flags); -- Thanks, Byungchul ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [RESEND, v3, 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value 2018-06-01 7:10 ` Byungchul Park @ 2018-06-01 15:52 ` Joel Fernandes 2018-06-03 5:20 ` Byungchul Park 0 siblings, 1 reply; 17+ messages in thread From: Joel Fernandes @ 2018-06-01 15:52 UTC (permalink / raw) To: Byungchul Park Cc: peterz, mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team, kernel-team On Fri, Jun 01, 2018 at 04:10:56PM +0900, Byungchul Park wrote: > > On Thu, May 31, 2018 at 11:12:55PM -0700, Joel Fernandes wrote: > > > On Mon, Jan 08, 2018 at 03:14:41PM +0900, byungchul park wrote: > > > > Currently, migrating tasks to cpu0 unconditionally happens when the > > > > heap is empty, since cp->elements[].cpu was initialized to 0(=cpu0). > > > > We have to distinguish between the empty case and cpu0 to avoid the > > > > unnecessary migrations. Therefore, it has to return an invalid value > > > > e.i. -1 in that case. > > > > > > > > Signed-off-by: Byungchul Park <byungchul.park@lge.com> > > > > Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > > > > Acked-by: Daniel Bristot de Oliveira <bristot@redhat.com> > > > > --- > > > > kernel/sched/cpudeadline.c | 10 +++++++++- > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c > > > > index 9f02035..bcf903f 100644 > > > > --- a/kernel/sched/cpudeadline.c > > > > +++ b/kernel/sched/cpudeadline.c > > > > @@ -138,6 +138,12 @@ int cpudl_find(struct cpudl *cp, struct task_struct *p, > > > > int best_cpu = cpudl_maximum_cpu(cp); > > > > WARN_ON(best_cpu != -1 && !cpu_present(best_cpu)); > > > > + /* > > > > + * The heap tree is empty for now, just return. > > > > + */ > > > > + if (best_cpu == -1) > > > > + return 0; > > > > + > > > > if (cpumask_test_cpu(best_cpu, &p->cpus_allowed) && > > > > dl_time_before(dl_se->deadline, cpudl_maximum_dl(cp))) { > > > > if (later_mask) > > > > @@ -265,8 +271,10 @@ int cpudl_init(struct cpudl *cp) > > > > return -ENOMEM; > > > > } > > > > - for_each_possible_cpu(i) > > > > + for_each_possible_cpu(i) { > > > > + cp->elements[i].cpu = -1; > > > > cp->elements[i].idx = IDX_INVALID; > > > > > > Shouldn't you also set cp->elements[cpu].cpu to -1 in cpudl_clear (when you > > > set cp->elements[cpu].cpu to IDX_INVALID there)? > > > > I messed up my words, I meant : "when setting cp->elements[cpu].idx to > > IDX_INVALID there". Which means I need to call it a day :-) > > Ah.. I agree with you. It might be a problem when removing the last > element.. Then I think the following change should also be applied > additionally. Right? Yes. > --- > diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c > index 8d9562d..44d4c88 100644 > --- a/kernel/sched/cpudeadline.c > +++ b/kernel/sched/cpudeadline.c > @@ -172,12 +172,14 @@ void cpudl_clear(struct cpudl *cp, int cpu) > } else { > new_cpu = cp->elements[cp->size - 1].cpu; > cp->elements[old_idx].dl = cp->elements[cp->size - 1].dl; > - cp->elements[old_idx].cpu = new_cpu; > + cp->elements[old_idx].cpu = (new_cpu == cpu) ? -1 : new_cpu; > cp->size--; > - cp->elements[new_cpu].idx = old_idx; > cp->elements[cpu].idx = IDX_INVALID; > - cpudl_heapify(cp, old_idx); > > + if (new_cpu != cpu) { > + cp->elements[new_cpu].idx = old_idx; > + cpudl_heapify(cp, old_idx); > + } > cpumask_set_cpu(cpu, cp->free_cpus); This looks a bit confusing. How about the following? (untested) thanks, - Joel ---8<----------------------- diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c index 50316455ea66..741a97e58c05 100644 --- a/kernel/sched/cpudeadline.c +++ b/kernel/sched/cpudeadline.c @@ -129,6 +129,10 @@ int cpudl_find(struct cpudl *cp, struct task_struct *p, } else { int best_cpu = cpudl_maximum(cp); + /* The max-heap is empty, just return. */ + if (best_cpu == -1) + return 0; + WARN_ON(best_cpu != -1 && !cpu_present(best_cpu)); if (cpumask_test_cpu(best_cpu, &p->cpus_allowed) && @@ -167,6 +171,12 @@ void cpudl_clear(struct cpudl *cp, int cpu) * This could happen if a rq_offline_dl is * called for a CPU without -dl tasks running. */ + } else if (cp->size == 1) { + /* Only one element in max-heap, clear it */ + cp->elements[0].cpu = -1; + cp->elements[cpu].idx = IDX_INVALID; + cp->size--; + cpumask_set_cpu(cpu, cp->free_cpus); } else { new_cpu = cp->elements[cp->size - 1].cpu; cp->elements[old_idx].dl = cp->elements[cp->size - 1].dl; @@ -262,6 +272,9 @@ int cpudl_init(struct cpudl *cp) for_each_possible_cpu(i) cp->elements[i].idx = IDX_INVALID; + /* Mark heap as initially empty */ + cp->elements[0].cpu = -1; + return 0; } ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [RESEND, v3, 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value 2018-06-01 15:52 ` Joel Fernandes @ 2018-06-03 5:20 ` Byungchul Park 0 siblings, 0 replies; 17+ messages in thread From: Byungchul Park @ 2018-06-03 5:20 UTC (permalink / raw) To: Joel Fernandes Cc: Byungchul Park, Peter Zijlstra, Ingo Molnar, rostedt, Thomas Gleixner, raistlin, linux-kernel, Juri Lelli, bristot, kernel-team, kernel-team On Sat, Jun 2, 2018 at 12:52 AM, Joel Fernandes <joel@joelfernandes.org> wrote: > On Fri, Jun 01, 2018 at 04:10:56PM +0900, Byungchul Park wrote: >> > On Thu, May 31, 2018 at 11:12:55PM -0700, Joel Fernandes wrote: >> > > On Mon, Jan 08, 2018 at 03:14:41PM +0900, byungchul park wrote: >> > > > Currently, migrating tasks to cpu0 unconditionally happens when the >> > > > heap is empty, since cp->elements[].cpu was initialized to 0(=cpu0). >> > > > We have to distinguish between the empty case and cpu0 to avoid the >> > > > unnecessary migrations. Therefore, it has to return an invalid value >> > > > e.i. -1 in that case. >> > > > >> > > > Signed-off-by: Byungchul Park <byungchul.park@lge.com> >> > > > Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org> >> > > > Acked-by: Daniel Bristot de Oliveira <bristot@redhat.com> >> > > > --- >> > > > kernel/sched/cpudeadline.c | 10 +++++++++- >> > > > 1 file changed, 9 insertions(+), 1 deletion(-) >> > > > >> > > > diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c >> > > > index 9f02035..bcf903f 100644 >> > > > --- a/kernel/sched/cpudeadline.c >> > > > +++ b/kernel/sched/cpudeadline.c >> > > > @@ -138,6 +138,12 @@ int cpudl_find(struct cpudl *cp, struct task_struct *p, >> > > > int best_cpu = cpudl_maximum_cpu(cp); >> > > > WARN_ON(best_cpu != -1 && !cpu_present(best_cpu)); >> > > > + /* >> > > > + * The heap tree is empty for now, just return. >> > > > + */ >> > > > + if (best_cpu == -1) >> > > > + return 0; >> > > > + >> > > > if (cpumask_test_cpu(best_cpu, &p->cpus_allowed) && >> > > > dl_time_before(dl_se->deadline, cpudl_maximum_dl(cp))) { >> > > > if (later_mask) >> > > > @@ -265,8 +271,10 @@ int cpudl_init(struct cpudl *cp) >> > > > return -ENOMEM; >> > > > } >> > > > - for_each_possible_cpu(i) >> > > > + for_each_possible_cpu(i) { >> > > > + cp->elements[i].cpu = -1; >> > > > cp->elements[i].idx = IDX_INVALID; >> > > >> > > Shouldn't you also set cp->elements[cpu].cpu to -1 in cpudl_clear (when you >> > > set cp->elements[cpu].cpu to IDX_INVALID there)? >> > >> > I messed up my words, I meant : "when setting cp->elements[cpu].idx to >> > IDX_INVALID there". Which means I need to call it a day :-) >> >> Ah.. I agree with you. It might be a problem when removing the last >> element.. Then I think the following change should also be applied >> additionally. Right? > > Yes. > >> --- >> diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c >> index 8d9562d..44d4c88 100644 >> --- a/kernel/sched/cpudeadline.c >> +++ b/kernel/sched/cpudeadline.c >> @@ -172,12 +172,14 @@ void cpudl_clear(struct cpudl *cp, int cpu) >> } else { >> new_cpu = cp->elements[cp->size - 1].cpu; >> cp->elements[old_idx].dl = cp->elements[cp->size - 1].dl; >> - cp->elements[old_idx].cpu = new_cpu; >> + cp->elements[old_idx].cpu = (new_cpu == cpu) ? -1 : new_cpu; >> cp->size--; >> - cp->elements[new_cpu].idx = old_idx; >> cp->elements[cpu].idx = IDX_INVALID; >> - cpudl_heapify(cp, old_idx); >> >> + if (new_cpu != cpu) { >> + cp->elements[new_cpu].idx = old_idx; >> + cpudl_heapify(cp, old_idx); >> + } >> cpumask_set_cpu(cpu, cp->free_cpus); > > This looks a bit confusing. How about the following? (untested) Hello, Whatever. Your code also looks good to me. I just wanna follow the maintainers' decision. ;) > thanks, > > - Joel > > ---8<----------------------- > > diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c > index 50316455ea66..741a97e58c05 100644 > --- a/kernel/sched/cpudeadline.c > +++ b/kernel/sched/cpudeadline.c > @@ -129,6 +129,10 @@ int cpudl_find(struct cpudl *cp, struct task_struct *p, > } else { > int best_cpu = cpudl_maximum(cp); > > + /* The max-heap is empty, just return. */ > + if (best_cpu == -1) > + return 0; > + > WARN_ON(best_cpu != -1 && !cpu_present(best_cpu)); > > if (cpumask_test_cpu(best_cpu, &p->cpus_allowed) && > @@ -167,6 +171,12 @@ void cpudl_clear(struct cpudl *cp, int cpu) > * This could happen if a rq_offline_dl is > * called for a CPU without -dl tasks running. > */ > + } else if (cp->size == 1) { > + /* Only one element in max-heap, clear it */ > + cp->elements[0].cpu = -1; > + cp->elements[cpu].idx = IDX_INVALID; > + cp->size--; > + cpumask_set_cpu(cpu, cp->free_cpus); > } else { > new_cpu = cp->elements[cp->size - 1].cpu; > cp->elements[old_idx].dl = cp->elements[cp->size - 1].dl; > @@ -262,6 +272,9 @@ int cpudl_init(struct cpudl *cp) > for_each_possible_cpu(i) > cp->elements[i].idx = IDX_INVALID; > > + /* Mark heap as initially empty */ > + cp->elements[0].cpu = -1; > + > return 0; > } > -- Thanks, Byungchul ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up 2018-01-08 6:14 [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up Byungchul Park 2018-01-08 6:14 ` [RESEND PATCH v3 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value Byungchul Park @ 2018-01-11 9:07 ` Peter Zijlstra 2018-02-09 0:37 ` Byungchul Park ` (3 more replies) 1 sibling, 4 replies; 17+ messages in thread From: Peter Zijlstra @ 2018-01-11 9:07 UTC (permalink / raw) To: Byungchul Park Cc: mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team Sorry for the huge delay on this, but I'll have to postpone further. Still busy with meltdown/spectre stuff. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up 2018-01-11 9:07 ` [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up Peter Zijlstra @ 2018-02-09 0:37 ` Byungchul Park 2018-02-26 7:51 ` Byungchul Park ` (2 subsequent siblings) 3 siblings, 0 replies; 17+ messages in thread From: Byungchul Park @ 2018-02-09 0:37 UTC (permalink / raw) To: Peter Zijlstra Cc: mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team On 1/11/2018 6:07 PM, Peter Zijlstra wrote: > > > Sorry for the huge delay on this, but I'll have to postpone further. > Still busy with meltdown/spectre stuff. Do you have time to see these patches and another set, now? -- Thanks, Byungchul ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up 2018-01-11 9:07 ` [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up Peter Zijlstra 2018-02-09 0:37 ` Byungchul Park @ 2018-02-26 7:51 ` Byungchul Park 2018-03-13 5:52 ` Byungchul Park 2018-05-09 6:33 ` Byungchul Park 3 siblings, 0 replies; 17+ messages in thread From: Byungchul Park @ 2018-02-26 7:51 UTC (permalink / raw) To: Peter Zijlstra Cc: mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team On 1/11/2018 6:07 PM, Peter Zijlstra wrote: > > > Sorry for the huge delay on this, but I'll have to postpone further. > Still busy with meltdown/spectre stuff. Do you have time to see the patch, now that it seems to be managed to solve those security issues? -- Thanks, Byungchul ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up 2018-01-11 9:07 ` [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up Peter Zijlstra 2018-02-09 0:37 ` Byungchul Park 2018-02-26 7:51 ` Byungchul Park @ 2018-03-13 5:52 ` Byungchul Park 2018-04-23 7:01 ` Byungchul Park 2018-05-09 6:33 ` Byungchul Park 3 siblings, 1 reply; 17+ messages in thread From: Byungchul Park @ 2018-03-13 5:52 UTC (permalink / raw) To: Peter Zijlstra Cc: mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team On 1/11/2018 6:07 PM, Peter Zijlstra wrote: > > > Sorry for the huge delay on this, but I'll have to postpone further. > Still busy with meltdown/spectre stuff. Could you review the patch? -- Thanks, Byungchul ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up 2018-03-13 5:52 ` Byungchul Park @ 2018-04-23 7:01 ` Byungchul Park 0 siblings, 0 replies; 17+ messages in thread From: Byungchul Park @ 2018-04-23 7:01 UTC (permalink / raw) To: Peter Zijlstra Cc: mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team On 3/13/2018 2:52 PM, Byungchul Park wrote: > On 1/11/2018 6:07 PM, Peter Zijlstra wrote: >> >> >> Sorry for the huge delay on this, but I'll have to postpone further. >> Still busy with meltdown/spectre stuff. > > Could you review the patch? Hello, Could you see the patch now? -- Thanks, Byungchul ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up 2018-01-11 9:07 ` [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up Peter Zijlstra ` (2 preceding siblings ...) 2018-03-13 5:52 ` Byungchul Park @ 2018-05-09 6:33 ` Byungchul Park 2018-05-25 5:13 ` Byungchul Park 3 siblings, 1 reply; 17+ messages in thread From: Byungchul Park @ 2018-05-09 6:33 UTC (permalink / raw) To: Peter Zijlstra Cc: mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team On Thu, Jan 11, 2018 at 10:07:16AM +0100, Peter Zijlstra wrote: > > > Sorry for the huge delay on this, but I'll have to postpone further. > Still busy with meltdown/spectre stuff. Please consider this. Even though it's not a big bug, anyway leading mis-behavior in certain situaions. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up 2018-05-09 6:33 ` Byungchul Park @ 2018-05-25 5:13 ` Byungchul Park 2018-06-01 3:07 ` Byungchul Park 0 siblings, 1 reply; 17+ messages in thread From: Byungchul Park @ 2018-05-25 5:13 UTC (permalink / raw) To: Peter Zijlstra Cc: mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team On 2018-05-09 15:33, Byungchul Park wrote: > On Thu, Jan 11, 2018 at 10:07:16AM +0100, Peter Zijlstra wrote: >> >> >> Sorry for the huge delay on this, but I'll have to postpone further. >> Still busy with meltdown/spectre stuff. > > Please consider this. Even though it's not a big bug, anyway leading > mis-behavior in certain situaions. Could you see this patches, it's been too long since the start tho? -- Thanks, Byungchul ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up 2018-05-25 5:13 ` Byungchul Park @ 2018-06-01 3:07 ` Byungchul Park 2018-06-01 6:02 ` Joel Fernandes 0 siblings, 1 reply; 17+ messages in thread From: Byungchul Park @ 2018-06-01 3:07 UTC (permalink / raw) To: Peter Zijlstra Cc: mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team On 2018-05-25 14:13, Byungchul Park wrote: > > > On 2018-05-09 15:33, Byungchul Park wrote: >> On Thu, Jan 11, 2018 at 10:07:16AM +0100, Peter Zijlstra wrote: >>> >>> >>> Sorry for the huge delay on this, but I'll have to postpone further. >>> Still busy with meltdown/spectre stuff. >> >> Please consider this. Even though it's not a big bug, anyway leading >> mis-behavior in certain situaions. > > Could you see this patches, it's been too long since the start tho? Please, any opinion. -- Thanks, Byungchul ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up 2018-06-01 3:07 ` Byungchul Park @ 2018-06-01 6:02 ` Joel Fernandes 2018-06-01 6:30 ` Byungchul Park 0 siblings, 1 reply; 17+ messages in thread From: Joel Fernandes @ 2018-06-01 6:02 UTC (permalink / raw) To: Byungchul Park Cc: Peter Zijlstra, mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team On Fri, Jun 01, 2018 at 12:07:48PM +0900, Byungchul Park wrote: > > > On 2018-05-25 14:13, Byungchul Park wrote: > > > > > > On 2018-05-09 15:33, Byungchul Park wrote: > > > On Thu, Jan 11, 2018 at 10:07:16AM +0100, Peter Zijlstra wrote: > > > > > > > > > > > > Sorry for the huge delay on this, but I'll have to postpone further. > > > > Still busy with meltdown/spectre stuff. > > > > > > Please consider this. Even though it's not a big bug, anyway leading > > > mis-behavior in certain situaions. > > > > Could you see this patches, it's been too long since the start tho? > > Please, any opinion. Just my opinion: this patch [1] is just a cosmetic change. I would argue that there's no readability improvement by wrapping up elements[0].dl. Infact I even feel that the elements[0].cpu should directly be accessed since both .cpu and .dl for the 0th element are directly accessed only from one place (cpudl_find) and only one time, and explicitly accessing index 0 makes it more clear that this is the root of the max-heap. IOW I don't see any benefit in hiding it behind a wrapper which hides the fact that we're accessing the root of the max heap, but I don't terribly hate this patch and I'm Ok if maintainers and other reviewers think its worth it. thanks, - Joel [1] https://patchwork.kernel.org/patch/10149099/ ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up 2018-06-01 6:02 ` Joel Fernandes @ 2018-06-01 6:30 ` Byungchul Park 0 siblings, 0 replies; 17+ messages in thread From: Byungchul Park @ 2018-06-01 6:30 UTC (permalink / raw) To: Joel Fernandes Cc: Peter Zijlstra, mingo, rostedt, tglx, raistlin, linux-kernel, juri.lelli, bristot, kernel-team On 2018-06-01 15:02, Joel Fernandes wrote: > On Fri, Jun 01, 2018 at 12:07:48PM +0900, Byungchul Park wrote: >> >> >> On 2018-05-25 14:13, Byungchul Park wrote: >>> >>> >>> On 2018-05-09 15:33, Byungchul Park wrote: >>>> On Thu, Jan 11, 2018 at 10:07:16AM +0100, Peter Zijlstra wrote: >>>>> >>>>> >>>>> Sorry for the huge delay on this, but I'll have to postpone further. >>>>> Still busy with meltdown/spectre stuff. >>>> >>>> Please consider this. Even though it's not a big bug, anyway leading >>>> mis-behavior in certain situaions. >>> >>> Could you see this patches, it's been too long since the start tho? >> >> Please, any opinion. > > Just my opinion: this patch [1] is just a cosmetic change. I would argue that > there's no readability improvement by wrapping up elements[0].dl. Infact I > even feel that the elements[0].cpu should directly be accessed since both > .cpu and .dl for the 0th element are directly accessed only from one place > (cpudl_find) and only one time, and explicitly accessing index 0 makes it > more clear that this is the root of the max-heap. > > IOW I don't see any benefit in hiding it behind a wrapper which hides the > fact that we're accessing the root of the max heap, but I don't terribly hate > this patch and I'm Ok if maintainers and other reviewers think its worth it. Hi Joel, Talking about the *1st patch*, no matter whether denied or not, even though I think it looks weird to abstract only p->elements[0].cpu with a function, but not cp->elements[0].dl. > thanks, > > - Joel > > [1] https://patchwork.kernel.org/patch/10149099/ > > -- Thanks, Byungchul ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-06-03 5:20 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-08 6:14 [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up Byungchul Park 2018-01-08 6:14 ` [RESEND PATCH v3 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value Byungchul Park 2018-06-01 6:12 ` [RESEND, v3, " Joel Fernandes 2018-06-01 6:18 ` Joel Fernandes 2018-06-01 7:10 ` Byungchul Park 2018-06-01 15:52 ` Joel Fernandes 2018-06-03 5:20 ` Byungchul Park 2018-01-11 9:07 ` [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up Peter Zijlstra 2018-02-09 0:37 ` Byungchul Park 2018-02-26 7:51 ` Byungchul Park 2018-03-13 5:52 ` Byungchul Park 2018-04-23 7:01 ` Byungchul Park 2018-05-09 6:33 ` Byungchul Park 2018-05-25 5:13 ` Byungchul Park 2018-06-01 3:07 ` Byungchul Park 2018-06-01 6:02 ` Joel Fernandes 2018-06-01 6:30 ` Byungchul Park
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).