[v3,0/2] Timer library changes

This patch series modifies the timer library in such a way that
structures that used to be statically allocated in a process's data
segment are now allocated in shared memory.  As these structures contain
lists of timers, new APIs are introduced that allow a caller to specify
the particular structure instance into which a timer should be inserted
or from which a timer should be removed.  This enables primary and
secondary processes to modify the same timer list, which enables some
multi-process use cases that were not previously possible; e.g. a
secondary process can start a timer whose expiration is detected in a
primary process running a new flavor of timer_manage().

The original library API is mostly unchanged, though implementations are
updated to call into newly added functions with a default structure
instance ID that provides the original behavior.  New functions are
introduced to enable applications to allocate structure instances to
house timer lists, and to reference them with an identifier when
starting and stopping timers, and finally, to manage the timer lists
referenced with an identifier.

My initial performance testing with the "timer_perf_autotest" test shows
no performance regression or improvement, and inspection of the
generated optimized code shows that the extra function call gets inlined
in the functions that now have an extra function call. 

Depends on: https://patches.dpdk.org/patch/48417/

Changes in v3:
 - remove C++ style comment in first patch in series (Stephen)

Changes in v2:
 - split these changes out into their own series
 - version the symbols where the existing ABI was updated, and
   provide alternate implementation with behavior equivalent to original
   behavior. Validated ABI compatibility with validate-abi.sh
 - refactor changes to simplify patches

Erik Gabriel Carrillo (2):
  timer: allow timer management in shared memory
  timer: add function to stop all timers in a list

 lib/librte_timer/Makefile              |   1 +
 lib/librte_timer/rte_timer.c           | 558 ++++++++++++++++++++++++++++++---
 lib/librte_timer/rte_timer.h           | 258 ++++++++++++++-
 lib/librte_timer/rte_timer_version.map |  23 ++
 4 files changed, 795 insertions(+), 45 deletions(-)
  

13/12/2018 23:26, Erik Gabriel Carrillo:
> This patch series modifies the timer library in such a way that
> structures that used to be statically allocated in a process's data
> segment are now allocated in shared memory.  As these structures contain
> lists of timers, new APIs are introduced that allow a caller to specify
> the particular structure instance into which a timer should be inserted
> or from which a timer should be removed.  This enables primary and
> secondary processes to modify the same timer list, which enables some
> multi-process use cases that were not previously possible; e.g. a
> secondary process can start a timer whose expiration is detected in a
> primary process running a new flavor of timer_manage().
> 
> The original library API is mostly unchanged, though implementations are
> updated to call into newly added functions with a default structure
> instance ID that provides the original behavior.  New functions are
> introduced to enable applications to allocate structure instances to
> house timer lists, and to reference them with an identifier when
> starting and stopping timers, and finally, to manage the timer lists
> referenced with an identifier.
> 
> My initial performance testing with the "timer_perf_autotest" test shows
> no performance regression or improvement, and inspection of the
> generated optimized code shows that the extra function call gets inlined
> in the functions that now have an extra function call. 
> 
> Depends on: https://patches.dpdk.org/patch/48417/
> 
> Changes in v3:
>  - remove C++ style comment in first patch in series (Stephen)
> 
> Changes in v2:
>  - split these changes out into their own series
>  - version the symbols where the existing ABI was updated, and
>    provide alternate implementation with behavior equivalent to original
>    behavior. Validated ABI compatibility with validate-abi.sh
>  - refactor changes to simplify patches
> 
> Erik Gabriel Carrillo (2):
>   timer: allow timer management in shared memory
>   timer: add function to stop all timers in a list
> 
>  lib/librte_timer/Makefile              |   1 +
>  lib/librte_timer/rte_timer.c           | 558 ++++++++++++++++++++++++++++++---
>  lib/librte_timer/rte_timer.h           | 258 ++++++++++++++-
>  lib/librte_timer/rte_timer_version.map |  23 ++
>  4 files changed, 795 insertions(+), 45 deletions(-)

It is a lot of changes!
Anyone to review please?
  

