Re: [Xen-devel] [PATCH RFC 17/49] xen/sched: move some per-vcpu items to struct sched_item

[Juergen Gross <jgross@xxxxxxxx>]

On 30/03/2019 10:59, Juergen Gross wrote:
> On 29/03/2019 22:33, Andrew Cooper wrote:
>> On 29/03/2019 15:09, Juergen Gross wrote:
>>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>>> index 8d579e2cf9..5d8f3255cb 100644
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -15,6 +15,7 @@
>>>  #include <xen/lib.h>
>>>  #include <xen/errno.h>
>>>  #include <xen/sched.h>
>>> +#include <xen/sched-if.h>
>>>  #include <xen/domain.h>
>>>  #include <xen/smp.h>
>>>  #include <xen/delay.h>
>>
>> I'm afraid that this feels like a step in the wrong direction.
>>
>> sched-if.h is (per the comments) supposed to be the schedulers
>> private.h, with the intention that struct scheduler didn't leak into the
>> rest of the codebase.  Also the logic for taking scheduler locks, etc,
>> and lastly for cpumask_scratch, which really is unsafe to use outside of
>> the scheduler (and has come up in several recent patch series).
>>
>> Sadly,
>>
>> andrewcoop@andrewcoop:/local/xen.git/xen$ git grep sched-if
>> arch/x86/acpi/cpu_idle.c:41:#include <xen/sched-if.h>
>> arch/x86/cpu/mcheck/mce.c:13:#include <xen/sched-if.h>
>> arch/x86/cpu/mcheck/mctelem.c:21:#include <xen/sched-if.h>
>> arch/x86/dom0_build.c:12:#include <xen/sched-if.h>
>> arch/x86/setup.c:6:#include <xen/sched-if.h>
>> arch/x86/smpboot.c:28:#include <xen/sched-if.h>
>> common/cpupool.c:19:#include <xen/sched-if.h>
>> common/domain.c:13:#include <xen/sched-if.h>
>> common/domctl.c:14:#include <xen/sched-if.h>
>> common/sched_arinc653.c:29:#include <xen/sched-if.h>
>> common/sched_credit.c:18:#include <xen/sched-if.h>
>> common/sched_credit2.c:21:#include <xen/sched-if.h>
>> common/sched_null.c:32:#include <xen/sched-if.h>
>> common/sched_rt.c:23:#include <xen/sched-if.h>
>> common/schedule.c:26:#include <xen/sched-if.h>
>> include/asm-x86/cpuidle.h:7:#include <xen/sched-if.h>
>>
>> and this change is making the situation worse.
>>
>> If at all possible, I'd prefer to see about disentangling the bits which
>> actually need external use, and putting them in sched.h, and making
>> sched-if.h properly private to the schedulers.  I actually even started
>> a cleanup series which moved all of the scheduler infrastructure into
>> common/sched/, but found a disappointing quantity of sched-if.h being
>> referenced externally.
> 
> I can add something like that to my series if you want. So:
> 
> - moving schedule.c, sched_*.c and cpupool.c to common/sched/
> - move stuff from sched-if.h to sched.h if needed outside of
>   common/sched/
> - move sched-if.h to common/sched/

Questions to especially the scheduler maintainers and "the REST": should
we move the scheduler stuff to xen/common/sched/ or would /xen/sched/ be
more appropriate?

Maybe it would be worthwhile to move e.g. the context switching from
xen/arch/*/domain.c to xen/sched/context_<arch>.c? I think this code is
rather scheduler related and moving it to the sched directory might help
hiding some scheduler internals from other sources, especially with my
core scheduling series. IMO this would make the xen/sched/ directory the
preferred one.


Juergen


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