patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH v2] mbuf: allow dynamic flags to be used by secondary process
       [not found] <20201015172019.3181-1-stephen@networkplumber.org>
@ 2020-10-20 20:58 ` Stephen Hemminger
  2020-10-24  0:43 ` [dpdk-stable] [PATCH v3] mbuf: fix dynamic flags lookup from " Stephen Hemminger
  1 sibling, 0 replies; 4+ 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] 4+ messages in thread

* [dpdk-stable] [PATCH v3] mbuf: fix dynamic flags lookup from secondary process
       [not found] <20201015172019.3181-1-stephen@networkplumber.org>
  2020-10-20 20:58 ` [dpdk-stable] [PATCH v2] mbuf: allow dynamic flags to be used by secondary process Stephen Hemminger
@ 2020-10-24  0:43 ` Stephen Hemminger
  2020-10-26 10:39   ` Olivier Matz
  1 sibling, 1 reply; 4+ 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] 4+ messages in thread

* Re: [dpdk-stable] [PATCH v3] mbuf: fix dynamic flags lookup from secondary process
  2020-10-24  0:43 ` [dpdk-stable] [PATCH v3] mbuf: fix dynamic flags lookup from " Stephen Hemminger
@ 2020-10-26 10:39   ` Olivier Matz
  2020-10-26 14:49     ` Stephen Hemminger
  0 siblings, 1 reply; 4+ 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] 4+ messages in thread

* Re: [dpdk-stable] [PATCH v3] mbuf: fix dynamic flags lookup from secondary process
  2020-10-26 10:39   ` Olivier Matz
@ 2020-10-26 14:49     ` Stephen Hemminger
  0 siblings, 0 replies; 4+ 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] 4+ messages in thread

end of thread, other threads:[~2020-10-26 14:49 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20201015172019.3181-1-stephen@networkplumber.org>
2020-10-20 20:58 ` [dpdk-stable] [PATCH v2] mbuf: allow dynamic flags to be used by secondary process Stephen Hemminger
2020-10-24  0:43 ` [dpdk-stable] [PATCH v3] mbuf: fix dynamic flags lookup from " Stephen Hemminger
2020-10-26 10:39   ` Olivier Matz
2020-10-26 14:49     ` Stephen Hemminger

patches for DPDK stable branches

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/stable/0 stable/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 stable stable/ https://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable

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


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