[ovs-dev,v5,1/3] netdev-dpdk: Add shared mempool config.

Mempools may currently be 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 <ktraynor@redhat.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
---
 Documentation/topics/dpdk/memory.rst |  44 +++++++++++
 NEWS                                 |   3 +
 lib/dpdk.c                           |   2 +-
 lib/netdev-dpdk.c                    | 109 ++++++++++++++++++++++++++-
 lib/netdev-dpdk.h                    |   5 +-
 vswitchd/vswitch.xml                 |  37 +++++++++
 6 files changed, 195 insertions(+), 5 deletions(-)

Hi Kevin, 

Thanks for the patch. 
LGTM in general, couple of minor comments/thoughts inline.

<snipped>

> +struct user_mempool_config {
> +    int adj_mtu;
> +    int socket_id;
> +};
> +
> +static struct user_mempool_config *user_mempools = NULL; static int
> +n_user_mempools;

Should we consider restricting n_user_mempools to some max value ?

> 
>  /* There should be one 'struct dpdk_tx_queue' created for @@ -573,4
> +582,42 @@ dpdk_buf_size(int mtu)  }
> 
> +static int
> +dpdk_get_user_adjusted_mtu(int port_adj_mtu, int port_mtu, int
> +port_socket_id) {
> +    int best_adj_user_mtu = INT_MAX;
> +
> +    for (unsigned i = 0; i < n_user_mempools; i++) {
> +        int user_adj_mtu, user_socket_id;
> +
> +        user_adj_mtu = user_mempools[i].adj_mtu;
> +        user_socket_id = user_mempools[i].socket_id;
> +        if (port_adj_mtu > user_adj_mtu
> +            || (user_socket_id != INT_MAX
> +                && user_socket_id != port_socket_id)) {
> +            continue;
> +        }
> +        if (user_adj_mtu < best_adj_user_mtu) {
> +            /* This is the is the lowest valid user MTU. */
> +            best_adj_user_mtu = user_adj_mtu;
> +            if (best_adj_user_mtu == port_adj_mtu) {
> +                /* Found an exact fit, no need to keep searching. */
> +                break;
> +            }
> +        }


I guess this is more appropriate than sorting etc to find lowest valid user MTU since we assume n_user_mempools will be quite small ?


> +    }
> +    if (best_adj_user_mtu == INT_MAX) {
> +        VLOG_DBG("No user configured shared mempool mbuf sizes found "
> +                 "suitable for port with MTU %d, NUMA %d.", port_mtu,
> +                 port_socket_id);
> +        best_adj_user_mtu = port_adj_mtu;
> +    } else {
> +        VLOG_DBG("Found user configured shared mempool with mbufs "
> +                 "of size %d, suitable for port with MTU %d, NUMA %d.",
> +                 MTU_TO_FRAME_LEN(best_adj_user_mtu), port_mtu,
> +                 port_socket_id);
> +    }
> +    return best_adj_user_mtu;
> +}
> +

<snipped>

Thanks and regards,
Sunil

On Fri, Jul 8, 2022 at 10:18 PM Pai G, Sunil <sunil.pai.g@intel.com> wrote:
>
> Hi Kevin,
>

Hi Sunil,

thanks for reviewing the patches.

> Thanks for the patch.
> LGTM in general, couple of minor comments/thoughts inline.
>
> <snipped>
>
> > +struct user_mempool_config {
> > +    int adj_mtu;
> > +    int socket_id;
> > +};
> > +
> > +static struct user_mempool_config *user_mempools = NULL; static int
> > +n_user_mempools;
>
> Should we consider restricting n_user_mempools to some max value ?
>

We could do. There is no issue with having many values, it just might
not make much sense for a user.

Is your concern that someone might maliciously keep adding values?
I'm not sure if there is some protection already within the smap_get
that would prevent this. i'd need to check that.
If there is, then that would be covered already. Otherwise we could
add some arbitrary max number like 100.

> >
> >  /* There should be one 'struct dpdk_tx_queue' created for @@ -573,4
> > +582,42 @@ dpdk_buf_size(int mtu)  }
> >
> > +static int
> > +dpdk_get_user_adjusted_mtu(int port_adj_mtu, int port_mtu, int
> > +port_socket_id) {
> > +    int best_adj_user_mtu = INT_MAX;
> > +
> > +    for (unsigned i = 0; i < n_user_mempools; i++) {
> > +        int user_adj_mtu, user_socket_id;
> > +
> > +        user_adj_mtu = user_mempools[i].adj_mtu;
> > +        user_socket_id = user_mempools[i].socket_id;
> > +        if (port_adj_mtu > user_adj_mtu
> > +            || (user_socket_id != INT_MAX
> > +                && user_socket_id != port_socket_id)) {
> > +            continue;
> > +        }
> > +        if (user_adj_mtu < best_adj_user_mtu) {
> > +            /* This is the is the lowest valid user MTU. */
> > +            best_adj_user_mtu = user_adj_mtu;
> > +            if (best_adj_user_mtu == port_adj_mtu) {
> > +                /* Found an exact fit, no need to keep searching. */
> > +                break;
> > +            }
> > +        }
>
>
> I guess this is more appropriate than sorting etc to find lowest valid user MTU since we assume n_user_mempools will be quite small ?
>

It is relying on MTU *and* socket id (which may be wildcard/any NUMA),
so we couldn't sort it as a flat list.
There would need to be multiple lists created for every numa etc

There is no need to go to that trouble to speed up the look up when we
expect a few entries and we only look this up on the control path
when we are changing MTU. So I think it's fine the way it is.

Kevin.

> > +    }
> > +    if (best_adj_user_mtu == INT_MAX) {
> > +        VLOG_DBG("No user configured shared mempool mbuf sizes found "
> > +                 "suitable for port with MTU %d, NUMA %d.", port_mtu,
> > +                 port_socket_id);
> > +        best_adj_user_mtu = port_adj_mtu;
> > +    } else {
> > +        VLOG_DBG("Found user configured shared mempool with mbufs "
> > +                 "of size %d, suitable for port with MTU %d, NUMA %d.",
> > +                 MTU_TO_FRAME_LEN(best_adj_user_mtu), port_mtu,
> > +                 port_socket_id);
> > +    }
> > +    return best_adj_user_mtu;
> > +}
> > +
>
> <snipped>
>
> Thanks and regards,
> Sunil
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Hi Kevin, 

Responses inline.

> > > +static struct user_mempool_config *user_mempools = NULL; static int
> > > +n_user_mempools;
> >
> > Should we consider restricting n_user_mempools to some max value ?
> >
> 
> We could do. There is no issue with having many values, it just might not
> make much sense for a user.
> 
> Is your concern that someone might maliciously keep adding values?

Yes, this was my concern. 

> I'm not sure if there is some protection already within the smap_get that
> would prevent this. i'd need to check that.
> If there is, then that would be covered already. Otherwise we could add
> some arbitrary max number like 100.

Sure, sounds good.
Otherwise, the patch looks good. I'll be happy to ack!

> 
> > >
> > >  /* There should be one 'struct dpdk_tx_queue' created for @@ -573,4
> > > +582,42 @@ dpdk_buf_size(int mtu)  }
> > >
> > > +static int
> > > +dpdk_get_user_adjusted_mtu(int port_adj_mtu, int port_mtu, int
> > > +port_socket_id) {
> > > +    int best_adj_user_mtu = INT_MAX;
> > > +
> > > +    for (unsigned i = 0; i < n_user_mempools; i++) {
> > > +        int user_adj_mtu, user_socket_id;
> > > +
> > > +        user_adj_mtu = user_mempools[i].adj_mtu;
> > > +        user_socket_id = user_mempools[i].socket_id;
> > > +        if (port_adj_mtu > user_adj_mtu
> > > +            || (user_socket_id != INT_MAX
> > > +                && user_socket_id != port_socket_id)) {
> > > +            continue;
> > > +        }
> > > +        if (user_adj_mtu < best_adj_user_mtu) {
> > > +            /* This is the is the lowest valid user MTU. */
> > > +            best_adj_user_mtu = user_adj_mtu;
> > > +            if (best_adj_user_mtu == port_adj_mtu) {
> > > +                /* Found an exact fit, no need to keep searching. */
> > > +                break;
> > > +            }
> > > +        }
> >
> >
> > I guess this is more appropriate than sorting etc to find lowest valid
> user MTU since we assume n_user_mempools will be quite small ?
> >
> 
> It is relying on MTU *and* socket id (which may be wildcard/any NUMA), so
> we couldn't sort it as a flat list.
> There would need to be multiple lists created for every numa etc
> 
> There is no need to go to that trouble to speed up the look up when we
> expect a few entries and we only look this up on the control path when we
> are changing MTU. So I think it's fine the way it is.
> 
> Kevin.

I agree with your assessment Kevin 😊

> 
> > > +    }
> > > +    if (best_adj_user_mtu == INT_MAX) {
> > > +        VLOG_DBG("No user configured shared mempool mbuf sizes found
> "
> > > +                 "suitable for port with MTU %d, NUMA %d.", port_mtu,
> > > +                 port_socket_id);
> > > +        best_adj_user_mtu = port_adj_mtu;
> > > +    } else {
> > > +        VLOG_DBG("Found user configured shared mempool with mbufs "
> > > +                 "of size %d, suitable for port with MTU %d, NUMA
> %d.",
> > > +                 MTU_TO_FRAME_LEN(best_adj_user_mtu), port_mtu,
> > > +                 port_socket_id);
> > > +    }
> > > +    return best_adj_user_mtu;
> > > +}
> > > +
> >
> > <snipped>
> >
> > Thanks and regards,
> > Sunil
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev