DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] mbuf: allow dynamic flags to be used by secondary process
@ 2020-10-15 17:20 Stephen Hemminger
  2020-10-20 12:18 ` Olivier Matz
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Stephen Hemminger @ 2020-10-15 17:20 UTC (permalink / raw)
  To: olivier.matz, anatoly.burakov; +Cc: dev, Stephen Hemminger

The dynamic flag management is broken for multi-process environment.
All calls to lookup dynamic flags or fields will fail in secondary process.
This is because the local pointer to the memzone is not ever initialized.

Fix it by using the same checks as dynfield_register().
I.e if shared memory zone has not been looked up already,
then discover it.

Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
Cc: olivier.matz@6wind.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_mbuf/rte_mbuf_dyn.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
index 538a43f6959f..ac4801b4d770 100644
--- a/lib/librte_mbuf/rte_mbuf_dyn.c
+++ b/lib/librte_mbuf/rte_mbuf_dyn.c
@@ -185,7 +185,7 @@ rte_mbuf_dynfield_lookup(const char *name, struct rte_mbuf_dynfield *params)
 {
 	struct mbuf_dynfield_elt *mbuf_dynfield;
 
-	if (shm == NULL) {
+	if (shm == NULL && init_shared_mem() < 0) {
 		rte_errno = ENOENT;
 		return -1;
 	}
@@ -384,7 +384,7 @@ rte_mbuf_dynflag_lookup(const char *name,
 {
 	struct mbuf_dynflag_elt *mbuf_dynflag;
 
-	if (shm == NULL) {
+	if (shm == NULL && init_shared_mem() < 0) {
 		rte_errno = ENOENT;
 		return -1;
 	}
-- 
2.27.0


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

* Re: [dpdk-dev] [PATCH] mbuf: allow dynamic flags to be used by secondary process
  2020-10-15 17:20 [dpdk-dev] [PATCH] mbuf: allow dynamic flags to be used by secondary process Stephen Hemminger
@ 2020-10-20 12:18 ` Olivier Matz
  2020-10-20 20:58 ` [dpdk-dev] [PATCH v2] " Stephen Hemminger
  2020-10-24  0:43 ` [dpdk-dev] [PATCH v3] mbuf: fix dynamic flags lookup from " Stephen Hemminger
  2 siblings, 0 replies; 12+ messages in thread
From: Olivier Matz @ 2020-10-20 12:18 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: anatoly.burakov, dev

Hi Stephen,

On Thu, Oct 15, 2020 at 10:20:19AM -0700, Stephen Hemminger wrote:
> mbuf: allow dynamic flags to be used by secondary process

Suggested title:
mbuf: fix dynamic flags lookup from secondary process

>
> The dynamic flag management is broken for multi-process environment.
> All calls to lookup dynamic flags or fields will fail in secondary process.
> This is because the local pointer to the memzone is not ever initialized.
> 
> Fix it by using the same checks as dynfield_register().
> I.e if shared memory zone has not been looked up already,
> then discover it.
> 
> Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
> Cc: olivier.matz@6wind.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Good catch, please Cc stable for v2.