On 2018-12-19 04:35, Thomas Monjalon wrote:
> 13/12/2018 23:26, Erik Gabriel Carrillo:
>> This patch series modifies the timer library in such a way that
>> structures that used to be statically allocated in a process's data
>> segment are now allocated in shared memory.  As these structures contain
>> lists of timers, new APIs are introduced that allow a caller to specify
>> the particular structure instance into which a timer should be inserted
>> or from which a timer should be removed.  This enables primary and
>> secondary processes to modify the same timer list, which enables some
>> multi-process use cases that were not previously possible; e.g. a
>> secondary process can start a timer whose expiration is detected in a
>> primary process running a new flavor of timer_manage().
>>
>> The original library API is mostly unchanged, though implementations are
>> updated to call into newly added functions with a default structure
>> instance ID that provides the original behavior.  New functions are
>> introduced to enable applications to allocate structure instances to
>> house timer lists, and to reference them with an identifier when
>> starting and stopping timers, and finally, to manage the timer lists
>> referenced with an identifier.
>>
>> My initial performance testing with the "timer_perf_autotest" test shows
>> no performance regression or improvement, and inspection of the
>> generated optimized code shows that the extra function call gets inlined
>> in the functions that now have an extra function call.
>>
>> Depends on: https://patches.dpdk.org/patch/48417/
>>
>> Changes in v3:
>>   - remove C++ style comment in first patch in series (Stephen)
>>
>> Changes in v2:
>>   - split these changes out into their own series
>>   - version the symbols where the existing ABI was updated, and
>>     provide alternate implementation with behavior equivalent to original
>>     behavior. Validated ABI compatibility with validate-abi.sh
>>   - refactor changes to simplify patches
>>
>> Erik Gabriel Carrillo (2):
>>    timer: allow timer management in shared memory
>>    timer: add function to stop all timers in a list
>>
>>   lib/librte_timer/Makefile              |   1 +
>>   lib/librte_timer/rte_timer.c           | 558 ++++++++++++++++++++++++++++++---
>>   lib/librte_timer/rte_timer.h           | 258 ++++++++++++++-
>>   lib/librte_timer/rte_timer_version.map |  23 ++
>>   4 files changed, 795 insertions(+), 45 deletions(-)
> 
> It is a lot of changes!
> Anyone to review please?
> 
> 

I can give reviewing the overall aim with the patch set a try: Do we 
really want DPDK to support more secondary process-based use cases? I 
would rather see it supporting fewer, with the long term goal of 
dropping support for secondary processes altogether.

DPDK secondary processes are a horrible mess, in my opinion, to put it 
bluntly.
  

Hi all,

I'd like to bring this patch proposal up again and see if I can get any more feedback from the maintainer or others.

I need to update the map file to reflect the next release, so I'll add those changes in if any other modifications are suggested.

Thanks,
Erik

ML:  https://mails.dpdk.org/archives/dev/2018-December/120864.html
Patchwork:  https://patches.dpdk.org/project/dpdk/list/?series=2767

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Erik Gabriel Carrillo
> Sent: Thursday, December 13, 2018 4:27 PM
> To: rsanford@akamai.com
> Cc: stephen@networkplumber.org; jerin.jacob@caviumnetworks.com;
> pbhagavatula@caviumnetworks.com; dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 0/2] Timer library changes
> 
> This patch series modifies the timer library in such a way that structures that
> used to be statically allocated in a process's data segment are now allocated
> in shared memory.  As these structures contain lists of timers, new APIs are
> introduced that allow a caller to specify the particular structure instance into
> which a timer should be inserted or from which a timer should be removed.
> This enables primary and secondary processes to modify the same timer list,
> which enables some multi-process use cases that were not previously
> possible; e.g. a secondary process can start a timer whose expiration is
> detected in a primary process running a new flavor of timer_manage().
> 
> The original library API is mostly unchanged, though implementations are
> updated to call into newly added functions with a default structure instance
> ID that provides the original behavior.  New functions are introduced to
> enable applications to allocate structure instances to house timer lists, and to
> reference them with an identifier when starting and stopping timers, and
> finally, to manage the timer lists referenced with an identifier.
> 
> My initial performance testing with the "timer_perf_autotest" test shows no
> performance regression or improvement, and inspection of the generated
> optimized code shows that the extra function call gets inlined in the functions
> that now have an extra function call.
> 
> Depends on: https://patches.dpdk.org/patch/48417/
> 
> Changes in v3:
>  - remove C++ style comment in first patch in series (Stephen)
> 
> Changes in v2:
>  - split these changes out into their own series
>  - version the symbols where the existing ABI was updated, and
>    provide alternate implementation with behavior equivalent to original
>    behavior. Validated ABI compatibility with validate-abi.sh
>  - refactor changes to simplify patches
> 
> Erik Gabriel Carrillo (2):
>   timer: allow timer management in shared memory
>   timer: add function to stop all timers in a list
> 
>  lib/librte_timer/Makefile              |   1 +
>  lib/librte_timer/rte_timer.c           | 558
> ++++++++++++++++++++++++++++++---
>  lib/librte_timer/rte_timer.h           | 258 ++++++++++++++-
>  lib/librte_timer/rte_timer_version.map |  23 ++
>  4 files changed, 795 insertions(+), 45 deletions(-)
> 
> --
> 2.6.4
  

05/03/2019 23:41, Carrillo, Erik G:
> Hi all,
> 
> I'd like to bring this patch proposal up again and see if I can get any more feedback from the maintainer or others.
> 
> I need to update the map file to reflect the next release, so I'll add those changes in if any other modifications are suggested.

Please send an updated version.
If nobody reply after 10 days, I will push it.

Would you be interested to become maintainer of the timer lib?
  

Hi Erik,

Apologies if I am reaching out a bit late. Please find my query below

<snipped>
> > This enables primary and secondary processes to modify the same timer
> > list, which enables some multi-process use cases that were not
> > previously possible; e.g. a secondary process can start a timer whose
> > expiration is detected in a primary process running a new flavor of
> timer_manage().
Does this mean the following, primary can detect the timer expire primed by secondary. On calling new timer_manage() from primary will it invoke call back handler of secondary? If yes, has this been tested with shared library too?
<snipped>
  

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Tuesday, March 5, 2019 8:39 PM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com
> Cc: dev@dpdk.org; techboard@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3 0/2] Timer library changes
> 
> Hi Erik,
> 
> Apologies if I am reaching out a bit late. Please find my query below
> 
> <snipped>
> > > This enables primary and secondary processes to modify the same
> > > timer list, which enables some multi-process use cases that were not
> > > previously possible; e.g. a secondary process can start a timer
> > > whose expiration is detected in a primary process running a new
> > > flavor of
> > timer_manage().
> Does this mean the following, primary can detect the timer expire primed by
> secondary. On calling new timer_manage() from primary will it invoke call
> back handler of secondary? If yes, has this been tested with shared library
> too?
> <snipped>

Hi Vipin,

No, with the proposed patch,  the callback handler would need to be a function pointer valid in the same process that is invoking the new timer_manage().

Thanks,
Gabriel
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, March 5, 2019 4:59 PM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Cc: techboard@dpdk.org; rsanford@akamai.com; dev@dpdk.org
> Subject: Re: [dpdk-techboard] [dpdk-dev] [PATCH v3 0/2] Timer library
> changes
> 
> 05/03/2019 23:41, Carrillo, Erik G:
> > Hi all,
> >
> > I'd like to bring this patch proposal up again and see if I can get any more
> feedback from the maintainer or others.
> >
> > I need to update the map file to reflect the next release, so I'll add those
> changes in if any other modifications are suggested.
> 
> Please send an updated version.
> If nobody reply after 10 days, I will push it.
> 

Thanks, Thomas.  I submitted a v4 a little earlier.

> Would you be interested to become maintainer of the timer lib?
> 
> 

Yes, I'd be willing.  I'll do my best ;)

Regards,
Gabriel
  

06/03/2019 19:54, Carrillo, Erik G:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 05/03/2019 23:41, Carrillo, Erik G:
> > > Hi all,
> > >
> > > I'd like to bring this patch proposal up again and see if I can get any more
> > feedback from the maintainer or others.
> > >
> > > I need to update the map file to reflect the next release, so I'll add those
> > changes in if any other modifications are suggested.
> > 
> > Please send an updated version.
> > If nobody reply after 10 days, I will push it.
> > 
> 
> Thanks, Thomas.  I submitted a v4 a little earlier.
> 
> > Would you be interested to become maintainer of the timer lib?
> > 
> > 
> 
> Yes, I'd be willing.  I'll do my best ;)

Great, thanks.
Next step will be to send a patch for MAINTAINERS file.
  

Hi Gabriel,

Thanks for the clarification.

> -----Original Message-----
> From: Carrillo, Erik G
> Sent: Wednesday, March 6, 2019 8:46 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>; rsanford@akamai.com
> Cc: dev@dpdk.org; techboard@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3 0/2] Timer library changes
> 
> > -----Original Message-----
> > From: Varghese, Vipin
> > Sent: Tuesday, March 5, 2019 8:39 PM
> > To: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com
> > Cc: dev@dpdk.org; techboard@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v3 0/2] Timer library changes
> >
> > Hi Erik,
> >
> > Apologies if I am reaching out a bit late. Please find my query below
> >
> > <snipped>
> > > > This enables primary and secondary processes to modify the same
> > > > timer list, which enables some multi-process use cases that were
> > > > not previously possible; e.g. a secondary process can start a
> > > > timer whose expiration is detected in a primary process running a
> > > > new flavor of
> > > timer_manage().
> > Does this mean the following, primary can detect the timer expire
> > primed by secondary. On calling new timer_manage() from primary will
> > it invoke call back handler of secondary? If yes, has this been tested
> > with shared library too?
> > <snipped>
> 
> Hi Vipin,
> 
> No, with the proposed patch,  the callback handler would need to be a function
> pointer valid in the same process that is invoking the new timer_manage().
> 
> Thanks,
> Gabriel