sched_credit: Remove cpu argument to __runq_insert()

PATCH In review Harmandeep Kaur 5 participants 12 messages Oct 30, 2015 – Nov 3, 2015

Share & tools

Harmandeep Kaur

author

2015-10-30 15:11 UTC #0 lore raw

PATCH 1/1 sched_credit: Remove cpu argument to __runq_insert()

__runq_insert() takes two arguments, cpu and svc. However, the cpu argument is redundant because we can get all the information we need about cpu from svc.

Signed-off-by Harmandeep Kaur <[email protected]>

---
 xen/common/sched_credit.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

xen/common/sched_credit.c +5 -6

index b8f28fe..5d73706 100644
@@ -252,13 +252,12 @@ __runq_elem(struct list_head *elem)
 }
 
 static inline void
-__runq_insert(unsigned int cpu, struct csched_vcpu *svc)
+__runq_insert(struct csched_vcpu *svc)
 {
-    const struct list_head * const runq = RUNQ(cpu);
+    const struct list_head * const runq = RUNQ(svc->vcpu->processor);
     struct list_head *iter;
 
     BUG_ON( __vcpu_on_runq(svc) );
-    BUG_ON( cpu != svc->vcpu->processor );
 
     list_for_each( iter, runq )
     {

@@ -905,7 +904,7 @@ csched_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
     struct csched_vcpu *svc = vc->sched_priv;
 
     if ( !__vcpu_on_runq(svc) && vcpu_runnable(vc) && !vc->is_running )
-        __runq_insert(vc->processor, svc);
+        __runq_insert(svc);
 
     SCHED_STAT_CRANK(vcpu_insert);
 }

@@ -1016,7 +1015,7 @@ csched_vcpu_wake(const struct scheduler *ops, struct vcpu *vc)
     }
 
     /* Put the VCPU on the runq and tickle CPUs */
-    __runq_insert(cpu, svc);
+    __runq_insert(svc);
     __runq_tickle(cpu, svc);
 }

@@ -1681,7 +1680,7 @@ csched_schedule(
      * Select next runnable local VCPU (ie top of local runq)
      */
     if ( vcpu_runnable(current) )
-        __runq_insert(cpu, scurr);
+        __runq_insert(scurr);
     else
         BUG_ON( is_idle_vcpu(current) || list_empty(runq) );

Signature · 1 line

1.9.1

Jan Beulich

2015-10-30 16:25 UTC #1 lore raw

Quoted 4 lines

Quoted 1 line
Quoted 1 line · On 30.10.15 at 16:09, <[email protected]> wrote:
__runq_insert() takes two arguments, cpu and svc. However, the cpu argument is redundant because we can get all the information we need about cpu from svc.

While true and looking like an improvement at the source level, ...

xen/common/sched_credit.c __runq_elem(struct list_head *elem) · line 257

Quoted 11 lines

xen/common/sched_credit.c +2 -2

@@ -252,13 +252,12 @@ __runq_elem(struct list_head *elem)
 }
 
 static inline void
-__runq_insert(unsigned int cpu, struct csched_vcpu *svc)
+__runq_insert(struct csched_vcpu *svc)
 {
-    const struct list_head * const runq = RUNQ(cpu);
+    const struct list_head * const runq = RUNQ(svc->vcpu->processor);

... this being an inline function the change will likely make the compiler produce worse code, if only ...

xen/common/sched_credit.c __runq_elem(struct list_head *elem) · line 261

Quoted 4 lines

xen/common/sched_credit.c +0 -1

     struct list_head *iter;
 
     BUG_ON( __vcpu_on_runq(svc) );
-    BUG_ON( cpu != svc->vcpu->processor );

... this was an ASSERT() instead of a BUG_ON() (which it looks like it should be).

Jan

Dario Faggioli

2015-10-30 16:33 UTC #2 lore raw

Quoted 18 lines · Jan Beulich

Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 30.10.15 at 16:09, <[email protected]> wrote:
xen/common/sched_credit.c __runq_elem(struct list_head *elem) · line 258
Quoted 9 lines
xen/common/sched_credit.c +2 -3
@@ -252,13 +252,12 @@ __runq_elem(struct list_head *elem)
 }
 
 static inline void
