On Thu, May 12, 2022 at 5:29 PM Kevin Traynor <[email protected]> wrote: >
Nit: title prefix should be "netdev-dpdk:" > 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* > 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]> > --- > 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? > +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. > + } > + 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* > + <p>Value should be set in the following form:</p> > + <p> > + <code>other_config:shared-mempool-config=< [snip] -- David Marchand _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
