On 24/05/2022 12:14, David Marchand wrote:
On Thu, May 12, 2022 at 5:29 PM Kevin Traynor <[email protected]> wrote:
Nit: title prefix should be "netdev-dpdk:"
Ah, good one, fixed in v4.
Shared memory mempools may be currently be shared between DPDK
There is a lot of "shared" and "memory" in this line :-).
How about:
Mempools may be currently shared between DPDK*
Agree that it sounds better, changed for this.
ports based on port MTU and NUMA. With some hint from the user
we can increase the sharing on MTU and hence reduce memory
consumption in many cases.
For example, a port with MTU 9000, uses a mempool with an
mbuf size based on 9000 MTU. A port with MTU 1500, uses a
different mempool with an mbuf size based on 1500 MTU.
In this case, assuming same NUMA, both these ports could
share the 9000 MTU mempool.
The user must give a hint as order of creation of ports and
setting of MTUs may vary and we need to ensure that upgrades
from older OVS versions do not require more memory.
This scheme can also prevent multiple mempools being created
for cases where a port is added picking up a default MTU and
an appropriate mempool, but later has it's MTU changed to a
different value requiring a different mempool.
Example usage:
$ ovs-vsctl --no-wait set Open_vSwitch . \
other_config:shared-mempool-config=9000,1500:1,6000:1
Port added on NUMA 0:
* MTU 1500, use mempool based on 9000 MTU
* MTU 5000, use mempool based on 9000 MTU
* MTU 9000, use mempool based on 9000 MTU
* MTU 9300, use mempool based on 9300 MTU (existing behaviour)
Port added on NUMA 1:
* MTU 1500, use mempool based on 1500 MTU
* MTU 5000, use mempool based on 6000 MTU
* MTU 9000, use mempool based on 9000 MTU
* MTU 9300, use mempool based on 9300 MTU (existing behaviour)
Default behaviour is unchanged and mempools are still only created
when needed.
Signed-off-by: Kevin Traynor <[email protected]>
This lgtm, I have some small nits below, but otherwise:
Reviewed-by: David Marchand <[email protected]>
Thanks for the reviews.
---
Documentation/topics/dpdk/memory.rst | 44 +++++++++++
lib/dpdk.c | 2 +-
lib/netdev-dpdk.c | 108 ++++++++++++++++++++++++++-
lib/netdev-dpdk.h | 5 +-
vswitchd/vswitch.xml | 37 +++++++++
5 files changed, 191 insertions(+), 5 deletions(-)
diff --git a/Documentation/topics/dpdk/memory.rst
b/Documentation/topics/dpdk/memory.rst
index 8b7758e6e..aa2dfce9c 100644
--- a/Documentation/topics/dpdk/memory.rst
+++ b/Documentation/topics/dpdk/memory.rst
@@ -214,2 +214,46 @@ Example 3: (2 rxq, 2 PMD, 9000 MTU)
Mbuf size = 10176 Bytes
Memory required = 26656 * 10176 = 271 MB
+
+Shared Mempool Configuration
+----------------------------
+
+In order to increase sharing of mempools, a user can configure the MTUs which
+mempools are based on by using ``shared-mempool-config``.
+
+A MTU configured by the user is adjusted to an mbuf size used for mempool
Nit: a* mbuf size?
"an mbuf" is correct by weird English rules - but now that you raise
this, it should be "An MTU.." too, so fixed that one.
+creation and stored. If a port is subsequently added that has an MTU which can
+be accommodated by this mbuf size, it will be used for mempool creation/reuse.
+
[snip]
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index f9535bfb4..5076158f0 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
[snip]
@@ -5335,4 +5386,53 @@ netdev_dpdk_rte_flow_tunnel_item_release(struct netdev
*netdev,
#endif /* ALLOW_EXPERIMENTAL_API */
+static void
+parse_user_mempools_list(const char *mtus)
+{
+ char *list, *copy, *key, *value;
+ int error = 0;
+
+ if (!mtus) {
+ return;
+ }
+
+ n_user_mempools = 0;
+ list = copy = xstrdup(mtus);
+
+ while (ofputil_parse_key_value(&list, &key, &value)) {
+ int socket_id, mtu, adj_mtu;
+
+ if (!str_to_int(key, 0, &mtu) || mtu < 0) {
+ error = EINVAL;
+ VLOG_WARN("Invalid user configured shared mempool MTU.");
+ break;
+ }
+
+ if (!str_to_int(value, 0, &socket_id)) {
+ /* No socket specified. It will apply for all numas. */
+ socket_id = INT_MAX;
+ } else if (socket_id < 0) {
+ error = EINVAL;
+ VLOG_WARN("Invalid user configured shared mempool NUMA.");
+ break;
+ }
+
+ user_mempools = xrealloc(user_mempools, (n_user_mempools + 1) *
+ sizeof(struct user_mempool_config));
+ adj_mtu = FRAME_LEN_TO_MTU(dpdk_buf_size(mtu));
+ user_mempools[n_user_mempools].adj_mtu = adj_mtu;
+ user_mempools[n_user_mempools].socket_id = socket_id;
+ n_user_mempools++;
+ VLOG_INFO("User configured shared mempool set for: MTU %d, NUMA %s.",
+ mtu, socket_id == INT_MAX ? "ALL" : value);
+ }
+
+ if (error) {
+ VLOG_WARN("User configured shared mempools will not be used.");
+ n_user_mempools = 0;
+ free(user_mempools);
Nit: we won't call this code again, and n_user_mempools == 0 protects
against access to user_mempools.
But, for consistency, I'd rather set user_mempools back to NULL.
sounds good, changed to do that.
+ }
+ free(copy);
+}
[snip]
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index cc1dd77ec..e9a5003ec 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -491,4 +491,41 @@
</column>
+ <column name="other_config" key="shared-mempool-config">
+ <p>Specifies dpdk shared memory mempool config.</p>
Nit: shared mempool*
changed to this.
+ <p>Value should be set in the following form:</p>
+ <p>
+ <code>other_config:shared-mempool-config=<
[snip]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