-__runq_insert(unsigned int cpu, struct csched_vcpu *svc)
+__runq_insert(struct csched_vcpu *svc)
 {
-    const struct list_head * const runq = RUNQ(cpu);
+    const struct list_head * const runq = RUNQ(svc->vcpu
->processor);
... this being an inline function the change will likely make the compiler produce worse code, if only ...

Quoted 8 lines

xen/common/sched_credit.c __runq_elem(struct list_head *elem) · line 261
Quoted 4 lines
xen/common/sched_credit.c +0 -1
     struct list_head *iter;
 
     BUG_ON( __vcpu_on_runq(svc) );
-    BUG_ON( cpu != svc->vcpu->processor );
... this was an ASSERT() instead of a BUG_ON() (which it looks like it should be).

Mmm... I'm sorry, but I'm not getting what you are actually suggesting.

Are you saying that we shouldn't make the change at all? Or that we should make the change and also turn the BUG_ON() (the one that is left in place) into an ASSERT()? Or that we should not mark the function as 'inline'?

Harman, perhaps, can you check the assembly code produced by the compiler before and after your patch and report here what the differences are?

Thanks and Regards,
Dario

Signature · 4 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Jan Beulich

2015-10-30 17:00 UTC #3 lore raw

Quoted 27 lines

Quoted 1 line
Quoted 1 line · On 30.10.15 at 17:33, <[email protected]> wrote:
Quoted 17 lines · Jan Beulich
xen/common/sched_credit.c __runq_elem(struct list_head *elem) · line 258
Quoted 13 lines
Quoted 1 line
Quoted 1 line · On 30.10.15 at 16:09, <[email protected]> wrote:
xen/common/sched_credit.c +2 -3
@@ -252,13 +252,12 @@ __runq_elem(struct list_head *elem)
 }
 
 static inline void
-__runq_insert(unsigned int cpu, struct csched_vcpu *svc)
+__runq_insert(struct csched_vcpu *svc)
 {
-    const struct list_head * const runq = RUNQ(cpu);
+    const struct list_head * const runq = RUNQ(svc->vcpu
->processor);
... this being an inline function the change will likely make the compiler produce worse code, if only ...
Quoted 8 lines
xen/common/sched_credit.c __runq_elem(struct list_head *elem) · line 261
Quoted 4 lines
xen/common/sched_credit.c +0 -1
     struct list_head *iter;
 
     BUG_ON( __vcpu_on_runq(svc) );
-    BUG_ON( cpu != svc->vcpu->processor );
... this was an ASSERT() instead of a BUG_ON() (which it looks like it should be).

Quoted 6 lines

Mmm... I'm sorry, but I'm not getting what you are actually suggesting.
Are you saying that we shouldn't make the change at all? Or that we should make the change and also turn the BUG_ON() (the one that is left in place) into an ASSERT()? Or that we should not mark the function as 'inline'?

No, I'm suggesting that instead of this change the BUG_ON() (or perhaps both and also others) should be converted to ASSERT().

Jan

Dario Faggioli

2015-11-02 11:02 UTC #4 lore raw

[Trimming the Cc list, and adding George, which I did not realize he
was not there!]

Quoted 9 lines · Jan Beulich

Quoted 8 lines
Quoted 1 line
Quoted 1 line · On 30.10.15 at 17:33, <[email protected]> wrote:
Are you saying that we shouldn't make the change at all? Or that we should make the change and also turn the BUG_ON() (the one that is left
in place) into an ASSERT()? Or that we should not mark the function as
'inline'?

Quoted 3 lines

No, I'm suggesting that instead of this change the BUG_ON() (or perhaps both and also others) should be converted to ASSERT().

I'm all in favour of turning these two (if both are staying) BUG_ON()s in ASSERT()s.

What I especially don't like of the function right now, is that by looking at the prototype, and at least at some of the call sites, it feels like it is possible to insert a vCPU v in the runqueue of an arbitrary pCPU, while that must always happens in v->processor's runqueue. That is why I'd like to be able to remove that cpu argument.

Perhaps avoiding inlining can really be considered? Looking at history, the function was shorter and called less times when originally developed and marked as such. I'm not sure that's still a good thing...

Regards,
Dario

Signature · 4 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

George Dunlap

2015-11-03 10:35 UTC #5 lore raw

In reply to Jan Beulich

Quoted 29 lines · Jan Beulich

Quoted 29 lines
Quoted 1 line
Quoted 1 line · On 30.10.15 at 17:33, <[email protected]> wrote:
Quoted 17 lines · Jan Beulich
xen/common/sched_credit.c __runq_elem(struct list_head *elem) · line 258
Quoted 13 lines
Quoted 1 line
Quoted 1 line · On 30.10.15 at 16:09, <[email protected]> wrote:
xen/common/sched_credit.c +2 -3
@@ -252,13 +252,12 @@ __runq_elem(struct list_head *elem)
 }
 
 static inline void
