[PATCH] sched_credit: Remove cpu argument to __runq_insert()

[PATCH] sched_credit: Remove cpu argument to __runq_insert()

[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 <write.harmandeep@gmail.com>
---
 xen/common/sched_credit.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index b8f28fe..5d73706 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -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) );
 
-- 
1.9.1

Re: [PATCH] sched_credit: Remove cpu argument to __runq_insert()

[Jan Beulich]

>>> On 30.10.15 at 16:09, <write.harmandeep@gmail.com> 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, ...

> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -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 ...

>      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

Re: [PATCH] sched_credit: Remove cpu argument to __runq_insert()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1648 bytes --]

On Fri, 2015-10-30 at 10:25 -0600, Jan Beulich wrote:
> > > > On 30.10.15 at 16:09, <write.harmandeep@gmail.com> wrote:

> > --- a/xen/common/sched_credit.c
> > +++ b/xen/common/sched_credit.c
> > @@ -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 ...
> 
> >      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
-- 
<<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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] sched_credit: Remove cpu argument to __runq_insert()

[Jan Beulich]

>>> On 30.10.15 at 17:33, <dario.faggioli@citrix.com> wrote:
> On Fri, 2015-10-30 at 10:25 -0600, Jan Beulich wrote:
>> > > > On 30.10.15 at 16:09, <write.harmandeep@gmail.com> wrote:
>> > --- a/xen/common/sched_credit.c
>> > +++ b/xen/common/sched_credit.c
>> > @@ -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 ...
>> 
>> >      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'?

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

Jan

Re: [PATCH] sched_credit: Remove cpu argument to __runq_insert()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1532 bytes --]

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

On Fri, 2015-10-30 at 11:00 -0600, Jan Beulich wrote:
> > > > On 30.10.15 at 17:33, <dario.faggioli@citrix.com> 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'?
> 
> 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
-- 
<<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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] sched_credit: Remove cpu argument to __runq_insert()

[George Dunlap]

On Fri, Oct 30, 2015 at 5:00 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 30.10.15 at 17:33, <dario.faggioli@citrix.com> wrote:
>> On Fri, 2015-10-30 at 10:25 -0600, Jan Beulich wrote:
>>> > > > On 30.10.15 at 16:09, <write.harmandeep@gmail.com> wrote:
>>> > --- a/xen/common/sched_credit.c
>>> > +++ b/xen/common/sched_credit.c
>>> > @@ -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 ...
>>>
>>> >      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'?
>
> 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

Re: [PATCH] sched_credit: Remove cpu argument to __runq_insert()

[Jan Beulich]

>>> On 03.11.15 at 11:16, <George.Dunlap@eu.citrix.com> wrote:
> On Fri, Oct 30, 2015 at 5:00 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 30.10.15 at 17:33, <dario.faggioli@citrix.com> wrote:
>>> On Fri, 2015-10-30 at 10:25 -0600, Jan Beulich wrote:
>>>> > > > On 30.10.15 at 16:09, <write.harmandeep@gmail.com> wrote:
>>>> > --- a/xen/common/sched_credit.c
>>>> > +++ b/xen/common/sched_credit.c
>>>> > @@ -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 ...
>>>>
>>>> >      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'?
>>
>> 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?

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

> 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

Re: [PATCH] sched_credit: Remove cpu argument to __runq_insert()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1953 bytes --]

On Tue, 2015-11-03 at 05:38 -0700, Jan Beulich wrote:
> > > > On 03.11.15 at 11:16, <George.Dunlap@eu.citrix.com> 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?
> > 
> > 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 <dario.faggioli@citrix.com>

That being said...

> > 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
-- 
<<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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] sched_credit: Remove cpu argument to __runq_insert()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 976 bytes --]

On Fri, 2015-10-30 at 20:39 +0530, Harmandeep Kaur 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.
> 
> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
>
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
-- 
<<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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] sched_credit: Remove cpu argument to __runq_insert()

[Harmandeep Kaur]

[-- Attachment #1.1: Type: text/plain, Size: 1170 bytes --]

On Fri, Oct 30, 2015 at 10:16 PM, Dario Faggioli <dario.faggioli@citrix.com>
wrote:

> On Fri, 2015-10-30 at 20:39 +0530, Harmandeep Kaur 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.
> >
> > Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
> >
> 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 :)


> 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)
>
>

[-- Attachment #1.2: Type: text/html, Size: 2097 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] sched_credit: Remove cpu argument to __runq_insert()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1116 bytes --]

On Fri, 2015-10-30 at 22:20 +0530, Harmandeep Kaur wrote:
> On Fri, Oct 30, 2015 at 10:16 PM, Dario Faggioli <
> dario.faggioli@citrix.com> wrote:
> > 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/. 
>
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
-- 
<<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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] sched_credit: Remove cpu argument to __runq_insert()

[Wei Liu]

On Fri, Oct 30, 2015 at 06:01:36PM +0100, Dario Faggioli wrote:
> On Fri, 2015-10-30 at 22:20 +0530, Harmandeep Kaur wrote:
> > On Fri, Oct 30, 2015 at 10:16 PM, Dario Faggioli <
> > dario.faggioli@citrix.com> wrote:
> > > 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/. 
> >
> 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.

> What I usually do is `cat <patch> | ./scripts/get_maintainer.pl', and
> most of the time the answer I get makes sense.
> 
> 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
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel