Hi, ENA PMD uses timer service to implement various periodic device status check routine. Because of that, it already initializes the timer subsystem. As unit tests also initalizes the timer subsystem, return value is equal to -EAGAIN, as it was already done by the ENA PMD. This patch set adds missing documentation that this kind of return code may appear and this is not a failure and also changes unit tests tool, to do not fail if timer subsystem was already initialized. Stanislaw Kardach (2): timer: clarify subsystem_init return value test: proceed if timer subsystem was initialized app/test/test.c | 11 ++++++----- lib/librte_timer/rte_timer.h | 1 + 2 files changed, 7 insertions(+), 5 deletions(-) -- 2.25.1
From: Stanislaw Kardach <kda@semihalf.com> rte_timer_subsystem_init() may return -EALREADY if it has been already initialized. Therefore put explicitly into doxygen that this is not a failure for the application. Signed-off-by: Stanislaw Kardach <kda@semihalf.com> Reviewed-by: Michal Krawczyk <mk@semihalf.com> --- lib/librte_timer/rte_timer.h | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h index 22a27aa08d..0f820555c0 100644 --- a/lib/librte_timer/rte_timer.h +++ b/lib/librte_timer/rte_timer.h @@ -171,6 +171,7 @@ int rte_timer_data_dealloc(uint32_t id); * - 0: Success * - -ENOMEM: Unable to allocate memory needed to initialize timer * subsystem + * - -EALREADY: timer subsystem was already initialized. Not an error. */ int rte_timer_subsystem_init(void); -- 2.25.1
From: Stanislaw Kardach <kda@semihalf.com> rte_timer_subsystem_init() may return -EALREADY if the timer subsystem was already initialized. This can happen i.e. in PMD code (see eth_ena_dev_init). This is not an error, rather a notification as the initialization function simply returns without any action taken. Signed-off-by: Stanislaw Kardach <kda@semihalf.com> Reviewed-by: Michal Krawczyk <mk@semihalf.com> --- app/test/test.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/app/test/test.c b/app/test/test.c index 624dd48042..864523ed61 100644 --- a/app/test/test.c +++ b/app/test/test.c @@ -134,8 +134,13 @@ main(int argc, char **argv) goto out; } + argv += ret; + + prgname = argv[0]; + #ifdef RTE_LIB_TIMER - if (rte_timer_subsystem_init() < 0) { + ret = rte_timer_subsystem_init(); + if (ret < 0 && ret != -EALREADY) { ret = -1; goto out; } @@ -146,10 +151,6 @@ main(int argc, char **argv) goto out; } - argv += ret; - - prgname = argv[0]; - recursive_call = getenv(RECURSIVE_ENV_VAR); if (recursive_call != NULL) { ret = do_recursive_call(); -- 2.25.1
> -----Original Message-----
> From: Michal Krawczyk <mk@semihalf.com>
> Sent: Friday, March 26, 2021 5:48 AM
> To: dev@dpdk.org; Robert Sanford <rsanford@akamai.com>; Carrillo, Erik G
> <erik.g.carrillo@intel.com>
> Cc: Stanislaw Kardach <kda@semihalf.com>; Michal Krawczyk
> <mk@semihalf.com>
> Subject: [PATCH 1/2] timer: clarify subsystem_init return value
>
> From: Stanislaw Kardach <kda@semihalf.com>
>
> rte_timer_subsystem_init() may return -EALREADY if it has been already
> initialized. Therefore put explicitly into doxygen that this is not a failure for
> the application.
>
> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> Reviewed-by: Michal Krawczyk <mk@semihalf.com>
> ---
Thanks, Michal.
Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
26/03/2021 11:47, Michal Krawczyk: > From: Stanislaw Kardach <kda@semihalf.com> > > rte_timer_subsystem_init() may return -EALREADY if the timer subsystem > was already initialized. This can happen i.e. in PMD code (see > eth_ena_dev_init). This is not an error, rather a notification as the > initialization function simply returns without any action taken. Missing these lines: Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process") Cc: stable@dpdk.org > Signed-off-by: Stanislaw Kardach <kda@semihalf.com> > Reviewed-by: Michal Krawczyk <mk@semihalf.com> > --- > app/test/test.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/app/test/test.c b/app/test/test.c > index 624dd48042..864523ed61 100644 > --- a/app/test/test.c > +++ b/app/test/test.c > @@ -134,8 +134,13 @@ main(int argc, char **argv) > goto out; > } > > + argv += ret; > + > + prgname = argv[0]; > + > #ifdef RTE_LIB_TIMER > - if (rte_timer_subsystem_init() < 0) { > + ret = rte_timer_subsystem_init(); > + if (ret < 0 && ret != -EALREADY) { > ret = -1; > goto out; > } > @@ -146,10 +151,6 @@ main(int argc, char **argv) > goto out; > } > > - argv += ret; > - > - prgname = argv[0]; > - How this change for argv/prgname is related to the fix?
Hi Thomas, Thanks for the review. On Tue, Apr 6, 2021 at 5:24 PM Thomas Monjalon <thomas@monjalon.net> wrote: > 26/03/2021 11:47, Michal Krawczyk: > > From: Stanislaw Kardach <kda@semihalf.com> > > > > rte_timer_subsystem_init() may return -EALREADY if the timer subsystem > > was already initialized. This can happen i.e. in PMD code (see > > eth_ena_dev_init). This is not an error, rather a notification as the > > initialization function simply returns without any action taken. > > Missing these lines: > > Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process") > Cc: stable@dpdk.org Will add in V2. > > > > Signed-off-by: Stanislaw Kardach <kda@semihalf.com> > > Reviewed-by: Michal Krawczyk <mk@semihalf.com> > > --- > > app/test/test.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/app/test/test.c b/app/test/test.c > > index 624dd48042..864523ed61 100644 > > --- a/app/test/test.c > > +++ b/app/test/test.c > > @@ -134,8 +134,13 @@ main(int argc, char **argv) > > goto out; > > } > > > > + argv += ret; > > + > > + prgname = argv[0]; > > + > > #ifdef RTE_LIB_TIMER > > - if (rte_timer_subsystem_init() < 0) { > > + ret = rte_timer_subsystem_init(); > > + if (ret < 0 && ret != -EALREADY) { > > ret = -1; > > goto out; > > } > > @@ -146,10 +151,6 @@ main(int argc, char **argv) > > goto out; > > } > > > > - argv += ret; > > - > > - prgname = argv[0]; > > - > > How this change for argv/prgname is related to the fix? > This patch saves the return value of rte_timer_subsystem_init() in ret which overwrites the previous ret that held the number of arguments consumed by rte_eal_init(). Similarly because rte_eal_init() reshuffles argv, the prgname is effectively at argv[ret]. So I need to move this logic before the timer subsystem check.
06/04/2021 17:31, Stanisław Kardach:
> Hi Thomas,
>
> Thanks for the review.
>
> On Tue, Apr 6, 2021 at 5:24 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> > 26/03/2021 11:47, Michal Krawczyk:
> > > From: Stanislaw Kardach <kda@semihalf.com>
> > >
> > > rte_timer_subsystem_init() may return -EALREADY if the timer subsystem
> > > was already initialized. This can happen i.e. in PMD code (see
> > > eth_ena_dev_init). This is not an error, rather a notification as the
> > > initialization function simply returns without any action taken.
> >
> > Missing these lines:
> >
> > Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process")
> > Cc: stable@dpdk.org
>
> Will add in V2.
>
> >
> >
> > > Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> > > Reviewed-by: Michal Krawczyk <mk@semihalf.com>
> > > ---
> > > app/test/test.c | 11 ++++++-----
> > > 1 file changed, 6 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/app/test/test.c b/app/test/test.c
> > > index 624dd48042..864523ed61 100644
> > > --- a/app/test/test.c
> > > +++ b/app/test/test.c
> > > @@ -134,8 +134,13 @@ main(int argc, char **argv)
> > > goto out;
> > > }
> > >
> > > + argv += ret;
> > > +
> > > + prgname = argv[0];
> > > +
> > > #ifdef RTE_LIB_TIMER
> > > - if (rte_timer_subsystem_init() < 0) {
> > > + ret = rte_timer_subsystem_init();
> > > + if (ret < 0 && ret != -EALREADY) {
> > > ret = -1;
> > > goto out;
> > > }
> > > @@ -146,10 +151,6 @@ main(int argc, char **argv)
> > > goto out;
> > > }
> > >
> > > - argv += ret;
> > > -
> > > - prgname = argv[0];
> > > -
> >
> > How this change for argv/prgname is related to the fix?
> >
> This patch saves the return value of rte_timer_subsystem_init() in ret
> which overwrites the previous ret that held the number of arguments
> consumed by rte_eal_init(). Similarly because rte_eal_init() reshuffles
> argv, the prgname is effectively at argv[ret]. So I need to move this logic
> before the timer subsystem check.
OK I didn't see the consequence on ret variable.
In this case I can merge with the added lines.
No need of v2.
26/03/2021 11:47, Michal Krawczyk:
> ENA PMD uses timer service to implement various periodic device status
> check routine. Because of that, it already initializes the timer
> subsystem. As unit tests also initalizes the timer subsystem, return
> value is equal to -EAGAIN, as it was already done by the ENA PMD.
>
> This patch set adds missing documentation that this kind of return code
> may appear and this is not a failure and also changes unit tests tool,
> to do not fail if timer subsystem was already initialized.
>
> Stanislaw Kardach (2):
> timer: clarify subsystem_init return value
> test: proceed if timer subsystem was initialized
Applied, thanks