Re: [Xen-devel] [PATCH v8 10/11] viridian: add implementation of synthetic timers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.03.19 at 15:37, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 18 March 2019 14:24
>> 
>> >>> On 18.03.19 at 12:20, <paul.durrant@xxxxxxxxxx> wrote:
>> > @@ -72,11 +77,14 @@ static void update_reference_tsc(struct domain *d, 
>> > bool initialize)
>> >       * ticks per 100ns shifted left by 64.
>> >       */
>> >      p->TscScale = ((10000ul << 32) / d->arch.tsc_khz) << 32;
>> > +    smp_wmb();
>> > +
>> > +    seq = p->TscSequence + 1;
>> > +    if ( seq == 0xFFFFFFFF || seq == 0 ) /* Avoid both 'invalid' values */
>> > +        seq = 1;
>> >
>> > -    p->TscSequence++;
>> > -    if ( p->TscSequence == 0xFFFFFFFF ||
>> > -         p->TscSequence == 0 ) /* Avoid both 'invalid' values */
>> > -        p->TscSequence = 1;
>> > +    p->TscSequence = seq;
>> > +    vd->reference_tsc_valid = true;
>> 
>> Strictly speaking, don't you need another smp_wmb() between
>> these two lines?
>> 
> 
> Since the data in the page is not used by time_now() I don't think so.

Oh, have I been remembering an old version of the patch, where
there was a consumer of p->TscSequence?

>> > +            return;
>> > +        }
>> > +    }
>> > +    ASSERT(expiration - now > 0);
>> > +
>> > +    vs->expiration = expiration;
>> > +    timeout = (expiration - now) * 100ull;
>> > +
>> > +    vs->started = true;
>> > +    migrate_timer(&vs->timer, smp_processor_id());
>> 
>> Why is this smp_processor_id() when viridian_time_vcpu_init() uses
>> v->processor? How relevant is it in the first place to trace the pCPU
>> the vCPU runs on for the timer?
> 
> I was just following suit with other timer code. It seems to be the norm to 
> migrate to the current pCPU just prior to starting a timer.

But wouldn't v->processor then be more visibly correct (besides
likely being cheaper to get at), as to the correlation to the vCPU
in question? I can't actually see why "migrate to the current pCPU"
would be the norm; I could only see this as an implication from
that other code you looked at simply acting on the current vCPU.

Then again I'm having trouble spotting why it would be important
for the timer to run on the same CPU the vCPU runs one. By the
time the timer fires, the vCPU may have gone elsewhere.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel