[ovs-dev] [branch-2.9 PATCH 1/2] netdev-dpdk: Free mempool only when no in-use mbufs.

Stokes, Ian ian.stokes at intel.com
Thu Apr 5 13:37:48 UTC 2018


> DPDK mempools are freed when they are no longer needed.
> This can happen when a port is removed or a port's mtu is reconfigured so
> that a new mempool is used.
> 
> It is possible that an mbuf is attempted to be returned to a freed mempool
> from NIC Tx queues and this can lead to a segfault.
> 
> In order to prevent this, only free mempools when they are not needed and
> have no in-use mbufs. As this might not be possible immediately, sweep the
> mempools anytime a port tries to get a mempool.
> 

Thanks for working on this Kevin. Just a query below. From a testing POV I didn't come across any issues.

> Fixes: 8d38823bdf8b ("netdev-dpdk: fix memory leak")
> Cc: mark.b.kavanagh81 at gmail.com
> Signed-off-by: Kevin Traynor <ktraynor at redhat.com>
> ---
> 
> Found on OVS 2.6, but code is operationally similar on recent the branch-
> 2.*'s. If the patch is acceptable I can backport to older branches.

Sounds good to me.
> 
>  lib/netdev-dpdk.c | 40 +++++++++++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index c19cedc..9b8e732
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -590,8 +590,24 @@ dpdk_mp_create(int socket_id, int mtu)  }
> 
> +/* Free unused mempools. */
> +static void
> +dpdk_mp_sweep(void) OVS_REQUIRES(dpdk_mp_mutex) {
> +    struct dpdk_mp *dmp, *next;
> +
> +    LIST_FOR_EACH_SAFE (dmp, next, list_node, &dpdk_mp_list) {
> +        if (!dmp->refcount && rte_mempool_full(dmp->mp)) {

I hadn't looked at rte_mempool_full() before. I noticed the documentation warns not to use it as part of the datapath but for debug purposes only.

Does its use add to the downtime in a noticeable way while we reconfigure? I'm thinking of a deployment where the number of lcores is high and we reconfigure often. When cache is enabled, it has to browse the length of all lcores. There may not be an issue here but was curious.

Did you do any testing around this?

Thanks
Ian

> +            ovs_list_remove(&dmp->list_node);
> +            rte_mempool_free(dmp->mp);
> +            rte_free(dmp);
> +        }
> +    }
> +}
> +
>  static struct dpdk_mp *
>  dpdk_mp_get(int socket_id, int mtu)
>  {
>      struct dpdk_mp *dmp;
> +    bool reuse = false;
> 
>      ovs_mutex_lock(&dpdk_mp_mutex);
> @@ -599,15 +615,18 @@ dpdk_mp_get(int socket_id, int mtu)
>          if (dmp->socket_id == socket_id && dmp->mtu == mtu) {
>              dmp->refcount++;
> -            goto out;
> +            reuse = true;
> +            break;
>          }
>      }
> +    /* Sweep mempools after reuse or before create. */
> +    dpdk_mp_sweep();
> 
> -    dmp = dpdk_mp_create(socket_id, mtu);
> -    if (dmp) {
> -        ovs_list_push_back(&dpdk_mp_list, &dmp->list_node);
> +    if (!reuse) {
> +        dmp = dpdk_mp_create(socket_id, mtu);
> +        if (dmp) {
> +            ovs_list_push_back(&dpdk_mp_list, &dmp->list_node);
> +        }
>      }
> 
> -out:
> -
>      ovs_mutex_unlock(&dpdk_mp_mutex);
> 
> @@ -615,5 +634,5 @@ out:
>  }
> 
> -/* Release an existing mempool. */
> +/* Decrement reference to a mempool. */
>  static void
>  dpdk_mp_put(struct dpdk_mp *dmp)
> @@ -625,10 +644,5 @@ dpdk_mp_put(struct dpdk_mp *dmp)
>      ovs_mutex_lock(&dpdk_mp_mutex);
>      ovs_assert(dmp->refcount);
> -
> -    if (!--dmp->refcount) {
> -        ovs_list_remove(&dmp->list_node);
> -        rte_mempool_free(dmp->mp);
> -        rte_free(dmp);
> -     }
> +    dmp->refcount--;
>      ovs_mutex_unlock(&dpdk_mp_mutex);
>  }
> --
> 1.8.3.1
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list