[dpdk-dev] [PATCH 0/2] Fix unit tests execution for ENA PMD

[dpdk-dev] [PATCH 0/2] Fix unit tests execution for ENA PMD

[Michal Krawczyk]

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

[dpdk-dev] [PATCH 1/2] timer: clarify subsystem_init return value

[Michal Krawczyk]

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

Re: [dpdk-dev] [PATCH 1/2] timer: clarify subsystem_init return value

[Carrillo, Erik G]

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

[dpdk-dev] [PATCH 2/2] test: proceed if timer subsystem was initialized

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

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

Re: [dpdk-dev] [PATCH 2/2] test: proceed if timer subsystem was initialized

[Thomas Monjalon]

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?

Re: [dpdk-dev] [PATCH 2/2] test: proceed if timer subsystem was initialized

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

Re: [dpdk-dev] [PATCH 2/2] test: proceed if timer subsystem was initialized

[Thomas Monjalon]

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.

Re: [dpdk-dev] [PATCH 0/2] Fix unit tests execution for ENA PMD

[Thomas Monjalon]

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