Re: [Xen-devel] [PATCH 04/16] xen: sched: implement .init_pdata in all schedulers

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 22/03/16 08:03, Juergen Gross wrote:
> On 18/03/16 20:04, Dario Faggioli wrote:
>> by borrowing some of the code of .alloc_pdata, i.e.,
>> the bits that perform initializations, leaving only
>> actual allocations in there, when any, which is the
>> case for Credit1 and RTDS.
>>
>> On the other hand, in Credit2, since we don't really
>> need any per-pCPU data allocation, everything that was
>> being done in .alloc_pdata, is now done in .init_pdata.
>> And the fact that now .alloc_pdata can be left undefined,
>> allows us to just get rid of it.
>>
>> Still for Credit2, the fact that .init_pdata is called
>> during CPU_STARTING (rather than CPU_UP_PREPARE) kills
>> the need for the scheduler to setup a similar callback
>> itself, simplifying the code.
>>
>> And thanks to such simplification, it is now also ok to
>> move some of the logic meant at double checking that a
>> cpu was (or was not) initialized, into ASSERTS (rather
>> than an if() and a BUG_ON).
>>
>> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
>> ---
>> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>> Cc: Meng Xu <mengxu@xxxxxxxxxxxxx>
>> Cc: Juergen Gross <jgross@xxxxxxxx>
>> ---
>>  xen/common/sched_credit.c  |   20 +++++++++---
>>  xen/common/sched_credit2.c |   72 
>> +++-----------------------------------------
>>  xen/common/sched_rt.c      |    9 ++++--
>>  3 files changed, 26 insertions(+), 75 deletions(-)
>>
>> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
>> index 288749f..d4a0f5e 100644
>> --- a/xen/common/sched_credit.c
>> +++ b/xen/common/sched_credit.c
>> @@ -526,8 +526,6 @@ static void *
>>  csched_alloc_pdata(const struct scheduler *ops, int cpu)
>>  {
>>      struct csched_pcpu *spc;
>> -    struct csched_private *prv = CSCHED_PRIV(ops);
>> -    unsigned long flags;
>>  
>>      /* Allocate per-PCPU info */
>>      spc = xzalloc(struct csched_pcpu);
>> @@ -540,6 +538,19 @@ csched_alloc_pdata(const struct scheduler *ops, int cpu)
>>          return ERR_PTR(-ENOMEM);
>>      }
>>  
>> +    return spc;
>> +}
>> +
>> +static void
>> +csched_init_pdata(const struct scheduler *ops, void *pdata, int cpu)
>> +{
>> +    struct csched_private *prv = CSCHED_PRIV(ops);
>> +    struct csched_pcpu * const spc = pdata;
>> +    unsigned long flags;
>> +
>> +    /* cpu data needs to be allocated, but STILL uninitialized */
>> +    ASSERT(spc && spc->runq.next == spc->runq.prev && spc->runq.next == 
>> NULL);
> 
> This looks weird. I'd prefer:
> 
> ASSERT(spc && spc->runq.next == NULL && spc->runq.prev == NULL);

I prefer Juergen's suggestion too.  I wouldn't say it's worth respinning
over, but since you have to make adjustments to the previous patch
anyway, you might as well change this while you're at it.

With that change:

Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>


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