diff mbox series

[ovs-dev,v8,4/6] netdev-dpdk: manage failure in mempool name creation.

Message ID 1508432048-2265-5-git-send-email-antonio.fischetti@intel.com
State Accepted
Headers show
Series netdev-dpdk: Fix mempool management and other cleanup. | expand

Commit Message

Fischetti, Antonio Oct. 19, 2017, 4:54 p.m. UTC 
In case a mempool name could not be generated log a message
and return a null mempool pointer to the caller.

CC: Mark B Kavanagh <mark.b.kavanagh@intel.com>
CC: Darrell Ball <dlu998@gmail.com>
CC: Ciara Loftus <ciara.loftus@intel.com>
CC: Kevin Traynor <ktraynor@redhat.com>
CC: Aaron Conole <aconole@redhat.com>
Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each port.")
Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
 lib/netdev-dpdk.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Stokes, Ian Oct. 19, 2017, 5:22 p.m. UTC | #1 
> In case a mempool name could not be generated log a message and return a
> null mempool pointer to the caller.
> 

Now that I look at this, is it the case that the issues have been resolved in patches 1 and 2 but this is a clean up patch for something that might happen?


> CC: Mark B Kavanagh <mark.b.kavanagh@intel.com>
> CC: Darrell Ball <dlu998@gmail.com>
> CC: Ciara Loftus <ciara.loftus@intel.com>
> CC: Kevin Traynor <ktraynor@redhat.com>
> CC: Aaron Conole <aconole@redhat.com>
> Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each
> port.")
> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> ---
>  lib/netdev-dpdk.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index dc1e9c3..6fc6e1b
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -502,6 +502,9 @@ dpdk_mp_name(struct dpdk_mp *dmp)
>      int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
>                         h, dmp->socket_id, dmp->mtu, dmp->mp_size);
>      if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
> +        VLOG_DBG("snprintf returned %d. Failed to generate a mempool "
> +            "name for \"%s\". Hash:0x%x, mtu:%d, mbufs:%u.",
> +            ret, dmp->if_name, h, dmp->mtu, dmp->mp_size);
>          return NULL;
>      }
>      return mp_name;
> @@ -533,6 +536,10 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool
> *mp_exists)
> 
>      do {
>          char *mp_name = dpdk_mp_name(dmp);
> +        if (!mp_name) {
> +            rte_free(dmp);
> +            return NULL;
> +        }
> 
>          VLOG_DBG("Port %s: Requesting a mempool of %u mbufs "
>                    "on socket %d for %d Rx and %d Tx queues.",
> --
> 2.4.11
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Fischetti, Antonio Oct. 19, 2017, 5:58 p.m. UTC | #2 
> -----Original Message-----
> From: Stokes, Ian
> Sent: Thursday, October 19, 2017 6:23 PM
> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
> Subject: RE: [ovs-dev] [PATCH v8 4/6] netdev-dpdk: manage failure in mempool
> name creation.
> 
> > In case a mempool name could not be generated log a message and return a
> > null mempool pointer to the caller.
> >
> 
> Now that I look at this, is it the case that the issues have been resolved in
> patches 1 and 2 but this is a clean up patch for something that might happen?

[Antonio] Exactly. All the issues we saw: 
 - issue with vhostuserclient in a PVP test
 - issue of new MTU not displayed when an existing mempool is returned.
 - issue with the NUMA-Aware usecase
get resolved by patches #1 and #2.

All remaining patches are small improvements or just cosmetics.

This patch would be a small improvement to manage the unlikely event of a name creation failure that 'might' happen. 
So this has nothing to do with the issues listed above. I added this patch to the series because  - after looking at the code - I thought it would be good to track and manage an empty mempool name, just to be extra-safe.


> 
> 
> > CC: Mark B Kavanagh <mark.b.kavanagh@intel.com>
> > CC: Darrell Ball <dlu998@gmail.com>
> > CC: Ciara Loftus <ciara.loftus@intel.com>
> > CC: Kevin Traynor <ktraynor@redhat.com>
> > CC: Aaron Conole <aconole@redhat.com>
> > Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each
> > port.")
> > Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> > ---
> >  lib/netdev-dpdk.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index dc1e9c3..6fc6e1b
> > 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -502,6 +502,9 @@ dpdk_mp_name(struct dpdk_mp *dmp)
> >      int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
> >                         h, dmp->socket_id, dmp->mtu, dmp->mp_size);
> >      if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
> > +        VLOG_DBG("snprintf returned %d. Failed to generate a mempool "
> > +            "name for \"%s\". Hash:0x%x, mtu:%d, mbufs:%u.",
> > +            ret, dmp->if_name, h, dmp->mtu, dmp->mp_size);
> >          return NULL;
> >      }
> >      return mp_name;
> > @@ -533,6 +536,10 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool
> > *mp_exists)
> >
> >      do {
> >          char *mp_name = dpdk_mp_name(dmp);
> > +        if (!mp_name) {
> > +            rte_free(dmp);
> > +            return NULL;
> > +        }
> >
> >          VLOG_DBG("Port %s: Requesting a mempool of %u mbufs "
> >                    "on socket %d for %d Rx and %d Tx queues.",
> > --
> > 2.4.11
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Stokes, Ian Oct. 19, 2017, 6:16 p.m. UTC | #3 
> > -----Original Message-----
> > From: Stokes, Ian
> > Sent: Thursday, October 19, 2017 6:23 PM
> > To: Fischetti, Antonio <antonio.fischetti@intel.com>;
> > dev@openvswitch.org
> > Subject: RE: [ovs-dev] [PATCH v8 4/6] netdev-dpdk: manage failure in
> > mempool name creation.
> >
> > > In case a mempool name could not be generated log a message and
> > > return a null mempool pointer to the caller.
> > >
> >
> > Now that I look at this, is it the case that the issues have been
> > resolved in patches 1 and 2 but this is a clean up patch for something
> that might happen?
> 
> [Antonio] Exactly. All the issues we saw:
>  - issue with vhostuserclient in a PVP test
>  - issue of new MTU not displayed when an existing mempool is returned.
>  - issue with the NUMA-Aware usecase
> get resolved by patches #1 and #2.
> 
> All remaining patches are small improvements or just cosmetics.
> 
> This patch would be a small improvement to manage the unlikely event of a
> name creation failure that 'might' happen.
> So this has nothing to do with the issues listed above. I added this patch
> to the series because  - after looking at the code - I thought it would be
> good to track and manage an empty mempool name, just to be extra-safe.
> 

Ok, thanks for clarifying Antonio, I'm going to start testing on this patch series tomorrow with a view to validation. 

I think most reviewers have acked at this point but I want to flag it again to those ccd, I'm anxious to get this patch upstreamed to fix master once it has passed validation.
> 
> >
> >
> > > CC: Mark B Kavanagh <mark.b.kavanagh@intel.com>
> > > CC: Darrell Ball <dlu998@gmail.com>
> > > CC: Ciara Loftus <ciara.loftus@intel.com>
> > > CC: Kevin Traynor <ktraynor@redhat.com>
> > > CC: Aaron Conole <aconole@redhat.com>
> > > Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for
> > > each
> > > port.")
> > > Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> > > ---
> > >  lib/netdev-dpdk.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > > dc1e9c3..6fc6e1b
> > > 100644
> > > --- a/lib/netdev-dpdk.c
> > > +++ b/lib/netdev-dpdk.c
> > > @@ -502,6 +502,9 @@ dpdk_mp_name(struct dpdk_mp *dmp)
> > >      int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
> "ovs_%x_%d_%d_%u",
> > >                         h, dmp->socket_id, dmp->mtu, dmp->mp_size);
> > >      if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
> > > +        VLOG_DBG("snprintf returned %d. Failed to generate a mempool
> "
> > > +            "name for \"%s\". Hash:0x%x, mtu:%d, mbufs:%u.",
> > > +            ret, dmp->if_name, h, dmp->mtu, dmp->mp_size);
> > >          return NULL;
> > >      }
> > >      return mp_name;
> > > @@ -533,6 +536,10 @@ dpdk_mp_create(struct netdev_dpdk *dev, int
> > > mtu, bool
> > > *mp_exists)
> > >
> > >      do {
> > >          char *mp_name = dpdk_mp_name(dmp);
> > > +        if (!mp_name) {
> > > +            rte_free(dmp);
> > > +            return NULL;
> > > +        }
> > >
> > >          VLOG_DBG("Port %s: Requesting a mempool of %u mbufs "
> > >                    "on socket %d for %d Rx and %d Tx queues.",
> > > --
> > > 2.4.11
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Kevin Traynor Oct. 20, 2017, 8:43 a.m. UTC | #4 
On 10/19/2017 05:54 PM, antonio.fischetti@intel.com wrote:
> In case a mempool name could not be generated log a message
> and return a null mempool pointer to the caller.
> 
> CC: Mark B Kavanagh <mark.b.kavanagh@intel.com>
> CC: Darrell Ball <dlu998@gmail.com>
> CC: Ciara Loftus <ciara.loftus@intel.com>
> CC: Kevin Traynor <ktraynor@redhat.com>
> CC: Aaron Conole <aconole@redhat.com>
> Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each port.")
> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> ---

Acked-by: Kevin Traynor <ktraynor@redhat.com>

>  lib/netdev-dpdk.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index dc1e9c3..6fc6e1b 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -502,6 +502,9 @@ dpdk_mp_name(struct dpdk_mp *dmp)
>      int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
>                         h, dmp->socket_id, dmp->mtu, dmp->mp_size);
>      if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
> +        VLOG_DBG("snprintf returned %d. Failed to generate a mempool "
> +            "name for \"%s\". Hash:0x%x, mtu:%d, mbufs:%u.",
> +            ret, dmp->if_name, h, dmp->mtu, dmp->mp_size);
>          return NULL;
>      }
>      return mp_name;
> @@ -533,6 +536,10 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool *mp_exists)
>  
>      do {
>          char *mp_name = dpdk_mp_name(dmp);
> +        if (!mp_name) {
> +            rte_free(dmp);
> +            return NULL;
> +        }
>  
>          VLOG_DBG("Port %s: Requesting a mempool of %u mbufs "
>                    "on socket %d for %d Rx and %d Tx queues.",
>
Mark Kavanagh Oct. 20, 2017, 9:28 a.m. UTC | #5 
>From: Fischetti, Antonio
>Sent: Thursday, October 19, 2017 5:54 PM
>To: dev@openvswitch.org
>Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Darrell Ball
><dlu998@gmail.com>; Loftus, Ciara <ciara.loftus@intel.com>; Kevin Traynor
><ktraynor@redhat.com>; Aaron Conole <aconole@redhat.com>; Fischetti, Antonio
><antonio.fischetti@intel.com>
>Subject: [PATCH v8 4/6] netdev-dpdk: manage failure in mempool name creation.
>
>In case a mempool name could not be generated log a message
>and return a null mempool pointer to the caller.
>
>CC: Mark B Kavanagh <mark.b.kavanagh@intel.com>
>CC: Darrell Ball <dlu998@gmail.com>
>CC: Ciara Loftus <ciara.loftus@intel.com>
>CC: Kevin Traynor <ktraynor@redhat.com>
>CC: Aaron Conole <aconole@redhat.com>
>Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each
>port.")
>Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
>---
> lib/netdev-dpdk.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>index dc1e9c3..6fc6e1b 100644
>--- a/lib/netdev-dpdk.c
>+++ b/lib/netdev-dpdk.c
>@@ -502,6 +502,9 @@ dpdk_mp_name(struct dpdk_mp *dmp)
>     int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
>                        h, dmp->socket_id, dmp->mtu, dmp->mp_size);
>     if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
>+        VLOG_DBG("snprintf returned %d. Failed to generate a mempool "
>+            "name for \"%s\". Hash:0x%x, mtu:%d, mbufs:%u.",
>+            ret, dmp->if_name, h, dmp->mtu, dmp->mp_size);
>         return NULL;
>     }
>     return mp_name;
>@@ -533,6 +536,10 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool
>*mp_exists)
>
>     do {
>         char *mp_name = dpdk_mp_name(dmp);
>+        if (!mp_name) {
>+            rte_free(dmp);
>+            return NULL;
>+        }


This is a fix, and as such, needs to include 'Fixes: <commit ID>" in the commit message.

I believe that Kevin made the same comment on this patch in v7.

Thanks,
Mark
 
>
>         VLOG_DBG("Port %s: Requesting a mempool of %u mbufs "
>                   "on socket %d for %d Rx and %d Tx queues.",
>--
>2.4.11
Fischetti, Antonio Oct. 20, 2017, 9:37 a.m. UTC | #6 
> -----Original Message-----
> From: Kavanagh, Mark B
> Sent: Friday, October 20, 2017 10:28 AM
> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
> Cc: Darrell Ball <dlu998@gmail.com>; Loftus, Ciara <ciara.loftus@intel.com>;
> Kevin Traynor <ktraynor@redhat.com>; Aaron Conole <aconole@redhat.com>
> Subject: RE: [PATCH v8 4/6] netdev-dpdk: manage failure in mempool name
> creation.
> 
> >From: Fischetti, Antonio
> >Sent: Thursday, October 19, 2017 5:54 PM
> >To: dev@openvswitch.org
> >Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Darrell Ball
> ><dlu998@gmail.com>; Loftus, Ciara <ciara.loftus@intel.com>; Kevin Traynor
> ><ktraynor@redhat.com>; Aaron Conole <aconole@redhat.com>; Fischetti, Antonio
> ><antonio.fischetti@intel.com>
> >Subject: [PATCH v8 4/6] netdev-dpdk: manage failure in mempool name creation.
> >
> >In case a mempool name could not be generated log a message
> >and return a null mempool pointer to the caller.
> >
> >CC: Mark B Kavanagh <mark.b.kavanagh@intel.com>
> >CC: Darrell Ball <dlu998@gmail.com>
> >CC: Ciara Loftus <ciara.loftus@intel.com>
> >CC: Kevin Traynor <ktraynor@redhat.com>
> >CC: Aaron Conole <aconole@redhat.com>
> >Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each
> >port.")
> >Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> >---
> > lib/netdev-dpdk.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> >index dc1e9c3..6fc6e1b 100644
> >--- a/lib/netdev-dpdk.c
> >+++ b/lib/netdev-dpdk.c
> >@@ -502,6 +502,9 @@ dpdk_mp_name(struct dpdk_mp *dmp)
> >     int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
> >                        h, dmp->socket_id, dmp->mtu, dmp->mp_size);
> >     if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
> >+        VLOG_DBG("snprintf returned %d. Failed to generate a mempool "
> >+            "name for \"%s\". Hash:0x%x, mtu:%d, mbufs:%u.",
> >+            ret, dmp->if_name, h, dmp->mtu, dmp->mp_size);
> >         return NULL;
> >     }
> >     return mp_name;
> >@@ -533,6 +536,10 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool
> >*mp_exists)
> >
> >     do {
> >         char *mp_name = dpdk_mp_name(dmp);
> >+        if (!mp_name) {
> >+            rte_free(dmp);
> >+            return NULL;
> >+        }
> 
> 
> This is a fix, and as such, needs to include 'Fixes: <commit ID>" in the commit
> message.
> 
> I believe that Kevin made the same comment on this patch in v7.

[Antonio] Actually I added 
   Fixes: d555d9bded5f ("netdev-dpdk: Create separate...
right before the Signed-off-by line.
Is that what you meant?


> 
> Thanks,
> Mark
> 
> >
> >         VLOG_DBG("Port %s: Requesting a mempool of %u mbufs "
> >                   "on socket %d for %d Rx and %d Tx queues.",
> >--
> >2.4.11
Mark Kavanagh Oct. 20, 2017, 9:41 a.m. UTC | #7 
>From: Fischetti, Antonio
>Sent: Friday, October 20, 2017 10:38 AM
>To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@openvswitch.org
>Cc: Darrell Ball <dlu998@gmail.com>; Loftus, Ciara <ciara.loftus@intel.com>;
>Kevin Traynor <ktraynor@redhat.com>; Aaron Conole <aconole@redhat.com>
>Subject: RE: [PATCH v8 4/6] netdev-dpdk: manage failure in mempool name
>creation.
>
>
>
>> -----Original Message-----
>> From: Kavanagh, Mark B
>> Sent: Friday, October 20, 2017 10:28 AM
>> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
>> Cc: Darrell Ball <dlu998@gmail.com>; Loftus, Ciara <ciara.loftus@intel.com>;
>> Kevin Traynor <ktraynor@redhat.com>; Aaron Conole <aconole@redhat.com>
>> Subject: RE: [PATCH v8 4/6] netdev-dpdk: manage failure in mempool name
>> creation.
>>
>> >From: Fischetti, Antonio
>> >Sent: Thursday, October 19, 2017 5:54 PM
>> >To: dev@openvswitch.org
>> >Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Darrell Ball
>> ><dlu998@gmail.com>; Loftus, Ciara <ciara.loftus@intel.com>; Kevin Traynor
>> ><ktraynor@redhat.com>; Aaron Conole <aconole@redhat.com>; Fischetti,
>Antonio
>> ><antonio.fischetti@intel.com>
>> >Subject: [PATCH v8 4/6] netdev-dpdk: manage failure in mempool name
>creation.
>> >
>> >In case a mempool name could not be generated log a message
>> >and return a null mempool pointer to the caller.
>> >
>> >CC: Mark B Kavanagh <mark.b.kavanagh@intel.com>
>> >CC: Darrell Ball <dlu998@gmail.com>
>> >CC: Ciara Loftus <ciara.loftus@intel.com>
>> >CC: Kevin Traynor <ktraynor@redhat.com>
>> >CC: Aaron Conole <aconole@redhat.com>
>> >Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each
>> >port.")
>> >Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
>> >---
>> > lib/netdev-dpdk.c | 7 +++++++
>> > 1 file changed, 7 insertions(+)
>> >
>> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> >index dc1e9c3..6fc6e1b 100644
>> >--- a/lib/netdev-dpdk.c
>> >+++ b/lib/netdev-dpdk.c
>> >@@ -502,6 +502,9 @@ dpdk_mp_name(struct dpdk_mp *dmp)
>> >     int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
>> >                        h, dmp->socket_id, dmp->mtu, dmp->mp_size);
>> >     if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
>> >+        VLOG_DBG("snprintf returned %d. Failed to generate a mempool "
>> >+            "name for \"%s\". Hash:0x%x, mtu:%d, mbufs:%u.",
>> >+            ret, dmp->if_name, h, dmp->mtu, dmp->mp_size);
>> >         return NULL;
>> >     }
>> >     return mp_name;
>> >@@ -533,6 +536,10 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool
>> >*mp_exists)
>> >
>> >     do {
>> >         char *mp_name = dpdk_mp_name(dmp);
>> >+        if (!mp_name) {
>> >+            rte_free(dmp);
>> >+            return NULL;
>> >+        }
>>
>>
>> This is a fix, and as such, needs to include 'Fixes: <commit ID>" in the
>commit
>> message.
>>
>> I believe that Kevin made the same comment on this patch in v7.
>
>[Antonio] Actually I added
>   Fixes: d555d9bded5f ("netdev-dpdk: Create separate...
>right before the Signed-off-by line.
>Is that what you meant?

Perfect - I actually hadn't spotted that.

With that: Acked-by: Mark Kavanagh <mark.b.kavanagh@intel.comn>

Now, it seems that I really do need to go get that coffee...  ;)

>
>
>>
>> Thanks,
>> Mark
>>
>> >
>> >         VLOG_DBG("Port %s: Requesting a mempool of %u mbufs "
>> >                   "on socket %d for %d Rx and %d Tx queues.",
>> >--
>> >2.4.11
Darrell Ball Oct. 27, 2017, 2:19 a.m. UTC | #8 
On 10/19/17, 9:56 AM, "ovs-dev-bounces@openvswitch.org on behalf of antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf of antonio.fischetti@intel.com> wrote:

    In case a mempool name could not be generated log a message
    and return a null mempool pointer to the caller.
    
    CC: Mark B Kavanagh <mark.b.kavanagh@intel.com>
    CC: Darrell Ball <dlu998@gmail.com>
    CC: Ciara Loftus <ciara.loftus@intel.com>
    CC: Kevin Traynor <ktraynor@redhat.com>
    CC: Aaron Conole <aconole@redhat.com>
    Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each port.")
    Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>

    ---
     lib/netdev-dpdk.c | 7 +++++++
     1 file changed, 7 insertions(+)
    
    diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
    index dc1e9c3..6fc6e1b 100644
    --- a/lib/netdev-dpdk.c
    +++ b/lib/netdev-dpdk.c
    @@ -502,6 +502,9 @@ dpdk_mp_name(struct dpdk_mp *dmp)
         int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
                            h, dmp->socket_id, dmp->mtu, dmp->mp_size);
         if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {

I see you copied me on an earlier version, so I’ll add one comment.
Given MTU size restriction for dpdk and rte max queues and max buf desc enforcement, can this condition be met ?
I think there are 11 remaining characters (out of 31) for mp_size and the calculation is 

dmp->mp_size = dev->requested_n_rxq * dev->requested_rxq_size
            + dev->requested_n_txq * dev->requested_txq_size
            + MIN(RTE_MAX_LCORE, dev->requested_n_rxq) * NETDEV_MAX_BURST
            + MIN_NB_MBUF;

I got 9 decimal digits max, assuming my math is correct…

Previous filtering for mtu (netdev_dpdk_set_mtu) and max queues (dpdk_eth_dev_init)/max buf desc (dpdk_process_queue_size)
enforcement (present and also any future design) should prevent snprintf from having an unexpected return value number of chars, which would
mean snprintf should only have an unexpected return value number of chars, if a future coding error were to be introduced.
FYI, OVS traditionally deals with other such cases with ovs_assert(), not a VLOG_DBG, since it is easier to find the bug earlier.
I believe snprintf at this level should not be catching user config errors.



    +        VLOG_DBG("snprintf returned %d. Failed to generate a mempool "
    +            "name for \"%s\". Hash:0x%x, mtu:%d, mbufs:%u.",
    +            ret, dmp->if_name, h, dmp->mtu, dmp->mp_size);
             return NULL;
         }
         return mp_name;
    @@ -533,6 +536,10 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool *mp_exists)
     
         do {
             char *mp_name = dpdk_mp_name(dmp);
    +        if (!mp_name) {
    +            rte_free(dmp);
    +            return NULL;
    +        }
     
             VLOG_DBG("Port %s: Requesting a mempool of %u mbufs "
                       "on socket %d for %d Rx and %d Tx queues.",
    -- 
    2.4.11
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=cKHgut93ZGYQzoLXpXQWPqcZhBX3NMdQaRnbNlfvhiU&s=tx4JYNmCFZ-vgGQzTIteThFJN2RSlMtqg34oTwlFEF0&e=
diff mbox series

Patch

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index dc1e9c3..6fc6e1b 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -502,6 +502,9 @@  dpdk_mp_name(struct dpdk_mp *dmp)
     int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
                        h, dmp->socket_id, dmp->mtu, dmp->mp_size);
     if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
+        VLOG_DBG("snprintf returned %d. Failed to generate a mempool "
+            "name for \"%s\". Hash:0x%x, mtu:%d, mbufs:%u.",
+            ret, dmp->if_name, h, dmp->mtu, dmp->mp_size);
         return NULL;
     }
     return mp_name;
@@ -533,6 +536,10 @@  dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool *mp_exists)
 
     do {
         char *mp_name = dpdk_mp_name(dmp);
+        if (!mp_name) {
+            rte_free(dmp);
+            return NULL;
+        }
 
         VLOG_DBG("Port %s: Requesting a mempool of %u mbufs "
                   "on socket %d for %d Rx and %d Tx queues.",