DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/2] Fix unit tests execution for ENA PMD
@ 2021-03-26 10:47 Michal Krawczyk
  2021-03-26 10:47 ` [dpdk-dev] [PATCH 1/2] timer: clarify subsystem_init return value Michal Krawczyk
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Michal Krawczyk @ 2021-03-26 10:47 UTC (permalink / raw)
  To: dev; +Cc: 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH 1/2] timer: clarify subsystem_init return value
  2021-03-26 10:47 [dpdk-dev] [PATCH 0/2] Fix unit tests execution for ENA PMD Michal Krawczyk
@ 2021-03-26 10:47 ` Michal Krawczyk
  2021-03-29 20:50   ` Carrillo, Erik G
  2021-03-26 10:47 ` [dpdk-dev] [PATCH 2/2] test: proceed if timer subsystem was initialized Michal Krawczyk
  2021-04-08 21:15 ` [dpdk-dev] [PATCH 0/2] Fix unit tests execution for ENA PMD Thomas Monjalon
  2 siblings, 1 reply; 8+ messages in thread
From: Michal Krawczyk @ 2021-03-26 10:47 UTC (permalink / raw)
  To: dev, Robert Sanford, Erik Gabriel Carrillo
  Cc: Stanislaw Kardach, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH 2/2] test: proceed if timer subsystem was initialized
  2021-03-26 10:47 [dpdk-dev] [PATCH 0/2] Fix unit tests execution for ENA PMD Michal Krawczyk
  2021-03-26 10:47 ` [dpdk-dev] [PATCH 1/2] timer: clarify subsystem_init return value Michal Krawczyk
@ 2021-03-26 10:47 ` Michal Krawczyk
  2021-04-06 15:24   ` Thomas Monjalon
  2021-04-08 21:15 ` [dpdk-dev] [PATCH 0/2] Fix unit tests execution for ENA PMD Thomas Monjalon
  2 siblings, 1 reply; 8+ messages in thread
From: Michal Krawczyk @ 2021-03-26 10:47 UTC (permalink / raw)
  To: dev; +Cc: Stanislaw Kardach, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] timer: clarify subsystem_init return value
  2021-03-26 10:47 ` [dpdk-dev] [PATCH 1/2] timer: clarify subsystem_init return value Michal Krawczyk
@ 2021-03-29 20:50   ` Carrillo, Erik G
  0 siblings, 0 replies; 8+ messages in thread
From: Carrillo, Erik G @ 2021-03-29 20:50 UTC (permalink / raw)
  To: Michal Krawczyk, dev, Robert Sanford; +Cc: Stanislaw Kardach

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] test: proceed if timer subsystem was initialized
  2021-03-26 10:47 ` [dpdk-dev] [PATCH 2/2] test: proceed if timer subsystem was initialized Michal Krawczyk
@ 2021-04-06 15:24   ` Thomas Monjalon
  2021-04-06 15:31     ` Stanisław Kardach
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Monjalon @ 2021-04-06 15:24 UTC (permalink / raw)
  To: Stanislaw Kardach; +Cc: dev, Michal Krawczyk, erik.g.carrillo

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?



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] test: proceed if timer subsystem was initialized
  2021-04-06 15:24   ` Thomas Monjalon
@ 2021-04-06 15:31     ` Stanisław Kardach
  2021-04-06 15:40       ` Thomas Monjalon
  0 siblings, 1 reply; 8+ messages in thread
From: Stanisław Kardach @ 2021-04-06 15:31 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, Michal Krawczyk, erik.g.carrillo

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] test: proceed if timer subsystem was initialized
  2021-04-06 15:31     ` Stanisław Kardach
@ 2021-04-06 15:40       ` Thomas Monjalon
  0 siblings, 0 replies; 8+ messages in thread
From: Thomas Monjalon @ 2021-04-06 15:40 UTC (permalink / raw)
  To: Stanisław Kardach; +Cc: dev, Michal Krawczyk, erik.g.carrillo

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.





^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Fix unit tests execution for ENA PMD
  2021-03-26 10:47 [dpdk-dev] [PATCH 0/2] Fix unit tests execution for ENA PMD Michal Krawczyk
  2021-03-26 10:47 ` [dpdk-dev] [PATCH 1/2] timer: clarify subsystem_init return value Michal Krawczyk
  2021-03-26 10:47 ` [dpdk-dev] [PATCH 2/2] test: proceed if timer subsystem was initialized Michal Krawczyk
@ 2021-04-08 21:15 ` Thomas Monjalon
  2 siblings, 0 replies; 8+ messages in thread
From: Thomas Monjalon @ 2021-04-08 21:15 UTC (permalink / raw)
  To: Michal Krawczyk; +Cc: dev

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-04-08 21:15 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-26 10:47 [dpdk-dev] [PATCH 0/2] Fix unit tests execution for ENA PMD Michal Krawczyk
2021-03-26 10:47 ` [dpdk-dev] [PATCH 1/2] timer: clarify subsystem_init return value Michal Krawczyk
2021-03-29 20:50   ` Carrillo, Erik G
2021-03-26 10:47 ` [dpdk-dev] [PATCH 2/2] test: proceed if timer subsystem was initialized Michal Krawczyk
2021-04-06 15:24   ` Thomas Monjalon
2021-04-06 15:31     ` Stanisław Kardach
2021-04-06 15:40       ` Thomas Monjalon
2021-04-08 21:15 ` [dpdk-dev] [PATCH 0/2] Fix unit tests execution for ENA PMD Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).