Re: [Xen-devel] [PATCH RFC 08/49] xen/sched: use new sched_item instead of vcpu in scheduler interfaces

[Juergen Gross <jgross@xxxxxxxx>]

On 30/03/2019 11:24, Juergen Gross wrote:
> On 29/03/2019 19:42, Andrew Cooper wrote:
>> On 29/03/2019 15:08, Juergen Gross wrote:
>>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>>> index 6b5d454630..d1a958143a 100644
>>> --- a/xen/common/schedule.c
>>> +++ b/xen/common/schedule.c
>>> @@ -256,6 +256,7 @@ static void sched_spin_unlock_double(spinlock_t *lock1, 
>>> spinlock_t *lock2,
>>>  int sched_init_vcpu(struct vcpu *v, unsigned int processor)
>>>  {
>>>      struct domain *d = v->domain;
>>> +    struct sched_item item = { .vcpu = v };
>>>  
>>>      v->processor = processor;
>>>  
>>> @@ -267,7 +268,7 @@ int sched_init_vcpu(struct vcpu *v, unsigned int 
>>> processor)
>>>      init_timer(&v->poll_timer, poll_timer_fn,
>>>                 v, v->processor);
>>>  
>>> -    v->sched_priv = SCHED_OP(dom_scheduler(d), alloc_vdata, v,
>>> +    v->sched_priv = SCHED_OP(dom_scheduler(d), alloc_vdata, &item,
>>>                       d->sched_priv);
>>
>> I realise this is perhaps an over-the-top request, but can we see about
>> doing more here?
>>
>> SCHED_OP() is a thoroughly objectionable piece of obfuscation, which
>> breaks cscope/ctags and also results in especially poor code generation.
>>
>> Given that we are changing the interface anyway and touching all
>> codepaths, would you mind also adding static inline wrappers like I
>> started with 340edc3 ?
> 
> Okay, I'll do that.
> 
>> TBH, I'm even happy to give this a go and give you the back the
>> resulting tree, if you'd prefer.
> 
> I think its is easier to do it myself, as I'm touching nearly all of
> the call sites anyway.

And another thought I had: with RETPOLINE indirect jumps are even more
expensive. Would it be a good idea to remove the function pointers from
struct scheduler and generate the inline wrappers at build time? The
wrappers could then call the related specific scheduler function based
on the scheduler Id using a chain of if ... else if ... statements. It
would prefer the default scheduler over the others and test only for
configured schedulers. Scheduler registration could be done the same way
removing the need for an extra link section.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel