|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCHv3 1/7] plat/common: Calculate shift factors for coversion between ns and tick
> -----Original Message-----
> From: Santiago Pagani <Santiago.Pagani@xxxxxxxxx>
> Sent: 2019年9月16日 14:48
> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>;
> Sharan Santhanam <Sharan.Santhanam@xxxxxxxxx>
> Cc: Julien Grall <Julien.Grall@xxxxxxx>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@xxxxxxx>; Wei Chen (Arm Technology China)
> <Wei.Chen@xxxxxxx>; Jianyong Wu (Arm Technology China)
> <Jianyong.Wu@xxxxxxx>
> Subject: Re: [UNIKRAFT PATCHv3 1/7] plat/common: Calculate shift factors
> for coversion between ns and tick
>
> Hi Justin,
>
> Thanks for the reply. Please find a last comment on regards to one of the
> points inline.
>
> Thanks and best,
> Santiago
>
> On 16.09.19, 07:48, "Justin He (Arm Technology China)"
> <Justin.He@xxxxxxx> wrote:
>
> Hi Santiago, thanks for the review
> Please see inline
>
> > -----Original Message-----
> > From: Santiago Pagani <Santiago.Pagani@xxxxxxxxx>
> > Sent: 2019年9月10日 15:02
> > To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> > devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer
> <simon.kuenzer@xxxxxxxxx>;
> > Sharan Santhanam <Sharan.Santhanam@xxxxxxxxx>
> > Cc: Julien Grall <Julien.Grall@xxxxxxx>; Kaly Xin (Arm Technology
> China)
> > <Kaly.Xin@xxxxxxx>; Wei Chen (Arm Technology China)
> > <Wei.Chen@xxxxxxx>; Jianyong Wu (Arm Technology China)
> > <Jianyong.Wu@xxxxxxx>
> > Subject: Re: [UNIKRAFT PATCHv3 1/7] plat/common: Calculate shift
> factors
> > for coversion between ns and tick
> >
> > Hi all,
> >
> > Thanks for the patch. Please find my comments inline:
> >
> > On 30.07.19, 16:27, "Jia He" <justin.he@xxxxxxx> wrote:
> >
> > We had shift factor for coverting counter ticks to ns, but it's not
> > enough. Sometime, we need to covert ns to ticks. For example, we'll
> > transfer sleep(ns) to counter ticks. If we don't have the shift
> factor,
> > the conversion accuracy will be lower.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> > Signed-off-by: Jia He <justin.he@xxxxxxx>
> > ---
> > plat/common/arm/time.c | 111
> > +++++++++++++++++++++++++++++++----------
> > 1 file changed, 86 insertions(+), 25 deletions(-)
> >
> > diff --git a/plat/common/arm/time.c b/plat/common/arm/time.c
> > index 1b30903..4c66039 100644
> > --- a/plat/common/arm/time.c
> > +++ b/plat/common/arm/time.c
> > @@ -47,21 +47,83 @@
> > static uint64_t boot_ticks;
> > static uint32_t counter_freq;
> >
> > -/*
> > - * Shift factor for counter scaling multiplier; referred to as S
> in the
> > - * following comments.
> > - */
> > -static uint8_t counter_shift;
> >
> > -/* Multiplier for converting counter ticks to nsecs. (0.S) fixed
> point.
> */
> > +/* Shift factor for converting ticks to ns */
> > +static uint8_t counter_shift_to_ns;
> > +
> > +/* Shift factor for converting ns to ticks */
> > +static uint8_t counter_shift_to_tick;
> > +
> > +/* Multiplier for converting counter ticks to nsecs */
> > static uint32_t ns_per_tick;
> >
> > +/* Multiplier for converting nsecs to counter ticks */
> > +static uint32_t tick_per_ns;
> > +
> > +/*
> > + * The maximum time range in seconds which can be converted by
> > multiplier
> > + * and shift factors. This will guarantee the converted value not
> to
> > exceed
> > + * 64-bit unsigned integer. Increase the time range will reduce the
> > accuracy
> > + * of conversion, because we will get smaller multiplier and shift
> factors.
> > + * In this case, we selected 3600s as the time range.
> > + */
> > +#define __MAX_CONVERT_SECS 3600UL
> > COMMNET: As I understand it, this #define is also used when
> calculating
> > the ns_per_tick, it not only imposes a limit to the time range, but also
> to
> > the tick range. That is, we do not support converting ns_to_ticks for
> more
> > than 3600 seconds, and as well we do not support to convert
> ticks_to_ns
> > for more than 3600000000 ticks, correct? Maybe change the name of
> the
> > #define to something more generic.
> Hmm, __MAX_CONVERT_SECS is for converting ticks_to_ns only, not the
> converting of ns_to_ticks
> So, how about __MAX_SECS_CONVERT_TO_NS ?
>
> COMMENT: Well, the only place where __MAX_CONVERT_SECS is used is
> inside function 'calculate_mult_shift' to compute 'sftacc'. Function
> 'calculate_mult_shift' is then used both to compute 'tick_per_ns and
> counter_shift_to_tick', as well as 'ns_per_tick and counter_shift_to_ns'.
> Inside function 'calculate_mult_shift', the value of __MAX_CONVERT_SECS
> is multiplied by 'from'. In one case when calling the function, 'from' is set
> to NSEC_PER_SEC, however in the case, 'from' is set to ' counter_freq'.
> Therefore, as we are multiplying __MAX_CONVERT_SECS with two variables
> with opposed physical meanings, __MAX_CONVERT_SECS is in fact also
> been utilized in the opposite way.
Sorry for previous mistakes.
__MAX_SECS_CONVERT_TO_NS is not correct
In your mention 1st case, e.g. assume the freq is 25Mhz
calculate_mult_shift(&mult,&shift,1000000000L,25*1000000);
__MAX_CONVERT_SECS is to say the range ns is [0,3600*1000,000,000],
In your second case,
calculate_mult_shift(&mult,&shift ,25*1000000, 1000000000);
__MAX_CONVERT_SECS is to say the range ticks is [0, 3600*25*1000,000]
So, IMO, we *needn't change the name __MAX_CONVERT_SECS here.
What is your opinion?
--
Cheers,
Justin (Jia He)
>
> > +
> > /* How many nanoseconds per second */
> > #define NSEC_PER_SEC ukarch_time_sec_to_nsec(1)
> >
> > static inline uint64_t ticks_to_ns(uint64_t ticks)
> > {
> > - return (ns_per_tick * ticks) >> counter_shift;
> > + return (ns_per_tick * ticks) >> counter_shift_to_ns;
> > COMMENT: Maybe add an assertion for the maximum number of ticks
> as a
> > sanity check?
>
> Yes, reasonable to me
>
> > +}
> > +
> > +static inline uint64_t ns_to_ticks(uint64_t ns)
> > +{
> > + return (tick_per_ns * ns) >> counter_shift_to_tick;
> > COMMENT: Maybe add an assertion for the maximum number of
> seconds
> > as a sanity check?
>
> Yes
>
> > +}
> > +
> > +/*
> > + * Calculate multiplier/shift factors for scaled math.
> > + */
> > +static void calculate_mult_shift(uint32_t *mult, uint8_t *shift,
> > + uint64_t from, uint64_t to)
> > +{
> > + uint64_t tmp;
> > + uint32_t sft, sftacc = 32;
> > +
> > + /*
> > + * Calculate the shift factor which is limiting the conversion
> > + * range:
> > + */
> > + tmp = ((uint64_t)__MAX_CONVERT_SECS * from) >> 32;
> > + while (tmp) {
> > + tmp >>= 1;
> > + sftacc--;
> > + }
> > +
> > +
> > + /*
> > + * Calculate shift factor (S) and scaling multiplier (M).
> > + *
> > + * (S) needs to be the largest shift factor (<= max_shift) where
> > + * the result of the M calculation below fits into uint32_t
> > + * without truncation.
> > + *
> > + * multiplier = (target << shift) / source
> > + */
> > + for (sft = 32; sft > 0; sft--) {
> > + tmp = (uint64_t) to << sft;
> > +
> > + /* Ensuring we round to nearest when calculating the
> > + * multiplier
> > + */
> > + tmp += from / 2;
> > + tmp /= from;
> > + if ((tmp >> sftacc) == 0)
> > + break;
> > + }
> > + *mult = tmp;
> > + *shift = sft;
> > }
> >
> > /*
> > @@ -145,29 +207,28 @@ static __u64
> generic_timer_epochoffset(void)
> >
> > static int generic_timer_init(void)
> > {
> > + counter_freq = get_counter_frequency();
> > +
> > /*
> > - * Calculate counter shift factor and scaling multiplier.
> > - *
> > - * counter_shift (S) needs to be the largest (<=32) shift factor
> where
> > - * the result of the ns_per_tick calculation below fits into
> uint32_t
> > - * without truncation. Note that we disallow an S of zero to
> ensure
> > - * the loop always terminates.
> > - *
> > - * (0.S) ns_per_tick = NSEC_PER_SEC (S.S) / counter_freq (S.0)
> > + * Calculate the shift factor and scaling multiplier for
> > + * cpnverting ticks to ns.
> > */
> > TYPO: Typo in comment: cpnverting -> converting
>
> OK
>
> > - uint64_t tmp;
> > + calculate_mult_shift(&ns_per_tick, &counter_shift_to_ns,
> > + counter_freq, NSEC_PER_SEC);
> >
> > - counter_freq = get_counter_frequency();
> > - counter_shift = 32;
> > - do {
> > - tmp = (NSEC_PER_SEC << counter_shift) / counter_freq;
> > - if ((tmp & 0xFFFFFFFF00000000L) == 0L)
> > - ns_per_tick = (uint32_t)tmp;
> > - else
> > - counter_shift--;
> > - } while (counter_shift > 0 && ns_per_tick == 0L);
> > + /* We disallow zero ns_per_tick */
> > UK_BUGON(!ns_per_tick);
> >
> > + /*
> > + * Calculate the shift factor and scaling multiplier for
> > + * cpnverting ns to ticks.
> > + */
> > TYPO: Typo in comment: cpnverting -> converting
>
> OK
>
> > + calculate_mult_shift(&tick_per_ns, &counter_shift_to_tick,
> > + NSEC_PER_SEC, counter_freq);
> > +
> > + /* We disallow zero ns_per_tick */
> > + UK_BUGON(!tick_per_ns);
> > +
> > /*
> > * Monotonic time begins at boot_ticks (first read of counter
> > * before calibration).
> > --
> > 2.17.1
> >
> >
> >
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
>
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |