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

[Jiamei Xie <Jiamei.Xie@xxxxxxx>]

Hi Bertrand,

> -----Original Message-----
> From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Sent: 2022年6月28日 15:29
> To: Jiamei Xie <Jiamei.Xie@xxxxxxx>
> Cc: Julien Grall <julien@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; 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 Jiamei,
> 
> > On 28 Jun 2022, at 07:35, Jiamei Xie <Jiamei.Xie@xxxxxxx> wrote:
> >
> > 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;
> 
> The implementation must use do_div to properly handle the division from a
> 64bit by a 32bit on arm32 otherwise the code will be a lot slower.

Thanks for your explanation for this. I will keep the do_div. 

Best wishes
Jiamei Xie


> 
> Cheers
> Bertrand
>