Re: [Xen-devel] [RFC PATCH 1/4] Implement cbs algorithm, remove extra queues, latency scaling, and weight support from sedf

[Joshua Whitehead <josh.whitehead@xxxxxxxxxxxxxxx>]

On 6/17/2014 11:43 AM, Dario Faggioli wrote:
> Again about patch splitting.
> 
> In this patch you are killing a lot of things, which I agree about
> killing. But then you also add what's required to implement the CBS
> algorithm. That is really really really really hard to review.
> 
> It's probably not possible to come down to a patch with only '-', but
> still I think this patch should at least be split in two. :-)
> 
> In one (the first) you remove all the stuff we won't to be part of the
> algorithm any longer, like the extraqueue, the weight, the latency, that
> super-ugly thing it does at vcpu wakeup time, etc.
> 
> In the other (the second) you implement the CBS algorithm on top of
> what's remaining.
> 
> While doing this, be careful that, in order to avoid bisectability of
> the tree, the code base must always compile, even with only the first
> patch, in the above example. Given how OSSTEST (and it's bisector)
> works, I think it's best if you can confirm that, whatever patch you've
> got on top, the system boots.
> 
This is good to know- we actually have something very much along these lines in
our local repos.  We have a separate branch containing only the removal of the
extraneous bits of SEDF which does compile, and is what we used to implement CBS
on in the first place.  At some point we chose to combine the two operations for
the patch submission and in retrospect I'm not sure why.  So we should be able
to add an additional patch to the series (essentially splitting this patch) in
V2 that does as you mentioned above- separating the removal of parts from SEDF
and the implementation of CBS.

> On ven, 2014-06-13 at 15:58 -0400, Josh Whitehead wrote:
>> ---
>>
> So, empty changelog.
> 
> I guess this is because it's an RFC, so, yeah, no big deal, especially
> considering you really did a great job in the cover letter.
> 
> However, I really recommend to put something up there, even for very
> early revisions.
> 
> Oh, BTW, even if you really don't want to put any description or
> changelog of any sort (which you really should, though), we at least
> need the 'Signed-off-by:' thing.
> 
I apologize for this, I actually had a small entry on each of these patches as
well as the "Signed-off-by:" lines, but in getting the patch ready to send out I
must have forgotten to put them in after my last "format-patch".  This will be
corrected in V2.

>> diff --git a/xen/common/sched_sedf.c b/xen/common/sched_sedf.c
>> index 0c9011a..2ee4538 100644
>> --- a/xen/common/sched_sedf.c
>> +++ b/xen/common/sched_sedf.c
> 
>> @@ -58,24 +50,14 @@ struct sedf_priv_info {
>>  struct sedf_vcpu_info {
>>      struct vcpu *vcpu;
>>      struct list_head list;
>> -    struct list_head extralist[2];
>>   
>>      /* Parameters for EDF */
>>      s_time_t  period;  /* = relative deadline */
>>      s_time_t  slice;   /* = worst case execution time */
>> - 
>> -    /* Advaced Parameters */
>> +    /* Note: Server bandwidth = (slice / period) */
>>  
>> -    /* Latency Scaling */
>> -    s_time_t  period_orig;
>> -    s_time_t  slice_orig;
>> -    s_time_t  latency;
>> - 
>>      /* Status of domain */
>>      int       status;
>> -    /* Weights for "Scheduling for beginners/ lazy/ etc." ;) */
>> -    short     weight;
>> -    short     extraweight;
>>
> About these (so weight and related, latency and related), this is where
> I, _FOR_NOW_, would put a bunch of /* TODOs */, with some explanation of
> what's going on.
> 
> If it were me doing this, I would, most likely:
>  1. leave the parameters alone here
>  2. temporarily kill any handling and usage of them in the rest of the  
>     file
>  3. stick a TODO right here explaining 2.
>  4. once the support and handling of the parameters will be back, come 
>     back here, and remove the TODO
> 
> Of course, 4 depends on what we decide to do with 'weight' and
> 'latency'... which is one more reason to keep this in stand-by, but make
> sure this does not stop further development.
> 
> Also, I'd say the series should remain an RFC, until the TODOs are gone.
> 
> As I said, it's very hard to review the patch like this... Let me know
> if you agree in splitting it, in which case, I'd rather have a look at
> that version when it'll be ready.
> 
Yes I think splitting (as well as some of the other reorganization mentioned
elsewhere) would be best and make it easier to review.  Some of the above would
also depend on how we decided to handle the input from George as it sounds like
he doesn't feel it would cause much of an issue to change the parameters around.
 I can't promise anything as we have other projects going on concurrently, but
maybe we'll shoot to have a reorganized and cleaned up V2 out by next week.
Thanks again for the feedback, this has been helpful in solidifying our goals 
here.

- Josh Whitehead

> Regards,
> Dario
> 


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