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

[Meng Xu <mengxu@xxxxxxxxxxxxxx>]

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



You can use this. Actually, I agree with how you used this here.
Actually, this is also how the existing init_timer() uses it.


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


Hmm, I won't get confused with the comment from now on, but I'm unsure
if someone else will or not. The tricky thing is when I know it, I
won't feel weird. However, when I first read it, I feel a little
confusing if not reading the other parts of the code related to this
macro.

Anyway, I'm ok with either way: change the comment or not.

Best Regards,

Meng


-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/

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