Re: [PATCH] xen:rtds:Clear __RTDS_depleted bit once replenish vcpu

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3758 bytes --]

On Fri, 2016-10-21 at 22:12 -0400, Meng Xu wrote:
> We should clear the __RTDS_depleted bit once a VCPU budget is
> replenished.
> Because repl_timer_handler may be called after rt_schedule
> but before rt_context_saved, the VCPU may be not on CPU or on queue
> when the VCPU is the middle of context switch
>
Yes, this makes sense, and is a good fix.

I actually would prefer the subject to become (something like):

"xen: rtds: always clear the flag when replenishing a depleted vcpu"

whith that changed:

> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
>
Acked-by: Dario Faggioli <dario.faggioli@citrix.com>

This being said, I hope you see the value of having split the patches
--especially patches like this-- in such a way that each one deals with
one specific issue.

It doesn't matter if they end up being very small, in terms of changed
lines. In fact, the behavioral issue being dealt with in here is rather
subtle, and most important "self-contained", i.e., independent from any
other issue that we may or may not have already found.

Also, consider that, if it turns out that this patch is wrong, i.e., it
does not really fix the problem and/or introduce other ones, we will be
able to revert it. If the hunk below was part of a more complex patch,
that would have been both more difficult and less neat.

Another example:

> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -1488,8 +1488,8 @@ static void repl_timer_handler(void *data){
>              if ( svc->cur_deadline > next_on_runq->cur_deadline )
>                  runq_tickle(ops, next_on_runq);
>          }
> -        else if ( vcpu_on_q(svc) &&
> -                  __test_and_clear_bit(__RTDS_depleted, &svc->flags) 
> )
> +        else if ( __test_and_clear_bit(__RTDS_depleted, &svc->flags) 
> &&
> +                  vcpu_on_q(svc) )
>              runq_tickle(ops, svc);
>  
The previous patch was dropping the 'else', the reason for which was
not really easy to see to me. I went looking at the commit message, but
it was not really clear itself, and actually contained the description
of two problems and two fixes! :-O

So, again, for things like these, 1 issue ==> 1 patch.

About the other patch, I think I understand the situation, but I don't
like the fix much. I'm looking into it and thinking what could be an
alternative solution. I'll let you know.

And finally, independently from these patches, I was thinking whether
it would not be worth dealing with the case budget==period as a special
case. I mean, if that is the case, there's no need to program the
timer, etc, we could just identify the situation and act accordingly
(e.g., in burn_budget()).

This is just an idea, though, based on the fact that people may want to
set budget==period for some domain/vcpu (like the "special" ones, dom0
or driver domains), and if there are a few of them, we avoid the
overhead of timers firing very very close to re-scheduling points,
etc. 
On the other hand, special cases make the code uglier, and often harder
to follow. So I'm saying it may (in my opinion) be worth trying to
write a patch, but it's not guaranteed that we actually want to take
it... It's just hard to tell, before actually seeing the code.

This is, of course, just my idea, with which you can well disagree. :-)

Thanks and 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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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