[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RE: [PATCH] xen/arm: avoid extra caclulations when setting vtimer in context switch


  • To: Julien Grall <julien@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Jiamei Xie <Jiamei.Xie@xxxxxxx>
  • Date: Tue, 28 Jun 2022 06:35:09 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com])
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=UH+cqtypElYfY90Wuyq0LIWrsJrUyePK4fCJonffO00=; b=QHdixmv1ESTXoG/zDvOyCWo/TyM+sC8bpqxr5F8l9Bh2wdYLg5wkGJpgPJFt7CvSVXoiPG+osM7wRKsiqtZo4iNfb+tUyhl0sD+bhqiW2gl5fp9yTY/93uHgbaEpgdcb/OLel7UGLWDDtpwFkj/KLrJ7RlHmeWRbIyuhzAcdC7q8Lmcr5/OC0+kcAqUUL52MvLh7MHD2xASzi9mVXSMJIAmc4I/Z6GJinDqYU4b6Ot3c1u8kB1LJsR5ekvPtq3zTEfQnl2UPyphV1bmWgcpEFyspLVrZD3oQzOdcHLpuLMWP6QidWjCe0f5ZfUtq1lW8HhHtllIDf1NkvoU7tBUJPg==
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=UH+cqtypElYfY90Wuyq0LIWrsJrUyePK4fCJonffO00=; b=PEcaaSZlw2QG+EOJE6aTARl3eSIERmeNFyefugKUS3Mty8pF4hG/KkWmZeUp+yN1jN/Y+Q59MbzxEj3Xi31GyP5EAnnraJJu5aC/UmwUDu5Uuxqp/6zxLKQzEo0pKUQz2hK7awlh2YhRtVqJWpNQH6WKom2p+ACWxySMZuHdRn/pvFNHH91FLLSZBZr8Vt8OSKpcIm81LeNUReArYr29KIZisIiB8lDpO9kCsXky5wCKCCqnIwc1IfjzV1NUVJpzCP/Vq3uIm4e5wDdlCi4T+iCeAaPXpSfMPnWF5P25OdGnkg+jhlvb0rj9O1uP3NsHG9dL7oVF11qOumoGdtDJaQ==
  • Arc-seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=CDes4tHhCMeWc4X8urhDz2aoEsMJoKcyZrt4jVZ/mIDj1GxQiNoSWmut7FB91MGyHOk9iQ93yPeelgFCGMBPMYtyP871fAZhIOqLNSFYLpavUz5WC6BTI4BPQWg6gwfoSFRNwFpa1nMMLPshcoQVFNFG2o0+AIsFxDFNhCdAsf6jaYXWugurzg8APdKXeawdt8Y3Peq47j42Le1chpC8XoRVkm37iKk8YxNFX6qwcsIw0JXrgatSQMYnEyiigvTbXOBuoGGQC0Daf6dctnfH9lo2+hHH6C2H5tcFGqjnOUCIEMiHRvDNef7E58E3m0PrPaeN6e0gA0kY62YmARkLCg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=af+ZZ7oV3AJzVHaHlXTSwGhObS4zDiY3qSr57OKdqPZfOC4L5HVvcQQ9bmHBH0Qx2Qzu4aKLAjzbWURKoYyPfuQmhkkYatin6NqutdWkStMLJlXr20gX88LqdfXb7f12DgFkgVzCYD5uiir8D70tc3T8oFnmRnTTUGv4lY6Yi/HROxYObG9kVAs0+k4xPgwEmlzBdnOQtFmRCRsfZZTYgJd8R+pErZ2FjLER8Es2v3xd24Ld2CTNlbQNSyFzYQwpZwASweufCjJMIKYc1v1eXXCscDf8fUNqlPk7QhKOrtEsp7oCpiA7QE7rbyjTsfujm6ou8H3bm7ZXaWa7pI/hag==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Wei Chen <Wei.Chen@xxxxxxx>
  • Delivery-date: Tue, 28 Jun 2022 06:35:30 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHYidHeBWnNLqFeFEKI7B/maNA44q1jD7IAgAFN1oA=
  • Thread-topic: [PATCH] xen/arm: avoid extra caclulations when setting vtimer in context switch

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2022年6月27日 18:36
> To: Jiamei Xie <Jiamei.Xie@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Wei Chen <Wei.Chen@xxxxxxx>
> Subject: Re: [PATCH] xen/arm: avoid extra caclulations when setting vtimer
> in context switch
> 
> Hi Jiami
> 
> Title: s/caclulations/calculations/
> 
> However, I think the title should mention the overflow rather than the
> extra calculations. The former is more important the latter.
> 
I will change the title to " xen/arm: avoid overflow when setting vtimer in 
context switch"

