Re: [Xen-devel] [PATCH for 4.7 4/4] xen: adopt .deinit_pdata and improve timer handling

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Sat, 2016-05-07 at 23:19 +0200, Meng Xu wrote:
> On Tue, May 3, 2016 at 5:46 PM, Dario Faggioli
> <dario.faggioli@xxxxxxxxxx> wrote:
> > 
> > 
> > The scheduling hooks API is now used properly, and no
> > initialization or de-initialization happen in
> > alloc/free_pdata any longer.
> > 
> > In fact, just like it is for Credit2, there is no real
> > need for implementing alloc_pdata and free_pdata.
> > 
> > This also made it possible to improve the replenishment
> > timer handling logic, such that now the timer is always
> > kept on one of the pCPU of the scheduler it's servicing.
> > Before this commit, in fact, even if the pCPU where the
> > timer happened to be initialized at creation time was
> > moved to another cpupool, the timer stayed there,
> > potentially inferfearing with the new scheduler of the
> > pCPU itself.
> > 
> > Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> > --
> Reviewed-and-Tested-by: Meng Xu <mengxu@xxxxxxxxxxxxx>
> 
Thanks! :-)

> I do have a minor comment about the patch, although it is not
> important at all and it is not really about this patch...
> 
> > 
> > @@ -614,7 +612,8 @@ rt_deinit(struct scheduler *ops)
> >  {
> >      struct rt_private *prv = rt_priv(ops);
> > 
> > -    kill_timer(prv->repl_timer);
> > +    ASSERT(prv->repl_timer->status == TIMER_STATUS_invalid ||
> > +           prv->repl_timer->status == TIMER_STATUS_killed);
> I found in xen/timer.h, the comment after the definition of the
> TIMER_STATUS_invalid is
> 
>                 #define TIMER_STATUS_invalid  0 /* Should never see
> this.           */
> 
> This comment is a little contrary to how the status is used here.
> Actually, what does it exactly means by "Should never see this"?
> 
> This _invalid status is used in timer.h and it is the status when a
> timer is initialized by init_timer().
> 
As far as my understanding goes, this means that a timer, during its
operations, should never be found in this state.

In fact, this mark a situation where the timer has been allocated but
never initialized, and there are ASSERT()s around to enforce that.

However, if what one wants is _exactly_ to check whether the timer has
been allocated ut not initialized, I don't see why I can't use this.

> So I'm thinking maybe this comment can be better improved to avoid
> confusion?
> 
I don't think things are confusing, neither right now, nor after this
patch, but I'm open to others' opinion. :-)

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: signature.asc
Description: This is a digitally signed message part

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