Re: [Xen-devel] [PATCH] x86/hvm/pmtimer: improving scalability of virtual time update on Xen 4.0.0

[Song Xiang <classicxsong@xxxxxxxxx>]

It can be as follows:
@@ -90,8 +90,8 @@ static void pmt_update_time(PMTState *s)

     /* Update the timer */
     curr_gtime = hvm_get_guest_time(s->vcpu);
-    s->pm.tmr_val += ((curr_gtime - s->last_gtime) * s->scale) >> 32;
-    s->pm.tmr_val &= TMR_VAL_MASK;
+    *(volatile uint32_t *)&s->pm.tmr_val = (s->pm.tmr_val +
+        (((curr_gtime - s->last_gtime) * s->scale) >> 32)) &  
TMR_VAL_MASK;
     s->last_gtime = curr_gtime;

     /* If the counter's MSB has changed, set the status bit */


but it seems gcc (gcc 4.4.4) has omitted the statement s->pm.tmr_val  
&= TMR_VAL_MASK;
in the object file

在 2010-11-17，下午2:47， Tim Deegan 写道：

> At 21:39 +0000 on 17 Nov (1290029945), Song Xiang wrote:
> > diff --git a/xen/arch/x86/hvm/pmtimer.c b/xen/arch/x86/hvm/pmtimer.c
> > index 48fe26a..587944f 100644
> > --- a/xen/arch/x86/hvm/pmtimer.c
> > +++ b/xen/arch/x86/hvm/pmtimer.c
> > @@ -90,7 +90,7 @@ static void pmt_update_time(PMTState *s)
> >
> >     /* Update the timer */
> >     curr_gtime = hvm_get_guest_time(s->vcpu);
> > -    s->pm.tmr_val += ((curr_gtime - s->last_gtime) * s->scale) >>  
> > 32;
> > +    *(volatile uint32_t *)&s->pm.tmr_val = s->pm.tmr_val +  
> > (((curr_gtime - s->last_gtime) * s->scale) >> 32);
> >     s->pm.tmr_val &= TMR_VAL_MASK;
> >     s->last_gtime = curr_gtime;
> That doesn't make it an atomic update!  The variable is still written
> to twice. :)  You need to calculate the new tmr_val in a scratch
> variable, and then write it back once at the end of the function.
> (And no 'volatile' wll be necessary).
>
> Cheers,
>
> Tim.
>
> > @@ -206,10 +206,19 @@ static int handle_pmt_io(
> >
> >     if ( dir == IOREQ_READ )
> >     {
> > -        spin_lock(&s->lock);
> > -        pmt_update_time(s);
> > -        *val = s->pm.tmr_val;
> > -        spin_unlock(&s->lock);
> > +        /*
> > +         * if acquired the PMTState lock then update the time
> > +         * else other vcpu is updating it ,it should be up to date.
> > +         */
> > +        if (spin_trylock(&s->lock)) {
> > +            pmt_update_time(s);
> > +            *val = s->pm.tmr_val;
> > +            spin_unlock(&s->lock);
> > +        }
> > +        else {
> > +            spin_barrier(&s->lock);
> > +            *val = s->pm.tmr_val;
> > +        }
> >         return X86EMUL_OKAY;
> >     }
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxxxxxxxx
> > http://lists.xensource.com/xen-devel
>
> --
> Tim Deegan <Tim.Deegan@xxxxxxxxxx>
> Principal Software Engineer, Xen Platform Team
> Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel