mbox series

[ovs-dev,v8,0/6] netdev-dpdk: Fix mempool management and other cleanup.

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

Message

Fischetti, Antonio Oct. 19, 2017, 4:54 p.m. UTC
Patch #1, #2 and #4 contain the fixes.
All other patches in this series are a clean up for code readability or small
improvements.

List of versions:
 - v8:
   - Debug message rephrased in patch #2.
   - Reworked patch #4 for snprintf error code.
   - Comments in patch #6 moved into patch #1.

 - v7:
   - Restored 2 separate patches for the 2 fixes.
       - patch #1: detect when previous mempools must be released.
       - patch #2: mempool name generation for NUMA-awareness test case.
   - patch "netdev-dpdk: manage empty mempool names." renamed as
     "netdev-dpdk: manage failure in mempool name creation."
   - Various rework based on comments.

 - v6:
   - patches #1 and #2 squashed into one.
   - Reworked to consider the latest comments.
   - tested the release of pre-existing mempools (reported by Ciara L.)
   - tested the change of MTU when an existing mempool is returned
     (reported by Robert M.)
   - tested the NUMA-Awareness usecase (reported by Ciara L.)
 - v5: manage new MTU value when a pre-existing mempool is returned.
 - v4: fix NUMA awareness usecase
 - v3: avoid deletion of pre-existing mempools
 - v2: rework to accomodate code changes for dpdk ports too
 - v1: 1st implementation.

Fischetti, Antonio (6):
  netdev-dpdk: fix management of pre-existing mempools.
  Fix mempool names to reflect socket id.
  netdev-dpdk: skip init for existing mempools.
  netdev-dpdk: manage failure in mempool name creation.
  netdev-dpdk: Reword mp_size as n_mbufs.
  netdev-dpdk: Rename dpdk_mp_put as dpdk_mp_free.

 lib/netdev-dpdk.c | 96 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 59 insertions(+), 37 deletions(-)

Comments

Stokes, Ian Oct. 19, 2017, 5:21 p.m. UTC | #1
> Patch #1, #2 and #4 contain the fixes.
> All other patches in this series are a clean up for code readability or
> small improvements.
> 

Hi Antonio, if the fixes are in patches 1 2 and 4 is there a reason they have not been grouped in a patchset and patch 1,2,3?

The other patches could be applied separately afterwards?

> List of versions:
>  - v8:
>    - Debug message rephrased in patch #2.
>    - Reworked patch #4 for snprintf error code.
>    - Comments in patch #6 moved into patch #1.
> 
>  - v7:
>    - Restored 2 separate patches for the 2 fixes.
>        - patch #1: detect when previous mempools must be released.
>        - patch #2: mempool name generation for NUMA-awareness test case.
>    - patch "netdev-dpdk: manage empty mempool names." renamed as
>      "netdev-dpdk: manage failure in mempool name creation."
>    - Various rework based on comments.
> 
>  - v6:
>    - patches #1 and #2 squashed into one.
>    - Reworked to consider the latest comments.
>    - tested the release of pre-existing mempools (reported by Ciara L.)
>    - tested the change of MTU when an existing mempool is returned
>      (reported by Robert M.)
>    - tested the NUMA-Awareness usecase (reported by Ciara L.)
>  - v5: manage new MTU value when a pre-existing mempool is returned.
>  - v4: fix NUMA awareness usecase
>  - v3: avoid deletion of pre-existing mempools
>  - v2: rework to accomodate code changes for dpdk ports too
>  - v1: 1st implementation.
> 
> Fischetti, Antonio (6):
>   netdev-dpdk: fix management of pre-existing mempools.
>   Fix mempool names to reflect socket id.
>   netdev-dpdk: skip init for existing mempools.
>   netdev-dpdk: manage failure in mempool name creation.
>   netdev-dpdk: Reword mp_size as n_mbufs.
>   netdev-dpdk: Rename dpdk_mp_put as dpdk_mp_free.
> 
>  lib/netdev-dpdk.c | 96 ++++++++++++++++++++++++++++++++++----------------
> -----
>  1 file changed, 59 insertions(+), 37 deletions(-)
> 
> --
> 2.4.11
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Fischetti, Antonio Oct. 19, 2017, 6 p.m. UTC | #2
> -----Original Message-----
> From: Stokes, Ian
> Sent: Thursday, October 19, 2017 6:22 PM
> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
> Subject: RE: [ovs-dev] [PATCH v8 0/6] netdev-dpdk: Fix mempool management and
> other cleanup.
> 
> > Patch #1, #2 and #4 contain the fixes.

[Antonio] Further to previous line: patches #1 and #2 do fix 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 

All other patches #3, #4, #5 and #6 are small improvements or just clean-up that we 
could even skip at all.
Actually patch #4 is not just a clean-up, it's managing an unlikely event that 
'might' happen (I've never seen it), that's why I promoted it as a 'fix' in the
line above.

So just the first 2 patches are needed to fix the mempool issues.


> > All other patches in this series are a clean up for code readability or
> > small improvements.
> >
> 
> Hi Antonio, if the fixes are in patches 1 2 and 4 is there a reason they have
> not been grouped in a patchset and patch 1,2,3?
> 
> The other patches could be applied separately afterwards?

[Antonio] Patches #1 and #2 are the ones needed to fix the issues.
All other patches are clean-up/small improvement, that could be optionally
applied. I think it's better to apply them in the given order.


> 
> > List of versions:
> >  - v8:
> >    - Debug message rephrased in patch #2.
> >    - Reworked patch #4 for snprintf error code.
> >    - Comments in patch #6 moved into patch #1.
> >
> >  - v7:
> >    - Restored 2 separate patches for the 2 fixes.
> >        - patch #1: detect when previous mempools must be released.
> >        - patch #2: mempool name generation for NUMA-awareness test case.
> >    - patch "netdev-dpdk: manage empty mempool names." renamed as
> >      "netdev-dpdk: manage failure in mempool name creation."
> >    - Various rework based on comments.
> >
> >  - v6:
> >    - patches #1 and #2 squashed into one.
> >    - Reworked to consider the latest comments.
> >    - tested the release of pre-existing mempools (reported by Ciara L.)
> >    - tested the change of MTU when an existing mempool is returned
> >      (reported by Robert M.)
> >    - tested the NUMA-Awareness usecase (reported by Ciara L.)
> >  - v5: manage new MTU value when a pre-existing mempool is returned.
> >  - v4: fix NUMA awareness usecase
> >  - v3: avoid deletion of pre-existing mempools
> >  - v2: rework to accomodate code changes for dpdk ports too
> >  - v1: 1st implementation.
> >
> > Fischetti, Antonio (6):
> >   netdev-dpdk: fix management of pre-existing mempools.
> >   Fix mempool names to reflect socket id.
> >   netdev-dpdk: skip init for existing mempools.
> >   netdev-dpdk: manage failure in mempool name creation.
> >   netdev-dpdk: Reword mp_size as n_mbufs.
> >   netdev-dpdk: Rename dpdk_mp_put as dpdk_mp_free.
> >
> >  lib/netdev-dpdk.c | 96 ++++++++++++++++++++++++++++++++++----------------
> > -----
> >  1 file changed, 59 insertions(+), 37 deletions(-)
> >
> > --
> > 2.4.11
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Kevin Traynor Oct. 20, 2017, 8:48 a.m. UTC | #3
On 10/19/2017 07:00 PM, Fischetti, Antonio wrote:
> 
> 
>> -----Original Message-----
>> From: Stokes, Ian
>> Sent: Thursday, October 19, 2017 6:22 PM
>> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
>> Subject: RE: [ovs-dev] [PATCH v8 0/6] netdev-dpdk: Fix mempool management and
>> other cleanup.
>>
>>> Patch #1, #2 and #4 contain the fixes.
> 
> [Antonio] Further to previous line: patches #1 and #2 do fix 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 
> 
> All other patches #3, #4, #5 and #6 are small improvements or just clean-up that we 
> could even skip at all.
> Actually patch #4 is not just a clean-up, it's managing an unlikely event that 
> 'might' happen (I've never seen it), that's why I promoted it as a 'fix' in the
> line above.
> 
> So just the first 2 patches are needed to fix the mempool issues.
> 

Agreed. Let's not hold these 2 up if there are more comments/changes
needed on the other patches, as these 2 fix commonly used functionality
that is broken.

thanks,
Kevin

> 
>>> All other patches in this series are a clean up for code readability or
>>> small improvements.
>>>
>>
>> Hi Antonio, if the fixes are in patches 1 2 and 4 is there a reason they have
>> not been grouped in a patchset and patch 1,2,3?
>>
>> The other patches could be applied separately afterwards?
> 
> [Antonio] Patches #1 and #2 are the ones needed to fix the issues.
> All other patches are clean-up/small improvement, that could be optionally
> applied. I think it's better to apply them in the given order.
> 
> 
>>
>>> List of versions:
>>>  - v8:
>>>    - Debug message rephrased in patch #2.
>>>    - Reworked patch #4 for snprintf error code.
>>>    - Comments in patch #6 moved into patch #1.
>>>
>>>  - v7:
>>>    - Restored 2 separate patches for the 2 fixes.
>>>        - patch #1: detect when previous mempools must be released.
>>>        - patch #2: mempool name generation for NUMA-awareness test case.
>>>    - patch "netdev-dpdk: manage empty mempool names." renamed as
>>>      "netdev-dpdk: manage failure in mempool name creation."
>>>    - Various rework based on comments.
>>>
>>>  - v6:
>>>    - patches #1 and #2 squashed into one.
>>>    - Reworked to consider the latest comments.
>>>    - tested the release of pre-existing mempools (reported by Ciara L.)
>>>    - tested the change of MTU when an existing mempool is returned
>>>      (reported by Robert M.)
>>>    - tested the NUMA-Awareness usecase (reported by Ciara L.)
>>>  - v5: manage new MTU value when a pre-existing mempool is returned.
>>>  - v4: fix NUMA awareness usecase
>>>  - v3: avoid deletion of pre-existing mempools
>>>  - v2: rework to accomodate code changes for dpdk ports too
>>>  - v1: 1st implementation.
>>>
>>> Fischetti, Antonio (6):
>>>   netdev-dpdk: fix management of pre-existing mempools.
>>>   Fix mempool names to reflect socket id.
>>>   netdev-dpdk: skip init for existing mempools.
>>>   netdev-dpdk: manage failure in mempool name creation.
>>>   netdev-dpdk: Reword mp_size as n_mbufs.
>>>   netdev-dpdk: Rename dpdk_mp_put as dpdk_mp_free.
>>>
>>>  lib/netdev-dpdk.c | 96 ++++++++++++++++++++++++++++++++++----------------
>>> -----
>>>  1 file changed, 59 insertions(+), 37 deletions(-)
>>>
>>> --
>>> 2.4.11
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Stokes, Ian Oct. 27, 2017, 7:53 a.m. UTC | #4
> Patch #1, #2 and #4 contain the fixes.
> All other patches in this series are a clean up for code readability or
> small improvements.
> 

Thanks for working on this Antonio and for all the reviews, all comments seem to be resolved and I've validated the patch series.

I pushed this to the dpdk_merge branch

https://github.com/istokes/ovs/tree/dpdk_merge

It will be part of the pull request this week for master.

Thanks
Ian

> List of versions:
>  - v8:
>    - Debug message rephrased in patch #2.
>    - Reworked patch #4 for snprintf error code.
>    - Comments in patch #6 moved into patch #1.
> 
>  - v7:
>    - Restored 2 separate patches for the 2 fixes.
>        - patch #1: detect when previous mempools must be released.
>        - patch #2: mempool name generation for NUMA-awareness test case.
>    - patch "netdev-dpdk: manage empty mempool names." renamed as
>      "netdev-dpdk: manage failure in mempool name creation."
>    - Various rework based on comments.
> 
>  - v6:
>    - patches #1 and #2 squashed into one.
>    - Reworked to consider the latest comments.
>    - tested the release of pre-existing mempools (reported by Ciara L.)
>    - tested the change of MTU when an existing mempool is returned
>      (reported by Robert M.)
>    - tested the NUMA-Awareness usecase (reported by Ciara L.)
>  - v5: manage new MTU value when a pre-existing mempool is returned.
>  - v4: fix NUMA awareness usecase
>  - v3: avoid deletion of pre-existing mempools
>  - v2: rework to accomodate code changes for dpdk ports too
>  - v1: 1st implementation.
> 
> Fischetti, Antonio (6):
>   netdev-dpdk: fix management of pre-existing mempools.
>   Fix mempool names to reflect socket id.
>   netdev-dpdk: skip init for existing mempools.
>   netdev-dpdk: manage failure in mempool name creation.
>   netdev-dpdk: Reword mp_size as n_mbufs.
>   netdev-dpdk: Rename dpdk_mp_put as dpdk_mp_free.
> 
>  lib/netdev-dpdk.c | 96 ++++++++++++++++++++++++++++++++++----------------
> -----
>  1 file changed, 59 insertions(+), 37 deletions(-)
> 
> --
> 2.4.11
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev