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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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.

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.

> 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.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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