On Wed, 2016-08-17 at 20:57 -0400, Meng Xu wrote:
> On Wed, Aug 17, 2016 at 1:18 PM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> > 
> > diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> > index 41c61a7..903dbd8 100644
> > --- a/xen/common/sched_rt.c
> > +++ b/xen/common/sched_rt.c
> > 
> > @@ -1035,6 +1036,20 @@ rt_schedule(const struct scheduler *ops,
> > s_time_t now, bool_t tasklet_work_sched
> >      struct rt_vcpu *snext = NULL;
> >      struct task_slice ret = { .migrated = 0 };
> > 
> > +    /* TRACE */
> > +    {
> > +        struct __packed {
> > +            unsigned cpu:17, tasklet:8, tickled:4, idle:4;
> > +        } d;
> > +        d.cpu = cpu;
> > +        d.tasklet = tasklet_work_scheduled;
> > +        d.tickled = cpumask_test_cpu(cpu, &prv->tickled);
> > +        d.idle = is_idle_vcpu(current);
> > +        __trace_var(TRC_RTDS_SCHEDULE, 1,
> > +                    sizeof(d),
> > +                    (unsigned char *)&d);
> > +    }
>

> 1) IMHO, the trace should be wrapped by the if (
> unlikely(tb_init_done) ) {} statement as done in sched_credit2.c.
> Otherwise, we always enable the tracing which may hurt the
> performance, I think.
> 
You're right. Actually, in order to follow suite from sched_rt.c, I
think using trace_var() instead of __trace_var() is what we want.

Then, I think it will be a good thing, at some point, to convert all
these /*TRACE*/ blocks to extrapolate the tb_init_done check and make
it guard the trace record marshalling, like I did a few patches ago for
Credit2.... but that's another patch.

This is a cut-&-paste mistake, good job noticing it. :-)

> 2) Why does the cpu field here use 17 bits instead of 16 bits as used
> in credit2?
> This may be a typo I guess (since you are trying to align the
> structure to 32 bits I guess ;-) )?
>
Wow... 17.. I must have been drunk when writing that! :-O

> In addition, I'm wondering if uint16 is better than unsigned? I'm not
> that confident if unsigned type will always have 16 bits on all types
> of machines?
> 
Yeah, well, TBH, all this bitfields and packing, etc., is something I
truly hate. But I think in this case unsigned is fine.

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)