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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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?

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

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