> ---
>  lib/librte_mbuf/rte_mbuf_dyn.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
> index 538a43f6959f..ac4801b4d770 100644
> --- a/lib/librte_mbuf/rte_mbuf_dyn.c
> +++ b/lib/librte_mbuf/rte_mbuf_dyn.c
> @@ -185,7 +185,7 @@ rte_mbuf_dynfield_lookup(const char *name, struct rte_mbuf_dynfield *params)
>  {
>  	struct mbuf_dynfield_elt *mbuf_dynfield;
>  
> -	if (shm == NULL) {
> +	if (shm == NULL && init_shared_mem() < 0) {
>  		rte_errno = ENOENT;
>  		return -1;
>  	}

init_shared_mem() is supposed to be called with tailq lock held.

I think the test (shm == NULL && init_shared_mem() < 0) should be
moved in __mbuf_dynfield_lookup().

> @@ -384,7 +384,7 @@ rte_mbuf_dynflag_lookup(const char *name,
>  {
>  	struct mbuf_dynflag_elt *mbuf_dynflag;
>  
> -	if (shm == NULL) {
> +	if (shm == NULL && init_shared_mem() < 0) {
>  		rte_errno = ENOENT;
>  		return -1;
>  	}

Same here


Thanks,
Olivier

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

* [dpdk-dev] [PATCH v2] mbuf: allow dynamic flags to be used by secondary process
  2020-10-15 17:20 [dpdk-dev] [PATCH] mbuf: allow dynamic flags to be used by secondary process Stephen Hemminger
  2020-10-20 12:18 ` Olivier Matz
@ 2020-10-20 20:58 ` Stephen Hemminger
  2020-10-24  0:43 ` [dpdk-dev] [PATCH v3] mbuf: fix dynamic flags lookup from " Stephen Hemminger
  2 siblings, 0 replies; 12+ messages in thread
From: Stephen Hemminger @ 2020-10-20 20:58 UTC (permalink / raw)
  To: dev; +Cc: stable, Stephen Hemminger, olivier.matz

The dynamic flag management is broken if rte_mbuf_dynflag_lookup()
is done in a secondary process because the local pointer to
the memzone is not ever initialized.

Fix it by using the same checks as dynfield_register().
I.e if shared memory zone has not been looked up already,
then discover it.

Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
Cc: olivier.matz@6wind.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - move check to inside locked region

 lib/librte_mbuf/rte_mbuf_dyn.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
index 538a43f6959f..717898c0df02 100644
--- a/lib/librte_mbuf/rte_mbuf_dyn.c
+++ b/lib/librte_mbuf/rte_mbuf_dyn.c
@@ -185,13 +185,11 @@ rte_mbuf_dynfield_lookup(const char *name, struct rte_mbuf_dynfield *params)
 {
 	struct mbuf_dynfield_elt *mbuf_dynfield;
 
-	if (shm == NULL) {
-		rte_errno = ENOENT;
-		return -1;
-	}
-
 	rte_mcfg_tailq_read_lock();
-	mbuf_dynfield = __mbuf_dynfield_lookup(name);
+	if (shm == NULL && init_shared_mem() < 0)
+		mbuf_dynfield = NULL;
+	else
+		mbuf_dynfield = __mbuf_dynfield_lookup(name);
 	rte_mcfg_tailq_read_unlock();
 
 	if (mbuf_dynfield == NULL) {
@@ -384,13 +382,11 @@ rte_mbuf_dynflag_lookup(const char *name,
 {
 	struct mbuf_dynflag_elt *mbuf_dynflag;
 
-	if (shm == NULL) {
-		rte_errno = ENOENT;
-		return -1;
-	}
-
 	rte_mcfg_tailq_read_lock();
-	mbuf_dynflag = __mbuf_dynflag_lookup(name);
+	if (shm == NULL && init_shared_mem() < 0)
+		mbuf_dynflag = NULL;
+	else 
+		mbuf_dynflag = __mbuf_dynflag_lookup(name);
 	rte_mcfg_tailq_read_unlock();
 
 	if (mbuf_dynflag == NULL) {
-- 
2.27.0


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

* [dpdk-dev] [PATCH v3] mbuf: fix dynamic flags lookup from secondary process
  2020-10-15 17:20 [dpdk-dev] [PATCH] mbuf: allow dynamic flags to be used by secondary process Stephen Hemminger
  2020-10-20 12:18 ` Olivier Matz
  2020-10-20 20:58 ` [dpdk-dev] [PATCH v2] " Stephen Hemminger
@ 2020-10-24  0:43 ` Stephen Hemminger
  2020-10-26 10:39   ` Olivier Matz
                     ` (2 more replies)
  2 siblings, 3 replies; 12+ messages in thread
From: Stephen Hemminger @ 2020-10-24  0:43 UTC (permalink / raw)
  To: dev; +Cc: stable, Stephen Hemminger, olivier.matz

The dynamic flag management is broken if rte_mbuf_dynflag_lookup()
is done in a secondary process because the local pointer to
the memzone is not ever initialized.

Fix it by using the same checks as dynfield_register().
I.e if shared memory zone has not been looked up already,
then discover it.

Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
Cc: olivier.matz@6wind.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---

v3 - change title, fix one extra whitespace 

 lib/librte_mbuf/rte_mbuf_dyn.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
index 538a43f6959f..554ec5a1ca4f 100644
--- a/lib/librte_mbuf/rte_mbuf_dyn.c
+++ b/lib/librte_mbuf/rte_mbuf_dyn.c
@@ -185,13 +185,11 @@ rte_mbuf_dynfield_lookup(const char *name, struct rte_mbuf_dynfield *params)
 {
 	struct mbuf_dynfield_elt *mbuf_dynfield;
 
-	if (shm == NULL) {
-		rte_errno = ENOENT;
-		return -1;
-	}
-
 	rte_mcfg_tailq_read_lock();
-	mbuf_dynfield = __mbuf_dynfield_lookup(name);
+	if (shm == NULL && init_shared_mem() < 0)
+		mbuf_dynfield = NULL;
+	else
+		mbuf_dynfield = __mbuf_dynfield_lookup(name);
 	rte_mcfg_tailq_read_unlock();
 
 	if (mbuf_dynfield == NULL) {
@@ -384,13 +382,11 @@ rte_mbuf_dynflag_lookup(const char *name,
 {
 	struct mbuf_dynflag_elt *mbuf_dynflag;
 
-	if (shm == NULL) {
-		rte_errno = ENOENT;
-		return -1;
-	}
-
 	rte_mcfg_tailq_read_lock();
-	mbuf_dynflag = __mbuf_dynflag_lookup(name);
+	if (shm == NULL && init_shared_mem() < 0)
+		mbuf_dynflag = NULL;
+	else
+		mbuf_dynflag = __mbuf_dynflag_lookup(name);
 	rte_mcfg_tailq_read_unlock();
 
 	if (mbuf_dynflag == NULL) {
-- 
2.27.0


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

* Re: [dpdk-dev] [PATCH v3] mbuf: fix dynamic flags lookup from secondary process
  2020-10-24  0:43 ` [dpdk-dev] [PATCH v3] mbuf: fix dynamic flags lookup from " Stephen Hemminger
@ 2020-10-26 10:39   ` Olivier Matz
  2020-10-26 14:49     ` Stephen Hemminger
  2020-11-04  5:53   ` [dpdk-dev] [PATCH v4] mbuf: allow dynamic flags to be used by " Stephen Hemminger
  2020-11-04 16:20   ` [dpdk-dev] [PATCH v5] " Stephen Hemminger
  2 siblings, 1 reply; 12+ messages in thread
From: Olivier Matz @ 2020-10-26 10:39 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, stable

Hi Stephen,

On Fri, Oct 23, 2020 at 05:43:31PM -0700, Stephen Hemminger wrote:
> The dynamic flag management is broken if rte_mbuf_dynflag_lookup()
> is done in a secondary process because the local pointer to
> the memzone is not ever initialized.
> 
> Fix it by using the same checks as dynfield_register().
> I.e if shared memory zone has not been looked up already,
> then discover it.
> 
> Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
> Cc: olivier.matz@6wind.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> 
> v3 - change title, fix one extra whitespace 
> 
>  lib/librte_mbuf/rte_mbuf_dyn.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
> index 538a43f6959f..554ec5a1ca4f 100644
> --- a/lib/librte_mbuf/rte_mbuf_dyn.c
> +++ b/lib/librte_mbuf/rte_mbuf_dyn.c
> @@ -185,13 +185,11 @@ rte_mbuf_dynfield_lookup(const char *name, struct rte_mbuf_dynfield *params)
>  {
>  	struct mbuf_dynfield_elt *mbuf_dynfield;
>  
> -	if (shm == NULL) {
> -		rte_errno = ENOENT;
> -		return -1;
> -	}
> -
>  	rte_mcfg_tailq_read_lock();
> -	mbuf_dynfield = __mbuf_dynfield_lookup(name);
> +	if (shm == NULL && init_shared_mem() < 0)
> +		mbuf_dynfield = NULL;
> +	else
> +		mbuf_dynfield = __mbuf_dynfield_lookup(name);
>  	rte_mcfg_tailq_read_unlock();
>  
>  	if (mbuf_dynfield == NULL) {
>  		rte_errno = ENOENT;
>  		return -1;

There is still a small corner case here: on a primary process,
init_shared_mem() can return -1 in case rte_memzone_reserve_aligned()
returns a NULL memzone. In this situation, rte_errno is set by the
memzone layer by overriden to ENOENT.

Maybe something like this is better, what do you think?

@@ -172,7 +172,7 @@ __mbuf_dynfield_lookup(const char *name)
 			break;
 	}
 
-	if (te == NULL) {
+	if (te == NULL || mbuf_dynfield == NULL) {
 		rte_errno = ENOENT;
 		return NULL;
 	}
@@ -185,19 +185,15 @@ rte_mbuf_dynfield_lookup(const char *name, struct rte_mbuf_dynfield *params)
 {
 	struct mbuf_dynfield_elt *mbuf_dynfield;
 
-	if (shm == NULL) {
-		rte_errno = ENOENT;
-		return -1;
-	}
-
 	rte_mcfg_tailq_read_lock();
-	mbuf_dynfield = __mbuf_dynfield_lookup(name);
+	if (shm == NULL && init_shared_mem() < 0)
+		mbuf_dynfield = NULL;
+	else
+		mbuf_dynfield = __mbuf_dynfield_lookup(name);
 	rte_mcfg_tailq_read_unlock();
 
-	if (mbuf_dynfield == NULL) {
-		rte_errno = ENOENT;
+	if (mbuf_dynfield == NULL)
 		return -1;
-	}
 
 	if (params != NULL)
 		memcpy(params, &mbuf_dynfield->params, sizeof(*params));



Thanks,
Olivier


> @@ -384,13 +382,11 @@ rte_mbuf_dynflag_lookup(const char *name,
>  {
>  	struct mbuf_dynflag_elt *mbuf_dynflag;
>  
> -	if (shm == NULL) {
> -		rte_errno = ENOENT;
> -		return -1;
> -	}
> -
>  	rte_mcfg_tailq_read_lock();
> -	mbuf_dynflag = __mbuf_dynflag_lookup(name);
> +	if (shm == NULL && init_shared_mem() < 0)
> +		mbuf_dynflag = NULL;
> +	else
> +		mbuf_dynflag = __mbuf_dynflag_lookup(name);
>  	rte_mcfg_tailq_read_unlock();
>  
>  	if (mbuf_dynflag == NULL) {
> -- 
> 2.27.0
> 

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

* Re: [dpdk-dev] [PATCH v3] mbuf: fix dynamic flags lookup from secondary process
  2020-10-26 10:39   ` Olivier Matz
@ 2020-10-26 14:49     ` Stephen Hemminger
  2020-11-03 21:02       ` Thomas Monjalon
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2020-10-26 14:49 UTC (permalink / raw)
  To: Olivier Matz; +Cc: dev, stable

On Mon, 26 Oct 2020 11:39:35 +0100
Olivier Matz <olivier.matz@6wind.com> wrote:

> Hi Stephen,
> 
> On Fri, Oct 23, 2020 at 05:43:31PM -0700, Stephen Hemminger wrote:
> > The dynamic flag management is broken if rte_mbuf_dynflag_lookup()
> > is done in a secondary process because the local pointer to
> > the memzone is not ever initialized.
> > 
> > Fix it by using the same checks as dynfield_register().
> > I.e if shared memory zone has not been looked up already,
> > then discover it.
> > 
> > Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
> > Cc: olivier.matz@6wind.com
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > 
> > v3 - change title, fix one extra whitespace 
> > 
> >  lib/librte_mbuf/rte_mbuf_dyn.c | 20 ++++++++------------
> >  1 file changed, 8 insertions(+), 12 deletions(-)
> > 
> > diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
> > index 538a43f6959f..554ec5a1ca4f 100644
> > --- a/lib/librte_mbuf/rte_mbuf_dyn.c
> > +++ b/lib/librte_mbuf/rte_mbuf_dyn.c
> > @@ -185,13 +185,11 @@ rte_mbuf_dynfield_lookup(const char *name, struct rte_mbuf_dynfield *params)
> >  {
> >  	struct mbuf_dynfield_elt *mbuf_dynfield;
> >  
> > -	if (shm == NULL) {
> > -		rte_errno = ENOENT;
> > -		return -1;
> > -	}
> > -
> >  	rte_mcfg_tailq_read_lock();
> > -	mbuf_dynfield = __mbuf_dynfield_lookup(name);
> > +	if (shm == NULL && init_shared_mem() < 0)
> > +		mbuf_dynfield = NULL;
> > +	else
> > +		mbuf_dynfield = __mbuf_dynfield_lookup(name);
> >  	rte_mcfg_tailq_read_unlock();
> >  
> >  	if (mbuf_dynfield == NULL) {
> >  		rte_errno = ENOENT;
> >  		return -1;  
> 
> There is still a small corner case here: on a primary process,
> init_shared_mem() can return -1 in case rte_memzone_reserve_aligned()
> returns a NULL memzone. In this situation, rte_errno is set by the
> memzone layer by overriden to ENOENT.
> 
> Maybe something like this is better, what do you think?

Sure, for what I was using rte_errno was not important. And since it was
previously broken lets get it fixed.

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

* Re: [dpdk-dev] [PATCH v3] mbuf: fix dynamic flags lookup from secondary process
  2020-10-26 14:49     ` Stephen Hemminger
@ 2020-11-03 21:02       ` Thomas Monjalon
  0 siblings, 0 replies; 12+ messages in thread
From: Thomas Monjalon @ 2020-11-03 21:02 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Olivier Matz, dev, david.marchand

Stephen, we are waiting for a v4 please.


26/10/2020 15:49, Stephen Hemminger:
> On Mon, 26 Oct 2020 11:39:35 +0100
> Olivier Matz <olivier.matz@6wind.com> wrote:
> 
> > Hi Stephen,
> > 
> > On Fri, Oct 23, 2020 at 05:43:31PM -0700, Stephen Hemminger wrote:
> > > The dynamic flag management is broken if rte_mbuf_dynflag_lookup()
> > > is done in a secondary process because the local pointer to
> > > the memzone is not ever initialized.
> > > 
> > > Fix it by using the same checks as dynfield_register().
> > > I.e if shared memory zone has not been looked up already,
> > > then discover it.
> > > 
> > > Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
> > > Cc: olivier.matz@6wind.com
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---
> > > 
> > > v3 - change title, fix one extra whitespace 
> > > 
> > >  lib/librte_mbuf/rte_mbuf_dyn.c | 20 ++++++++------------
> > >  1 file changed, 8 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
> > > index 538a43f6959f..554ec5a1ca4f 100644
> > > --- a/lib/librte_mbuf/rte_mbuf_dyn.c
> > > +++ b/lib/librte_mbuf/rte_mbuf_dyn.c
> > > @@ -185,13 +185,11 @@ rte_mbuf_dynfield_lookup(const char *name, struct rte_mbuf_dynfield *params)
> > >  {
> > >  	struct mbuf_dynfield_elt *mbuf_dynfield;
> > >  
> > > -	if (shm == NULL) {
> > > -		rte_errno = ENOENT;
> > > -		return -1;
> > > -	}
> > > -
> > >  	rte_mcfg_tailq_read_lock();
> > > -	mbuf_dynfield = __mbuf_dynfield_lookup(name);
> > > +	if (shm == NULL && init_shared_mem() < 0)
> > > +		mbuf_dynfield = NULL;
> > > +	else
> > > +		mbuf_dynfield = __mbuf_dynfield_lookup(name);
> > >  	rte_mcfg_tailq_read_unlock();
> > >  
> > >  	if (mbuf_dynfield == NULL) {
> > >  		rte_errno = ENOENT;
> > >  		return -1;  
> > 
> > There is still a small corner case here: on a primary process,
> > init_shared_mem() can return -1 in case rte_memzone_reserve_aligned()
> > returns a NULL memzone. In this situation, rte_errno is set by the
> > memzone layer by overriden to ENOENT.
> > 
> > Maybe something like this is better, what do you think?
> 
> Sure, for what I was using rte_errno was not important. And since it was
> previously broken lets get it fixed.
> 






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

* [dpdk-dev] [PATCH v4] mbuf: allow dynamic flags to be used by secondary process
  2020-10-24  0:43 ` [dpdk-dev] [PATCH v3] mbuf: fix dynamic flags lookup from " Stephen Hemminger
  2020-10-26 10:39   ` Olivier Matz
@ 2020-11-04  5:53   ` Stephen Hemminger
  2020-11-04  8:17     ` Olivier Matz
  2020-11-04 16:20   ` [dpdk-dev] [PATCH v5] " Stephen Hemminger
  2 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2020-11-04  5:53 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, olivier.matz

The dynamic flag management is broken if rte_mbuf_dynflag_lookup()
is done in a secondary process because the local pointer to
the memzone is not ever initialized.

Fix it by using the same checks as dynfield_register().
I.e if shared memory zone has not been looked up already,
then discover it.

Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
Cc: olivier.matz@6wind.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v4 - incorporate Oliver's fix for rte_errno

 lib/librte_mbuf/rte_mbuf_dyn.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
index 538a43f6959f..b4c31896634c 100644
--- a/lib/librte_mbuf/rte_mbuf_dyn.c
+++ b/lib/librte_mbuf/rte_mbuf_dyn.c
@@ -172,7 +172,7 @@ __mbuf_dynfield_lookup(const char *name)
 			break;
 	}
 
-	if (te == NULL) {
+	if (te == NULL || mbuf_dynfield == NULL) {
 		rte_errno = ENOENT;
 		return NULL;
 	}
@@ -185,19 +185,15 @@ rte_mbuf_dynfield_lookup(const char *name, struct rte_mbuf_dynfield *params)
 {
 	struct mbuf_dynfield_elt *mbuf_dynfield;
 
-	if (shm == NULL) {
-		rte_errno = ENOENT;
-		return -1;
-	}
-
 	rte_mcfg_tailq_read_lock();
-	mbuf_dynfield = __mbuf_dynfield_lookup(name);
+	if (shm == NULL && init_shared_mem() < 0)
+		mbuf_dynfield = NULL;
+	else
+		mbuf_dynfield = __mbuf_dynfield_lookup(name);
 	rte_mcfg_tailq_read_unlock();
 
-	if (mbuf_dynfield == NULL) {
-		rte_errno = ENOENT;
+	if (mbuf_dynfield == NULL)
 		return -1;
-	}
 
 	if (params != NULL)
 		memcpy(params, &mbuf_dynfield->params, sizeof(*params));
@@ -384,13 +380,11 @@ rte_mbuf_dynflag_lookup(const char *name,
 {
 	struct mbuf_dynflag_elt *mbuf_dynflag;
 
-	if (shm == NULL) {
-		rte_errno = ENOENT;
-		return -1;
-	}
-
 	rte_mcfg_tailq_read_lock();
-	mbuf_dynflag = __mbuf_dynflag_lookup(name);
+	if (shm == NULL && init_shared_mem() < 0)
+		mbuf_dynflag = NULL;
+	else
+		mbuf_dynflag = __mbuf_dynflag_lookup(name);
 	rte_mcfg_tailq_read_unlock();
 
 	if (mbuf_dynflag == NULL) {
-- 
2.27.0


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

* Re: [dpdk-dev] [PATCH v4] mbuf: allow dynamic flags to be used by secondary process
  2020-11-04  5:53   ` [dpdk-dev] [PATCH v4] mbuf: allow dynamic flags to be used by " Stephen Hemminger
@ 2020-11-04  8:17     ` Olivier Matz
  0 siblings, 0 replies; 12+ messages in thread
From: Olivier Matz @ 2020-11-04  8:17 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

Hi Stephen,

On Tue, Nov 03, 2020 at 09:53:10PM -0800, Stephen Hemminger wrote:
> The dynamic flag management is broken if rte_mbuf_dynflag_lookup()
> is done in a secondary process because the local pointer to
> the memzone is not ever initialized.
> 
> Fix it by using the same checks as dynfield_register().
> I.e if shared memory zone has not been looked up already,
> then discover it.
> 
> Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
> Cc: olivier.matz@6wind.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v4 - incorporate Oliver's fix for rte_errno
> 
>  lib/librte_mbuf/rte_mbuf_dyn.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
> index 538a43f6959f..b4c31896634c 100644
> --- a/lib/librte_mbuf/rte_mbuf_dyn.c
> +++ b/lib/librte_mbuf/rte_mbuf_dyn.c
> @@ -172,7 +172,7 @@ __mbuf_dynfield_lookup(const char *name)
>  			break;
>  	}
>  
> -	if (te == NULL) {
> +	if (te == NULL || mbuf_dynfield == NULL) {
>  		rte_errno = ENOENT;
>  		return NULL;
>  	}
> @@ -185,19 +185,15 @@ rte_mbuf_dynfield_lookup(const char *name, struct rte_mbuf_dynfield *params)
>  {
>  	struct mbuf_dynfield_elt *mbuf_dynfield;
>  
> -	if (shm == NULL) {
> -		rte_errno = ENOENT;
> -		return -1;
> -	}
> -
>  	rte_mcfg_tailq_read_lock();
> -	mbuf_dynfield = __mbuf_dynfield_lookup(name);
> +	if (shm == NULL && init_shared_mem() < 0)
> +		mbuf_dynfield = NULL;
> +	else
> +		mbuf_dynfield = __mbuf_dynfield_lookup(name);
>  	rte_mcfg_tailq_read_unlock();
>  
> -	if (mbuf_dynfield == NULL) {
> -		rte_errno = ENOENT;
> +	if (mbuf_dynfield == NULL)
>  		return -1;
> -	}
>  
>  	if (params != NULL)
>  		memcpy(params, &mbuf_dynfield->params, sizeof(*params));
> @@ -384,13 +380,11 @@ rte_mbuf_dynflag_lookup(const char *name,
>  {
>  	struct mbuf_dynflag_elt *mbuf_dynflag;
>  
> -	if (shm == NULL) {
> -		rte_errno = ENOENT;
> -		return -1;
> -	}
> -
>  	rte_mcfg_tailq_read_lock();
> -	mbuf_dynflag = __mbuf_dynflag_lookup(name);
> +	if (shm == NULL && init_shared_mem() < 0)
> +		mbuf_dynflag = NULL;
> +	else
> +		mbuf_dynflag = __mbuf_dynflag_lookup(name);
>  	rte_mcfg_tailq_read_unlock();
>  
>  	if (mbuf_dynflag == NULL) {

The v4 change is missing for flags:

  -	if (mbuf_dynflag == NULL) {
  -		rte_errno = ENOENT;
  +	if (mbuf_dynflag == NULL)
   		return -1;
  -	}


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

* [dpdk-dev] [PATCH v5] mbuf: allow dynamic flags to be used by secondary process
  2020-10-24  0:43 ` [dpdk-dev] [PATCH v3] mbuf: fix dynamic flags lookup from " Stephen Hemminger
  2020-10-26 10:39   ` Olivier Matz
  2020-11-04  5:53   ` [dpdk-dev] [PATCH v4] mbuf: allow dynamic flags to be used by " Stephen Hemminger
@ 2020-11-04 16:20   ` Stephen Hemminger
  2020-11-04 16:26     ` Olivier Matz
  2 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2020-11-04 16:20 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, olivier.matz

The dynamic flag management is broken if rte_mbuf_dynflag_lookup()
is done in a secondary process because the local pointer to
the memzone is not ever initialized.

Fix it by using the same checks as dynfield_register().
I.e if shared memory zone has not been looked up already,
then discover it.

Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
Cc: olivier.matz@6wind.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v5 - fix additional rte_errno overwrite

 lib/librte_mbuf/rte_mbuf_dyn.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
index 538a43f6959f..c2f7220e7ba3 100644
--- a/lib/librte_mbuf/rte_mbuf_dyn.c
+++ b/lib/librte_mbuf/rte_mbuf_dyn.c
@@ -172,7 +172,7 @@ __mbuf_dynfield_lookup(const char *name)
 			break;
 	}
 
-	if (te == NULL) {
+	if (te == NULL || mbuf_dynfield == NULL) {
 		rte_errno = ENOENT;
 		return NULL;
 	}
@@ -185,19 +185,15 @@ rte_mbuf_dynfield_lookup(const char *name, struct rte_mbuf_dynfield *params)
 {
 	struct mbuf_dynfield_elt *mbuf_dynfield;
 
-	if (shm == NULL) {
-		rte_errno = ENOENT;
-		return -1;
-	}
-
 	rte_mcfg_tailq_read_lock();
-	mbuf_dynfield = __mbuf_dynfield_lookup(name);
+	if (shm == NULL && init_shared_mem() < 0)
+		mbuf_dynfield = NULL;
+	else
+		mbuf_dynfield = __mbuf_dynfield_lookup(name);
 	rte_mcfg_tailq_read_unlock();
 
-	if (mbuf_dynfield == NULL) {
-		rte_errno = ENOENT;
+	if (mbuf_dynfield == NULL)
 		return -1;
-	}
 
 	if (params != NULL)
 		memcpy(params, &mbuf_dynfield->params, sizeof(*params));
@@ -384,19 +380,15 @@ rte_mbuf_dynflag_lookup(const char *name,
 {
 	struct mbuf_dynflag_elt *mbuf_dynflag;
 
-	if (shm == NULL) {
-		rte_errno = ENOENT;
-		return -1;
-	}
-
 	rte_mcfg_tailq_read_lock();
-	mbuf_dynflag = __mbuf_dynflag_lookup(name);
+	if (shm == NULL && init_shared_mem() < 0)
+		mbuf_dynflag = NULL;
+	else
+		mbuf_dynflag = __mbuf_dynflag_lookup(name);
 	rte_mcfg_tailq_read_unlock();
 
-	if (mbuf_dynflag == NULL) {
-		rte_errno = ENOENT;
+	if (mbuf_dynflag == NULL)
 		return -1;
-	}
 
 	if (params != NULL)
 		memcpy(params, &mbuf_dynflag->params, sizeof(*params));
-- 
2.27.0


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

* Re: [dpdk-dev] [PATCH v5] mbuf: allow dynamic flags to be used by secondary process
  2020-11-04 16:20   ` [dpdk-dev] [PATCH v5] " Stephen Hemminger
@ 2020-11-04 16:26     ` Olivier Matz
  2020-11-04 17:41       ` David Marchand
  0 siblings, 1 reply; 12+ messages in thread
From: Olivier Matz @ 2020-11-04 16:26 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

On Wed, Nov 04, 2020 at 08:20:00AM -0800, Stephen Hemminger wrote:
> The dynamic flag management is broken if rte_mbuf_dynflag_lookup()
> is done in a secondary process because the local pointer to
> the memzone is not ever initialized.
> 
> Fix it by using the same checks as dynfield_register().
> I.e if shared memory zone has not been looked up already,
> then discover it.
> 
> Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
> Cc: olivier.matz@6wind.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Thanks!

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

* Re: [dpdk-dev] [PATCH v5] mbuf: allow dynamic flags to be used by secondary process
  2020-11-04 16:26     ` Olivier Matz
@ 2020-11-04 17:41       ` David Marchand
  0 siblings, 0 replies; 12+ messages in thread
From: David Marchand @ 2020-11-04 17:41 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, Olivier Matz

On Wed, Nov 4, 2020 at 5:27 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> On Wed, Nov 04, 2020 at 08:20:00AM -0800, Stephen Hemminger wrote:
> > The dynamic flag management is broken if rte_mbuf_dynflag_lookup()
> > is done in a secondary process because the local pointer to
> > the memzone is not ever initialized.
> >
> > Fix it by using the same checks as dynfield_register().
> > I.e if shared memory zone has not been looked up already,
> > then discover it.
> >
> > Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
Cc: stable@dpdk.org

> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks.

-- 
David Marchand


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

end of thread, other threads:[~2020-11-04 17:41 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-15 17:20 [dpdk-dev] [PATCH] mbuf: allow dynamic flags to be used by secondary process Stephen Hemminger
2020-10-20 12:18 ` Olivier Matz
2020-10-20 20:58 ` [dpdk-dev] [PATCH v2] " Stephen Hemminger
2020-10-24  0:43 ` [dpdk-dev] [PATCH v3] mbuf: fix dynamic flags lookup from " Stephen Hemminger
2020-10-26 10:39   ` Olivier Matz
2020-10-26 14:49     ` Stephen Hemminger
2020-11-03 21:02       ` Thomas Monjalon
2020-11-04  5:53   ` [dpdk-dev] [PATCH v4] mbuf: allow dynamic flags to be used by " Stephen Hemminger
2020-11-04  8:17     ` Olivier Matz
2020-11-04 16:20   ` [dpdk-dev] [PATCH v5] " Stephen Hemminger
2020-11-04 16:26     ` Olivier Matz
2020-11-04 17:41       ` David Marchand

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git