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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.11.15 at 11:16, <George.Dunlap@xxxxxxxxxxxxx> wrote:
> On Fri, Oct 30, 2015 at 5:00 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> On 30.10.15 at 17:33, <dario.faggioli@xxxxxxxxxx> wrote:
>>> On Fri, 2015-10-30 at 10:25 -0600, Jan Beulich wrote:
>>>> > > > On 30.10.15 at 16:09, <write.harmandeep@xxxxxxxxx> 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



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