Hi, Andrew
The change was proposed to solve incorrect truncation and overflow issue on old
revision.
The original code is:
Counter = (UINT32) DivU64x32Remainder (
Microseconds * 10,
gMetronome->TickPeriod,
&Remainder
);
In original code, there is Microseconds * 10 which may be out of UINTN
boundary. And Counter is forced to truncate to 32 bit, which may also bring
issue.
Thanks
Feng
-----Original Message-----
From: Andrew Fish [mailto:[email protected]]
Sent: Tuesday, September 16, 2014 05:10
To: [email protected]
Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg: CoreStall: improve accuracy in
iterating branch
On Sep 15, 2014, at 11:29 AM, Laszlo Ersek <[email protected]> wrote:
> On 09/15/14 18:53, Andrew Fish wrote:
>>
>> On Sep 15, 2014, at 12:00 AM, Tian, Feng <[email protected]> wrote:
>>
>>> Hi, Laszlo
>>>
>>> Here is our intention for the logic:
>>>
>>> If Microseconds is greater than or equal to 0x1999999999999999ULL(about
>>> 58,494 years), then the divide operation is performed first, which means we
>>> will lose some accuracy, but this is a very large delay request, so waiting
>>> a little bit extra in this extreme case is an acceptable compromise.
>>>
>>> So I would prefer not to add this patch if there is no real impact.
>>>
>>
>> Was this issue caught by some static analysis tool?
>
> Nikolai submitted a patch that simply caused me to look at the
> function in question. In 2005-2006 I had written a UDP data transfer
> program that was obsessed with accurate packet rate, time keeping, and
> preventing long-term drift (I still use it occasionally, if the link
> quality justifies it); hence my interest in fractional ticks.
>
Sorry I slotted this in the wrong location. I wanted to ask about the initial
patch that added 0x199999999999999ULL?
>> Why do we need to have code to deal with something that could never
>> happen?
>
> Well, such function arguments *could* happen. How the boot service
> should react is a different question. Technically the implementation
> matches the UEFI spec.
>
> (Compare for example the select() specification in POSIX:
> <http://pubs.opengroup.org/onlinepubs/9699919799/functions/select.html
> >
>
> Implementations may place limitations on the maximum timeout
> interval supported. All implementations shall support a maximum
> timeout interval of at least 31 days. If the timeout argument
> specifies a timeout interval greater than the implementation-
> defined maximum value, the maximum value shall be used as the
> actual timeout value. [...]
>
> The next errata / release of the UEFI spec might want to state
> something like this too.)
>
I was thinking that capping the max value would not be a bad idea. Thanks for
the POSIX reference.
Thanks,
Andrew Fish
> Thanks,
> Laszlo
>
>
> ----------------------------------------------------------------------
> --------
> Want excitement?
> Manually upgrade your production database.
> When you want reliability, choose Perforce Perforce version control.
> Predictably reliable.
> http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.
> clktrk _______________________________________________
> edk2-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce Perforce version control.
Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
