Re: [ovs-dev] [RFC PATCH v2 1/1] dpdk: Support both shared and per port mempools.
On 06/20/2018 01:34 PM, Ian Stokes wrote: > On 6/19/2018 12:11 PM, Kevin Traynor wrote: >> On 06/11/2018 05:37 PM, Ian Stokes wrote: >>> This commit re-introduces the concept of shared mempools as the default >>> memory model for DPDK devices. Per port mempools are still available but >>> must be enabled explicitly by a user. >>> >> >> Hi Ian, thanks for v2, comments below > > Thanks for the review Kevin, comments inline. > >> >>> OVS previously used a shared mempool model for ports with the same MTU >>> and socket configuration. This was replaced by a per port mempool model >>> to address issues flagged by users such as: >>> >>> https://mail.openvswitch.org/pipermail/ovs-discuss/2016-September/042560.html >>> >>> >>> However the per port model potentially requires an increase in memory >>> resource requirements to support the same number of ports and >>> configuration >>> as the shared port model. >>> >>> This is considered a blocking factor for current deployments of OVS >>> when upgrading to future OVS releases as a user may have to redimension >>> memory for the same deployment configuration. This may not be >>> possible for >>> users. >>> >>> This commit resolves the issue by re-introducing shared mempools as >>> the default memory behaviour in OVS DPDK but also refactors the memory >>> configuration code to allow for per port mempools. >>> >>> This patch adds a new global config option, per-port-mp, that >> >> s/per-port-mp/per-port-memory/ > > Apologies, I spotted a few of these 'per-port-mp' myself, I believe it's > in the documentation as well. I have them fixed for the next revision. > >> >>> controls the enablement of per port mempools for DPDK devices. >>> >>> ovs-vsctl set Open_vSwitch . other_config:per-port-memory=true >>> >>> This value defaults to false; to enable per port memory support, >>> this field should be set to true when setting other global parameters >>> on init (such as "dpdk-socket-mem", for example). Changing the value at >>> runtime is not supported, and requires restarting the vswitch >>> daemon. >>> >>> The mempool sweep functionality is also replaced with the >>> sweep functionality from OVS 2.9 found in commits >>> >>> c77f692 (netdev-dpdk: Free mempool only when no in-use mbufs.) >>> a7fb0a4 (netdev-dpdk: Add mempool reuse/free debug.) >>> >>> As this patch is RFC there are a number of TO-DOs including adding a >>> memory calculation section to the documentation for both models. This is >>> expected to be completed in the v1 after RFC. >>> >>> Signed-off-by: Ian Stokes >>> >>> --- >>> v1 -> v2 >>> * Rebase to head of master. >>> * Change global config option 'per-port-mp-enabled' to >>> 'per-port-memory'. >>>in commit message & documentation and code. >>> * Specify in documentation that restart of daemon is only required if >>> per >>>port-port-memory is set after dpdk-init=true. >>> * Return comment 'Contains all 'struct dpdk_mp's.' which was lost in >>>previous refactor. >>> * Improve comments regarding unique mempool names in the shared mempool >>>usecase. >>> * Check per_port_mp condition first when verifying if new mempool is >>>required in netdev_dpdk_mempool_configure() for the shared mempool >>>usecase. >>> * Correctly return ret variable instead of 0 for function >>>netdev_dpdk_mempool_configure(). >>> * Move VLOG_ERR regarding failure to create mempool for a device >>> prior to >>>dpdk_mp_create() returns. >>> * Add VLOG_DBG message flagging that the number of mbufs could not be >>>allocated and that it will be retried with half that amount. >>> * Fix indentation of VLOG_ERR in netdev_dpdk_mempool_configure(). >>> * Handle EEXIST case for per port memory in function dpdk_mp_get() to >>>avoid duplication of dpdk_mps, correctly set refcount and return >>>correct dpdk_mp for the device to use. >>> --- >>> Documentation/automake.mk| 1 + >>> Documentation/topics/dpdk/index.rst | 1 + >>> Documentation/topics/dpdk/memory.rst | 67 >>> NEWS | 1 + >>> lib/dpdk-stub.c | 6 + >>> lib/dpdk.c | 12 ++ >>> lib/dpdk.h | 1 + >>> lib/netdev-dpdk.c| 298 >>> +++ >>> vswitchd/vswitch.xml | 17 ++ >>> 9 files changed, 304 insertions(+), 100 deletions(-) >>> create mode 100644 Documentation/topics/dpdk/memory.rst >>> >>> diff --git a/Documentation/automake.mk b/Documentation/automake.mk >>> index 2202df4..141b33d 100644 >>> --- a/Documentation/automake.mk >>> +++ b/Documentation/automake.mk >>> @@ -36,6 +36,7 @@ DOC_SOURCE = \ >>> Documentation/topics/dpdk/index.rst \ >>> Documentation/topics/dpdk/bridge.rst \ >>> Documentation/topics/dpdk/jumbo-frames.rst \ >>> +Documentation/topics/dpdk/memory.rst \ >>> Documentation/topics/dpdk/pdump.rst \ >>>
Re: [ovs-dev] [RFC PATCH v2 1/1] dpdk: Support both shared and per port mempools.
On 6/19/2018 12:46 PM, Kevin Traynor wrote:
On 06/19/2018 12:16 PM, Kevin Traynor wrote:
On 06/19/2018 12:11 PM, Kevin Traynor wrote:
+if (per_port_mp && rte_errno == EEXIST) {
+LIST_FOR_EACH (next, list_node, _mp_list) {
+if (dmp->mp == next->mp) {
+rte_free(dmp);
+dmp = next;
+dmp->refcount = 1;
+}
+}
+}
+else {
+ovs_list_push_back(_mp_list, >list_node);
+}
I think you need to increment refcount and use the safe list option. How
about
Actually no, you don't need the safe list option, as it's the dmp that
is being freed
I obviously misread this (or wasn't concentrating enough), you do still
need the dmp = next as well.
Sure, I've responded in the original thread that I'll take this
approach. I'm not against using the second safe list but wanted to keep
it to 1 list if possible.
Ian
if (rte_errno == EEXIST) {
LIST_FOR_EACH_SAFE (next, list_node, _mp_list) {
if (dmp->mp == next->mp) {
next->refcount++;
rte_free(dmp);
break;
}
}
} else {
dmp->refcount++;
ovs_list_push_back(_mp_list, >list_node);
}
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC PATCH v2 1/1] dpdk: Support both shared and per port mempools.
On 6/19/2018 12:11 PM, Kevin Traynor wrote: On 06/11/2018 05:37 PM, Ian Stokes wrote: This commit re-introduces the concept of shared mempools as the default memory model for DPDK devices. Per port mempools are still available but must be enabled explicitly by a user. Hi Ian, thanks for v2, comments below Thanks for the review Kevin, comments inline. OVS previously used a shared mempool model for ports with the same MTU and socket configuration. This was replaced by a per port mempool model to address issues flagged by users such as: https://mail.openvswitch.org/pipermail/ovs-discuss/2016-September/042560.html However the per port model potentially requires an increase in memory resource requirements to support the same number of ports and configuration as the shared port model. This is considered a blocking factor for current deployments of OVS when upgrading to future OVS releases as a user may have to redimension memory for the same deployment configuration. This may not be possible for users. This commit resolves the issue by re-introducing shared mempools as the default memory behaviour in OVS DPDK but also refactors the memory configuration code to allow for per port mempools. This patch adds a new global config option, per-port-mp, that s/per-port-mp/per-port-memory/ Apologies, I spotted a few of these 'per-port-mp' myself, I believe it's in the documentation as well. I have them fixed for the next revision. controls the enablement of per port mempools for DPDK devices. ovs-vsctl set Open_vSwitch . other_config:per-port-memory=true This value defaults to false; to enable per port memory support, this field should be set to true when setting other global parameters on init (such as "dpdk-socket-mem", for example). Changing the value at runtime is not supported, and requires restarting the vswitch daemon. The mempool sweep functionality is also replaced with the sweep functionality from OVS 2.9 found in commits c77f692 (netdev-dpdk: Free mempool only when no in-use mbufs.) a7fb0a4 (netdev-dpdk: Add mempool reuse/free debug.) As this patch is RFC there are a number of TO-DOs including adding a memory calculation section to the documentation for both models. This is expected to be completed in the v1 after RFC. Signed-off-by: Ian Stokes --- v1 -> v2 * Rebase to head of master. * Change global config option 'per-port-mp-enabled' to 'per-port-memory'. in commit message & documentation and code. * Specify in documentation that restart of daemon is only required if per port-port-memory is set after dpdk-init=true. * Return comment 'Contains all 'struct dpdk_mp's.' which was lost in previous refactor. * Improve comments regarding unique mempool names in the shared mempool usecase. * Check per_port_mp condition first when verifying if new mempool is required in netdev_dpdk_mempool_configure() for the shared mempool usecase. * Correctly return ret variable instead of 0 for function netdev_dpdk_mempool_configure(). * Move VLOG_ERR regarding failure to create mempool for a device prior to dpdk_mp_create() returns. * Add VLOG_DBG message flagging that the number of mbufs could not be allocated and that it will be retried with half that amount. * Fix indentation of VLOG_ERR in netdev_dpdk_mempool_configure(). * Handle EEXIST case for per port memory in function dpdk_mp_get() to avoid duplication of dpdk_mps, correctly set refcount and return correct dpdk_mp for the device to use. --- Documentation/automake.mk| 1 + Documentation/topics/dpdk/index.rst | 1 + Documentation/topics/dpdk/memory.rst | 67 NEWS | 1 + lib/dpdk-stub.c | 6 + lib/dpdk.c | 12 ++ lib/dpdk.h | 1 + lib/netdev-dpdk.c| 298 +++ vswitchd/vswitch.xml | 17 ++ 9 files changed, 304 insertions(+), 100 deletions(-) create mode 100644 Documentation/topics/dpdk/memory.rst diff --git a/Documentation/automake.mk b/Documentation/automake.mk index 2202df4..141b33d 100644 --- a/Documentation/automake.mk +++ b/Documentation/automake.mk @@ -36,6 +36,7 @@ DOC_SOURCE = \ Documentation/topics/dpdk/index.rst \ Documentation/topics/dpdk/bridge.rst \ Documentation/topics/dpdk/jumbo-frames.rst \ + Documentation/topics/dpdk/memory.rst \ Documentation/topics/dpdk/pdump.rst \ Documentation/topics/dpdk/phy.rst \ Documentation/topics/dpdk/pmd.rst \ diff --git a/Documentation/topics/dpdk/index.rst b/Documentation/topics/dpdk/index.rst index 181f61a..cf24a7b 100644 --- a/Documentation/topics/dpdk/index.rst +++ b/Documentation/topics/dpdk/index.rst @@ -40,3 +40,4 @@ The DPDK Datapath /topics/dpdk/qos /topics/dpdk/pdump /topics/dpdk/jumbo-frames + /topics/dpdk/memory diff --git
Re: [ovs-dev] [RFC PATCH v2 1/1] dpdk: Support both shared and per port mempools.
On 06/19/2018 12:16 PM, Kevin Traynor wrote:
> On 06/19/2018 12:11 PM, Kevin Traynor wrote:
>>> +if (per_port_mp && rte_errno == EEXIST) {
>>> +LIST_FOR_EACH (next, list_node, _mp_list) {
>>> +if (dmp->mp == next->mp) {
>>> +rte_free(dmp);
>>> +dmp = next;
>>> +dmp->refcount = 1;
>>> +}
>>> +}
>>> +}
>>> +else {
>>> +ovs_list_push_back(_mp_list, >list_node);
>>> +}
>> I think you need to increment refcount and use the safe list option. How
>> about
>>
>
> Actually no, you don't need the safe list option, as it's the dmp that
> is being freed
I obviously misread this (or wasn't concentrating enough), you do still
need the dmp = next as well.
>
>> if (rte_errno == EEXIST) {
>> LIST_FOR_EACH_SAFE (next, list_node, _mp_list) {
>> if (dmp->mp == next->mp) {
>> next->refcount++;
>> rte_free(dmp);
>> break;
>> }
>> }
>> } else {
>> dmp->refcount++;
>> ovs_list_push_back(_mp_list, >list_node);
>> }
>>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC PATCH v2 1/1] dpdk: Support both shared and per port mempools.
On 06/19/2018 12:11 PM, Kevin Traynor wrote:
>> +if (per_port_mp && rte_errno == EEXIST) {
>> +LIST_FOR_EACH (next, list_node, _mp_list) {
>> +if (dmp->mp == next->mp) {
>> +rte_free(dmp);
>> +dmp = next;
>> +dmp->refcount = 1;
>> +}
>> +}
>> +}
>> +else {
>> +ovs_list_push_back(_mp_list, >list_node);
>> +}
> I think you need to increment refcount and use the safe list option. How
> about
>
Actually no, you don't need the safe list option, as it's the dmp that
is being freed
> if (rte_errno == EEXIST) {
> LIST_FOR_EACH_SAFE (next, list_node, _mp_list) {
> if (dmp->mp == next->mp) {
> next->refcount++;
> rte_free(dmp);
> break;
> }
> }
> } else {
> dmp->refcount++;
> ovs_list_push_back(_mp_list, >list_node);
> }
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC PATCH v2 1/1] dpdk: Support both shared and per port mempools.
On 06/11/2018 05:37 PM, Ian Stokes wrote: > This commit re-introduces the concept of shared mempools as the default > memory model for DPDK devices. Per port mempools are still available but > must be enabled explicitly by a user. > Hi Ian, thanks for v2, comments below > OVS previously used a shared mempool model for ports with the same MTU > and socket configuration. This was replaced by a per port mempool model > to address issues flagged by users such as: > > https://mail.openvswitch.org/pipermail/ovs-discuss/2016-September/042560.html > > However the per port model potentially requires an increase in memory > resource requirements to support the same number of ports and configuration > as the shared port model. > > This is considered a blocking factor for current deployments of OVS > when upgrading to future OVS releases as a user may have to redimension > memory for the same deployment configuration. This may not be possible for > users. > > This commit resolves the issue by re-introducing shared mempools as > the default memory behaviour in OVS DPDK but also refactors the memory > configuration code to allow for per port mempools. > > This patch adds a new global config option, per-port-mp, that s/per-port-mp/per-port-memory/ > controls the enablement of per port mempools for DPDK devices. > > ovs-vsctl set Open_vSwitch . other_config:per-port-memory=true > > This value defaults to false; to enable per port memory support, > this field should be set to true when setting other global parameters > on init (such as "dpdk-socket-mem", for example). Changing the value at > runtime is not supported, and requires restarting the vswitch > daemon. > > The mempool sweep functionality is also replaced with the > sweep functionality from OVS 2.9 found in commits > > c77f692 (netdev-dpdk: Free mempool only when no in-use mbufs.) > a7fb0a4 (netdev-dpdk: Add mempool reuse/free debug.) > > As this patch is RFC there are a number of TO-DOs including adding a > memory calculation section to the documentation for both models. This is > expected to be completed in the v1 after RFC. > > Signed-off-by: Ian Stokes > > --- > v1 -> v2 > * Rebase to head of master. > * Change global config option 'per-port-mp-enabled' to 'per-port-memory'. > in commit message & documentation and code. > * Specify in documentation that restart of daemon is only required if per > port-port-memory is set after dpdk-init=true. > * Return comment 'Contains all 'struct dpdk_mp's.' which was lost in > previous refactor. > * Improve comments regarding unique mempool names in the shared mempool > usecase. > * Check per_port_mp condition first when verifying if new mempool is > required in netdev_dpdk_mempool_configure() for the shared mempool > usecase. > * Correctly return ret variable instead of 0 for function > netdev_dpdk_mempool_configure(). > * Move VLOG_ERR regarding failure to create mempool for a device prior to > dpdk_mp_create() returns. > * Add VLOG_DBG message flagging that the number of mbufs could not be > allocated and that it will be retried with half that amount. > * Fix indentation of VLOG_ERR in netdev_dpdk_mempool_configure(). > * Handle EEXIST case for per port memory in function dpdk_mp_get() to > avoid duplication of dpdk_mps, correctly set refcount and return > correct dpdk_mp for the device to use. > --- > Documentation/automake.mk| 1 + > Documentation/topics/dpdk/index.rst | 1 + > Documentation/topics/dpdk/memory.rst | 67 > NEWS | 1 + > lib/dpdk-stub.c | 6 + > lib/dpdk.c | 12 ++ > lib/dpdk.h | 1 + > lib/netdev-dpdk.c| 298 > +++ > vswitchd/vswitch.xml | 17 ++ > 9 files changed, 304 insertions(+), 100 deletions(-) > create mode 100644 Documentation/topics/dpdk/memory.rst > > diff --git a/Documentation/automake.mk b/Documentation/automake.mk > index 2202df4..141b33d 100644 > --- a/Documentation/automake.mk > +++ b/Documentation/automake.mk > @@ -36,6 +36,7 @@ DOC_SOURCE = \ > Documentation/topics/dpdk/index.rst \ > Documentation/topics/dpdk/bridge.rst \ > Documentation/topics/dpdk/jumbo-frames.rst \ > + Documentation/topics/dpdk/memory.rst \ > Documentation/topics/dpdk/pdump.rst \ > Documentation/topics/dpdk/phy.rst \ > Documentation/topics/dpdk/pmd.rst \ > diff --git a/Documentation/topics/dpdk/index.rst > b/Documentation/topics/dpdk/index.rst > index 181f61a..cf24a7b 100644 > --- a/Documentation/topics/dpdk/index.rst > +++ b/Documentation/topics/dpdk/index.rst > @@ -40,3 +40,4 @@ The DPDK Datapath > /topics/dpdk/qos > /topics/dpdk/pdump > /topics/dpdk/jumbo-frames > + /topics/dpdk/memory > diff --git a/Documentation/topics/dpdk/memory.rst > b/Documentation/topics/dpdk/memory.rst > new file mode 100644 > index
[ovs-dev] [RFC PATCH v2 1/1] dpdk: Support both shared and per port mempools.
This commit re-introduces the concept of shared mempools as the default memory model for DPDK devices. Per port mempools are still available but must be enabled explicitly by a user. OVS previously used a shared mempool model for ports with the same MTU and socket configuration. This was replaced by a per port mempool model to address issues flagged by users such as: https://mail.openvswitch.org/pipermail/ovs-discuss/2016-September/042560.html However the per port model potentially requires an increase in memory resource requirements to support the same number of ports and configuration as the shared port model. This is considered a blocking factor for current deployments of OVS when upgrading to future OVS releases as a user may have to redimension memory for the same deployment configuration. This may not be possible for users. This commit resolves the issue by re-introducing shared mempools as the default memory behaviour in OVS DPDK but also refactors the memory configuration code to allow for per port mempools. This patch adds a new global config option, per-port-mp, that controls the enablement of per port mempools for DPDK devices. ovs-vsctl set Open_vSwitch . other_config:per-port-memory=true This value defaults to false; to enable per port memory support, this field should be set to true when setting other global parameters on init (such as "dpdk-socket-mem", for example). Changing the value at runtime is not supported, and requires restarting the vswitch daemon. The mempool sweep functionality is also replaced with the sweep functionality from OVS 2.9 found in commits c77f692 (netdev-dpdk: Free mempool only when no in-use mbufs.) a7fb0a4 (netdev-dpdk: Add mempool reuse/free debug.) As this patch is RFC there are a number of TO-DOs including adding a memory calculation section to the documentation for both models. This is expected to be completed in the v1 after RFC. Signed-off-by: Ian Stokes --- v1 -> v2 * Rebase to head of master. * Change global config option 'per-port-mp-enabled' to 'per-port-memory'. in commit message & documentation and code. * Specify in documentation that restart of daemon is only required if per port-port-memory is set after dpdk-init=true. * Return comment 'Contains all 'struct dpdk_mp's.' which was lost in previous refactor. * Improve comments regarding unique mempool names in the shared mempool usecase. * Check per_port_mp condition first when verifying if new mempool is required in netdev_dpdk_mempool_configure() for the shared mempool usecase. * Correctly return ret variable instead of 0 for function netdev_dpdk_mempool_configure(). * Move VLOG_ERR regarding failure to create mempool for a device prior to dpdk_mp_create() returns. * Add VLOG_DBG message flagging that the number of mbufs could not be allocated and that it will be retried with half that amount. * Fix indentation of VLOG_ERR in netdev_dpdk_mempool_configure(). * Handle EEXIST case for per port memory in function dpdk_mp_get() to avoid duplication of dpdk_mps, correctly set refcount and return correct dpdk_mp for the device to use. --- Documentation/automake.mk| 1 + Documentation/topics/dpdk/index.rst | 1 + Documentation/topics/dpdk/memory.rst | 67 NEWS | 1 + lib/dpdk-stub.c | 6 + lib/dpdk.c | 12 ++ lib/dpdk.h | 1 + lib/netdev-dpdk.c| 298 +++ vswitchd/vswitch.xml | 17 ++ 9 files changed, 304 insertions(+), 100 deletions(-) create mode 100644 Documentation/topics/dpdk/memory.rst diff --git a/Documentation/automake.mk b/Documentation/automake.mk index 2202df4..141b33d 100644 --- a/Documentation/automake.mk +++ b/Documentation/automake.mk @@ -36,6 +36,7 @@ DOC_SOURCE = \ Documentation/topics/dpdk/index.rst \ Documentation/topics/dpdk/bridge.rst \ Documentation/topics/dpdk/jumbo-frames.rst \ + Documentation/topics/dpdk/memory.rst \ Documentation/topics/dpdk/pdump.rst \ Documentation/topics/dpdk/phy.rst \ Documentation/topics/dpdk/pmd.rst \ diff --git a/Documentation/topics/dpdk/index.rst b/Documentation/topics/dpdk/index.rst index 181f61a..cf24a7b 100644 --- a/Documentation/topics/dpdk/index.rst +++ b/Documentation/topics/dpdk/index.rst @@ -40,3 +40,4 @@ The DPDK Datapath /topics/dpdk/qos /topics/dpdk/pdump /topics/dpdk/jumbo-frames + /topics/dpdk/memory diff --git a/Documentation/topics/dpdk/memory.rst b/Documentation/topics/dpdk/memory.rst new file mode 100644 index 000..7c00f0f --- /dev/null +++ b/Documentation/topics/dpdk/memory.rst @@ -0,0 +1,67 @@ +.. +Copyright 2018, Intel, Inc. + + Licensed under the Apache License, Version 2.0 (the "License"); you may + not use this file except in compliance with the License. You may obtain + a copy of the License
