[RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up

[RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up

[Byungchul Park]

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

[RESEND PATCH v3 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value

[Byungchul Park]

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

Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up

[Peter Zijlstra]

Sorry for the huge delay on this, but I'll have to postpone further.
Still busy with meltdown/spectre stuff.

Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up

[Byungchul Park]

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

Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up

[Byungchul Park]

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

Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up

[Byungchul Park]

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

Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up

[Byungchul Park]

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

Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up

[Byungchul Park]

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.

Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up

[Byungchul Park]

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

Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up

[Byungchul Park]

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

Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up

[Joel Fernandes]

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/

Re: [RESEND, v3, 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value

[Joel Fernandes]

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

Re: [RESEND, v3, 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value

[Joel Fernandes]

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

Re: [RESEND PATCH v3 1/2] sched/deadline: Add cpudl_maximum_dl() for clean-up

[Byungchul Park]

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

Re: [RESEND, v3, 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value

[Byungchul Park]

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

Re: [RESEND, v3, 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value

[Joel Fernandes]

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

Re: [RESEND, v3, 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value

[Byungchul Park]

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

[RESEND PATCH v3 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value

[Byungchul Park]

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

[RESEND PATCH v3 2/2] sched/deadline: Initialize cp->elements[].cpu to an invalid value

[Byungchul Park]

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