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

[Laszlo Ersek]

On 01/11/18 07:28, Pankaj Bansal wrote:
> Hi Laszlo,
> 
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Wednesday, January 10, 2018 10:34 PM
>> To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org
>> Cc: Leif Lindholm <leif.lindh...@linaro.org>; Ard Biesheuvel
>> <ard.biesheu...@linaro.org>; Michael Kinney <michael.d.kin...@intel.com>
>> Subject: Re: [edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement
>> GetElapsedTime function of TimerLib

>> (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.
> 
> Thank you. I will try to work on this problem of integrating these changes in 
> multiple
> TimerLib instances, when I have some room on my plate.
> For now, I guess I will just add wrapper functions on top of TimerLib APIs 
> for my Platform only.

Sure, that works as well -- feel free to scavenge the code as you see fit.

Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Pankaj Bansal]

Hi Laszlo,

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, January 10, 2018 10:34 PM
> To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org
> Cc: Leif Lindholm <leif.lindh...@linaro.org>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>; Michael Kinney <michael.d.kin...@intel.com>
> Subject: Re: [edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement
> GetElapsedTime function of TimerLib
> 
> 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:ler...@redhat.com]
> >> Sent: Wednesday, January 10, 2018 7:46 PM
> >> To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org
> >> Cc: Leif Lindholm <leif.lindh...@linaro.org>; Ard Biesheuvel
> >> <ard.biesheu...@linaro.org>; Michael Kinney
> >> <michael.d.kin...@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.lindh...@linaro.org>
> >>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Pankaj Bansal <pankaj.ban...@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

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

[Laszlo Ersek]

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:ler...@redhat.com]
>> Sent: Wednesday, January 10, 2018 7:46 PM
>> To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org
>> Cc: Leif Lindholm <leif.lindh...@linaro.org>; Ard Biesheuvel
>> <ard.biesheu...@linaro.org>; Michael Kinney <michael.d.kin...@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.lindh...@linaro.org>
>>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Pankaj Bansal <pankaj.ban...@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*

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

[Pankaj Bansal]

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:ler...@redhat.com]
> Sent: Wednesday, January 10, 2018 7:46 PM
> To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org
> Cc: Leif Lindholm <leif.lindh...@linaro.org>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>; Michael Kinney <michael.d.kin...@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.lindh...@linaro.org>
> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Pankaj Bansal <pankaj.ban...@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

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

[Laszlo Ersek]

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 
> Cc: Ard Biesheuvel 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Pankaj Bansal 
>
> 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.

(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.

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.

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?
  http://mid.mail-archive.com/8cba2a58-1333-7733-031d-0883dbd844c6@redhat.com

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Pankaj Bansal]

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 
Cc: Ard Biesheuvel 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal 

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);
+}
-- 
2.7.4

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel