Re: [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement GetElapsedTime function of TimerLib

[Laszlo Ersek <lersek@redhat.com>]

Hi Pankaj,

On 01/10/18 17:05, Pankaj Bansal wrote:
> Hi Laszlo,
> 
> Thanks for reviewing my changes and thanks for pointing me to the thread that you worked on.
> It was really helpful. please see my response inline ..
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, January 10, 2018 7:46 PM
>> To: Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>; Michael Kinney <michael.d.kinney@intel.com>
>> Subject: Re: [edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement
>> GetElapsedTime function of TimerLib
>>
>> Hello Pankaj,
>>
>> On 01/10/18 10:31, Pankaj Bansal wrote:
>>> This function calculates the time elaped in Naoseconds between call to
>>> this function and BaseTime, which is passed as argument.
>>>
>>> This is particularly useful in detecting timeout conditions.
>>>
>>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
>>>
>>> diff --git a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
>>> b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
>>> index b81293c..0898339 100644
>>> --- a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
>>> +++ b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
>>> @@ -290,3 +290,39 @@ GetTimeInNanoSecond (
>>>
>>>    return NanoSeconds;
>>>  }
>>> +
>>> +/**
>>> +  Get Elapsed time in Nanoseonds w.r.t BaseTime
>>> +
>>> +  This function calculates the time elaped in Naoseconds between call
>>> + to this  function and BaseTime, which is passed as argument.
>>> +
>>> +  @param  BaseTime     BaseTime in NanoSeconds.
>>> +
>>> +  @return The elapsed time in nanoseconds.
>>> +
>>> +**/
>>> +UINT64
>>> +EFIAPI
>>> +GetElapsedTime (
>>> +  IN      UINT64                     BaseTime
>>> +  )
>>> +{
>>> +  UINT64  NanoSeconds;
>>> +  UINT64  Ticks;
>>> +
>>> +  //
>>> +  // Get current Ticks.
>>> +  //
>>> +  Ticks = GetPerformanceCounter();
>>> +
>>> +  //
>>> +  // Convert Ticks to Nanoseconds
>>> +  //
>>> +  NanoSeconds = GetTimeInNanoSecond (Ticks);
>>> +
>>> +  //
>>> +  // return the difference
>>> +  //
>>> +  return (NanoSeconds - BaseTime);
>>> +}
>>>
>>
>> There are two problems with this patch set:
>>
>> (1) The TimerLib.h file (in the first patch) is a public library class header, for
>> which several library instances (implementations) exist. So, introducing a
>> new API requires adding an implementation (the same implementation, or
>> different ones, as necessary) to *all* instances in the edk2 tree.
> 
> I agree with your intent to prevent code duplication. But I still feel that we should enhance TimerLib.
> Because, a user intending to use various timer functionalities, should include only one header file (TimerLib.h) and one Library.
> Like Mike pointed out in your thread to enhance the TimerTickDiffLib, in future if someone wishes to add some new functionality related
> to timer and if he writes a new library for it, then its added cumbersome for users (user has to include 3 header files and libraries 3.)
> 
> To prevent code duplication, can we look at other ways like using weak symbols for such functions that are platform agnostic.
> Or putting these generic functions in one .c file in MdePkg and include that file in inf file of TimerLib implementation?
> 
>>
>> (2) The calculation in your GetElapsedTime() function is wrong.
>> GetTimeInNanoSecond() converts a small *difference* of ticks to time. It
>> does not convert an absolute tick value to an absolute time.
> 
> The GetTimeInNanoSecond() is only concerned with the ticks. Those ticks can be a difference of ticks or absolute ticks from counter start.
> The *difference* part of GetElapsedTime would depend on its usage. Like this :
> Start = GetElapsedTime (0);
> // do something 
> End = GetElapsedTime (Start);
> 
>>
>> Furthermore, tick differences are less trivial to calculate than one might
>> imagine, because (a) a performance counter may count down, and
>> *independently*, (b) the numeric low bound of the counter range may not
>> be zero.
>>
> 
> This is nice observation. It was an oversight on my part.
> 
>> Earlier I proposed a new TimerTickDiffLib class (and implementation) for
>> centralizing exactly this kind of calculation. Please see the thread at:
>>
>>   [edk2] TimerTickDiffLib for MdePkg?
>>
>> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.
>> mail-archive.com%2F8cba2a58-1333-7733-031d-
>> 0883dbd844c6%40redhat.com&data=02%7C01%7Cpankaj.bansal%40nxp.co
>> m%7C17efb96f35f0456be43108d55834c721%7C686ea1d3bc2b4c6fa92cd99c5
>> c301635%7C0%7C0%7C636511906063092098&sdata=AIqII9VcJEQXrJmM79G
>> DS%2BzBu3%2BCUva%2FZESawMRndzA%3D&reserved=0
>>
> 
> Your library has everything that I need. Can we work on to include this in MdePkg ?
> Then I won't need my changes.

(1) Please feel free to pick up my code and work on upstreaming it --
just please preserve my S-o-b on the patch(es) (in addition to yours),
and the Red Hat copyright notice (in addition to your company's) in the
new files.

(2) Last time I counted (reading up on the original thread), there were
21 TimerLib instances in edk2. Adding the same function to every one of
them looks terrible. In edk2, I know of only two ways to avoid this:

- introduce a new library class (with class header file), and a simple
library instance (implementation)

- collect (=move) as many as possible of the existing library instances
under common subdirectories, so that their INF files are basically in
the same subdirectory. Then the new code can be implemented in a new C
file, under the same subdirectory, and then all INF files can refer to
the new C file directly. This kind of code sharing works very well if
the library instances are otherwise closely related. It does not work at
all if the library instances live in separate top-level packages (which
is BTW the case here).


It seems that Mike wasn't opposed to introducing a TimerTickDiffLib
class (or something similar) to MdePkg. However, more work looked
necessary than I had expected, esp. with regard to the caching of timer
hardware characteristics (please see the last few messages in the thread
-- either the code has to add its own caching, or all existing, suitable
TimerLib instances have to be verified about caching). Again, please
feel free to pick up the code and rework it as Mike suggests -- it's
basically the GetTickDifference() function only that adds value. Feel
free to hammer it into any shape or form necessary.

Thanks
Laszlo