> On 27/06/2022 03:58, Jiamei Xie wrote:
> > virt_vtimer_save is calculating the new time for the vtimer in:
> > "v->arch.virt_timer.cval + v->domain->arch.virt_timer_base.offset
> > - boot_count".
> > In this formula, "cval + offset" might cause uint64_t overflow.
> > Changing it to "v->domain->arch.virt_timer_base.offset - boot_count +
> > v->arch.virt_timer.cval" can reduce the possibility of overflow
> 
> This read strange to me. We want to remove the overflow completely not
> reducing it. The overflow is completely removed by converting the
> "offset - bount_count" to ns upfront.
> 
> AFAICT, the commit message doesn't explain that.
Thanks for pointing out that. How about putting the commit message like the 
below:
    xen/arm: avoid overflow when setting vtimer in context switch

    virt_vtimer_save is calculating the new time for the vtimer in:
    "v->arch.virt_timer.cval + v->domain->arch.virt_timer_base.offset
    - boot_count".
    In this formula, "cval + offset" might cause uint64_t overflow.
    Changing it to "ticks_to_ns(v->domain->arch.virt_timer_base.offset -
    boot_count) + ticks_to_ns(v->arch.virt_timer.cval)" can avoid overflow,
    and "ticks_to_ns(arch.virt_timer_base.offset - boot_count)" will be
    always the same, which has been caculated in domain_vtimer_init.
    Introduce a new field virt_timer_base.nanoseconds to store this value
    for arm in struct arch_domain, so we can use it directly.
> 
> > , and
> > "arch.virt_timer_base.offset - boot_count" will be always the same,
> > which has been caculated in domain_vtimer_init. Introduce a new field
> > vtimer_offset.nanoseconds to store this value for arm in struct
> > arch_domain, so we can use it directly and extra caclulations can be
> > avoided.
> >
> > This patch is enlightened from [1].
> >
> > Signed-off-by: Jiamei Xie <jiamei.xie@xxxxxxx>
> >
> > [1] https://www.mail-archive.com/xen-
> devel@xxxxxxxxxxxxxxxxxxxx/msg123139.htm
> 
> This link doesn't work. But I would personally remove it from the commit
> message (or add ---) because it doesn't bring value (this patch looks
> like a v2 to me).
Sorry, a 'l' is missing at the end of the link.  The link is  
https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxxxxxxxxx/msg123139.html .
I will put it after --- in v3.
> 
> > ---
> > xen/arch/arm/include/asm/domain.h | 4 ++++
> >   xen/arch/arm/vtimer.c             | 6 ++++--
> >   2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/domain.h
> b/xen/arch/arm/include/asm/domain.h
> > index ed63c2b6f9..94fe5b6444 100644
> > --- a/xen/arch/arm/include/asm/domain.h
> > +++ b/xen/arch/arm/include/asm/domain.h
> > @@ -73,6 +73,10 @@ struct arch_domain
> >           uint64_t offset;
> >       } virt_timer_base;
> >
> > +    struct {
> > +        int64_t nanoseconds;
> 
> This should be s_time_t to match the argument of set_timer() and return
> of ticks_to_ns().
> 
> > +    } vtimer_offset;
> 
> Why are you adding a new structure rather than re-using virt_timer_base?
Sure, I'll add this field in virt_timer_base.
     struct {
         uint64_t offset;
         s_time_t nanoseconds;
     } virt_timer_base;
> 
> > +
> >       struct vgic_dist vgic;
> >
> >       struct vuart {
> > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> > index 6b78fea77d..54161e5fea 100644
> > --- a/xen/arch/arm/vtimer.c
> > +++ b/xen/arch/arm/vtimer.c
> > @@ -64,6 +64,7 @@ int domain_vtimer_init(struct domain *d, struct
> xen_arch_domainconfig *config)
> >   {
> >       d->arch.virt_timer_base.offset = get_cycles();
> >       d->time_offset.seconds = ticks_to_ns(d->arch.virt_timer_base.offset -
> boot_count);
> > +    d->arch.vtimer_offset.nanoseconds = d->time_offset.seconds;
> 
> Hmmm... I find odd to assign a field "nanoseconds" to "seconds". I would
> suggest to re-order so you first set nanoseconds and then set seconds.
> 
> This will make more obvious that this is not a mistake and "seconds"
> will be closer to the do_div() below.
Is it ok to remove do_div and write like below?
    d->arch.virt_timer_base.nanoseconds =
        ticks_to_ns(d->arch.virt_timer_base.offset - boot_count);
    d->time_offset.seconds = d->arch.virt_timer_base.nanoseconds /
                              1000000000;

Best wishes
Jiamei Xie


> 
> >       do_div(d->time_offset.seconds, 1000000000);
> >
> >       config->clock_frequency = timer_dt_clock_frequency;
> > @@ -144,8 +145,9 @@ void virt_timer_save(struct vcpu *v)
> >       if ( (v->arch.virt_timer.ctl & CNTx_CTL_ENABLE) &&
> >            !(v->arch.virt_timer.ctl & CNTx_CTL_MASK))
> >       {
> > -        set_timer(&v->arch.virt_timer.timer, ticks_to_ns(v-
> >arch.virt_timer.cval +
> > -                  v->domain->arch.virt_timer_base.offset - boot_count));
> > +        set_timer(&v->arch.virt_timer.timer,
> > +                  v->domain->arch.vtimer_offset.nanoseconds +
> > +                  ticks_to_ns(v->arch.virt_timer.cval));
> >       }
> >   }
> >
> 
> Cheers,
> 
> --
> Julien Grall

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.