-__runq_insert(unsigned int cpu, struct csched_vcpu *svc)
+__runq_insert(struct csched_vcpu *svc)
 {
-    const struct list_head * const runq = RUNQ(cpu);
+    const struct list_head * const runq = RUNQ(svc->vcpu
->processor);
... this being an inline function the change will likely make the compiler produce worse code, if only ...
Quoted 8 lines
xen/common/sched_credit.c __runq_elem(struct list_head *elem) · line 261
Quoted 4 lines
xen/common/sched_credit.c +0 -1
     struct list_head *iter;
 
     BUG_ON( __vcpu_on_runq(svc) );
-    BUG_ON( cpu != svc->vcpu->processor );
... this was an ASSERT() instead of a BUG_ON() (which it looks like it should be).
Mmm... I'm sorry, but I'm not getting what you are actually suggesting.

Quoted 7 lines

Quoted 4 lines
Are you saying that we shouldn't make the change at all? Or that we should make the change and also turn the BUG_ON() (the one that is left in place) into an ASSERT()? Or that we should not mark the function as 'inline'?
No, I'm suggesting that instead of this change the BUG_ON() (or perhaps both and also others) should be converted to ASSERT().

So you agree that this change makes the source code make more sense ("looks like an improvement at the source level"), but you think that this will make the compiled code less efficient; and you recommend instead of making the source code clearer, to make things even *better* by changing the BUG_ON() to an ASSERT?

Why do you think the compiler output will be less efficient?

Overall I think the burden of proof is on you to show that the code as it is introduces a sufficient performance improvement over the more readable code, rather than on Harmandeep (or Dario or I) to show that it doesn't.

-George

Jan Beulich

2015-11-03 12:38 UTC #6 lore raw

Quoted 39 lines

Quoted 1 line
Quoted 1 line · On 03.11.15 at 11:16, <[email protected]> wrote:
Quoted 29 lines · Jan Beulich
Quoted 29 lines
Quoted 1 line
Quoted 1 line · On 30.10.15 at 17:33, <[email protected]> wrote:
Quoted 17 lines · Jan Beulich
xen/common/sched_credit.c __runq_elem(struct list_head *elem) · line 258
Quoted 13 lines
Quoted 1 line
Quoted 1 line · On 30.10.15 at 16:09, <[email protected]> wrote:
xen/common/sched_credit.c +2 -3
@@ -252,13 +252,12 @@ __runq_elem(struct list_head *elem)
 }
 
 static inline void
-__runq_insert(unsigned int cpu, struct csched_vcpu *svc)
+__runq_insert(struct csched_vcpu *svc)
 {
-    const struct list_head * const runq = RUNQ(cpu);
+    const struct list_head * const runq = RUNQ(svc->vcpu
->processor);
... this being an inline function the change will likely make the compiler produce worse code, if only ...
Quoted 8 lines
xen/common/sched_credit.c __runq_elem(struct list_head *elem) · line 261
Quoted 4 lines
xen/common/sched_credit.c +0 -1
     struct list_head *iter;
 
     BUG_ON( __vcpu_on_runq(svc) );
-    BUG_ON( cpu != svc->vcpu->processor );
... this was an ASSERT() instead of a BUG_ON() (which it looks like it should be).
Mmm... I'm sorry, but I'm not getting what you are actually suggesting.
Quoted 7 lines
Quoted 4 lines
Are you saying that we shouldn't make the change at all? Or that we should make the change and also turn the BUG_ON() (the one that is left in place) into an ASSERT()? Or that we should not mark the function as 'inline'?
No, I'm suggesting that instead of this change the BUG_ON() (or perhaps both and also others) should be converted to ASSERT().

Quoted 7 lines

So you agree that this change makes the source code make more sense ("looks like an improvement at the source level"), but you think that this will make the compiled code less efficient; and you recommend instead of making the source code clearer, to make things even *better* by changing the BUG_ON() to an ASSERT?
Why do you think the compiler output will be less efficient?

Due to the two extra memory references, which the compiler is unlikely to be able to eliminate in all cases?.

Quoted 4 lines

Overall I think the burden of proof is on you to show that the code as it is introduces a sufficient performance improvement over the more readable code, rather than on Harmandeep (or Dario or I) to show that it doesn't.

Okay, so far I thought people suggesting a change are the ones to prove that the change doesn't have undesirable effects. If it's the other way around, I don't think I'm bothered enough to provide any proof. You're the maintainer of the code anyway, so if you think this is fine to go, so be it.

Jan

Dario Faggioli

Acked-by

2015-11-03 21:22 UTC #7 lore raw

Quoted 9 lines · Jan Beulich

Quoted 9 lines
Quoted 1 line
Quoted 1 line · On 03.11.15 at 11:16, <[email protected]> wrote:
So you agree that this change makes the source code make more sense ("looks like an improvement at the source level"), but you think that
this will make the compiled code less efficient; and you recommend instead of making the source code clearer, to make things even *better* by changing the BUG_ON() to an ASSERT?

Quoted 5 lines

Quoted 1 line
Why do you think the compiler output will be less efficient?
Due to the two extra memory references, which the compiler is unlikely to be able to eliminate in all cases?.

Right. I had a quick look at the assembly code, and I think I saw something like that. As far as I've seen, though, the text sections of the generated binaries --with and without this patch-- were equally big (due to alignment, I think).

Also, for the reasons explained here:
http://lists.xen.org/archives/html/xen-devel/2015-11/msg00051.html

As far as I'm concerned, this patch is:

Acked-by Dario Faggioli <[email protected]>

That being said...

Quoted 10 lines

Quoted 6 lines
Overall I think the burden of proof is on you to show that the code as
it is introduces a sufficient performance improvement over the more readable code, rather than on Harmandeep (or Dario or I) to show that
it doesn't.
Okay, so far I thought people suggesting a change are the ones to prove that the change doesn't have undesirable effects.

...just FTY, I probably will try having a look at what it means to make __runq_insert() non-inline. But that's another patch. :-)

Thanks and Regards,
Dario

Signature · 4 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Dario Faggioli

2015-10-30 16:47 UTC #8 lore raw

In reply to Harmandeep Kaur

Quoted 6 lines · Harmandeep Kaur

__runq_insert() takes two arguments, cpu and svc. However, the cpu argument is redundant because we can get all the information we need about cpu from svc.
Signed-off-by Harmandeep Kaur <[email protected]>

Thanks Harman, this looks good. Just one thing.

The list of people you Cc-ed, is not really accurate. In fact, Ian Campbell and Ian Jackson are toolstack maintainers, and even Jan, Keir and Tim, although they are hypervisors maintainers, should not need to be bothered by a patch touching only sched_credit.c.

Have a look at the MAINTAINERS file and at scripts/get_maintainer.pl.

Thanks and Regards,
Dario

Signature · 4 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Harmandeep Kaur

author

2015-10-30 16:50 UTC #9 lore raw

Quoted 7 lines · Dario Faggioli

Quoted 6 lines · Harmandeep Kaur
__runq_insert() takes two arguments, cpu and svc. However, the cpu argument is redundant because we can get all the information we need about cpu from svc.
Signed-off-by Harmandeep Kaur <[email protected]>

Quoted 9 lines

Thanks Harman, this looks good. Just one thing.
The list of people you Cc-ed, is not really accurate. In fact, Ian Campbell and Ian Jackson are toolstack maintainers, and even Jan, Keir and Tim, although they are hypervisors maintainers, should not need to be bothered by a patch touching only sched_credit.c.
Have a look at the MAINTAINERS file and at scripts/get_maintainer.pl.

I did ./scripts/get_maintainer.pl -f xen/common/. Anyways will do better next
time :)

Quoted 9 lines

Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Dario Faggioli

2015-10-30 17:01 UTC #10 lore raw

Quoted 9 lines · Harmandeep Kaur

Quoted 7 lines · Dario Faggioli
The list of people you Cc-ed, is not really accurate. In fact, Ian Campbell and Ian Jackson are toolstack maintainers, and even Jan, Keir
and Tim, although they are hypervisors maintainers, should not need to
be bothered by a patch touching only sched_credit.c.

Quoted 5 lines

Quoted 2 lines
Have a look at the MAINTAINERS file and at
scripts/get_maintainer.pl.
I did ./scripts/get_maintainer.pl -f xen/common/.

Why the "-f xen/common/." ? I've never used it like that, so I don't know what output should have been expected from it.

What I usually do is `cat <patch> | ./scripts/get_maintainer.pl', and most of the time the answer I get makes sense.

Regards,
Dario

Signature · 4 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Wei Liu

2015-11-02 12:36 UTC #11 lore raw

Quoted 10 lines · Dario Faggioli

Quoted 9 lines · Harmandeep Kaur
Quoted 7 lines · Dario Faggioli
The list of people you Cc-ed, is not really accurate. In fact, Ian Campbell and Ian Jackson are toolstack maintainers, and even Jan, Keir
and Tim, although they are hypervisors maintainers, should not need to
be bothered by a patch touching only sched_credit.c.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Have a look at the MAINTAINERS file and at
scripts/get_maintainer.pl.
I did ./scripts/get_maintainer.pl -f xen/common/.
Why the "-f xen/common/." ? I've never used it like that, so I don't know what output should have been expected from it.

That's useful when you want to derive list of maintainers from a file.

The actual rune Harmandeep needs should be

./scripts/get_maintainer.pl -f xen/common/sched_credit.c

Using "." makes that script point to "The rest" maintainers. There is a note in the help string of that script:

Notes:
Using "-f directory" may give unexpected results ...

Wei.

Quoted 18 lines

What I usually do is `cat <patch> | ./scripts/get_maintainer.pl', and most of the time the answer I get makes sense.

Regards,
Dario

Signature · 12 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)




_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel