[ovs-dev] [PATCH] netdev-dpdk: Remove use of rte_mempool_ops_get_count.

Stokes, Ian ian.stokes at intel.com
Wed May 23 11:53:28 UTC 2018


> > rte_mempool_ops_get_count is not exported by DPDK so it means it
> > cannot be used by OVS when using DPDK as a shared library.
> >
> > It was only being used for extra paranoid, things might change in the
> > future mode anyway, so remove it and just use rte_mempool_full.
> 
> How about keeping the dpdk_mp_full() function in a following manner:
> 
> static int
> dpdk_mp_full(const struct rte_mempool *mp) OVS_REQUIRES(dpdk_mp_mutex) {
>     /* XXX: A comment about possible false positives and why we're not
>      *      worrying about them so much. */
>     return rte_mempool_full(mp);
> }
> 
> ?
> 
> I feel that we still need some explanation inside the code.
> What do you think?
> 

It probably makes sense so that others unfamiliar will have context in the future.

I've validated the other patches and was about to push them but I can hold off for this change instead. It's a minor change so I can push it as an incremental on the current set if it helps move the process along.

Ian

> Best regards, Ilya Maximets.
> 
> >
> > The mempools are still removed later and are checked to see that the
> > buffers are back in it. Previously the mempools were removed straight
> > away and there was no checking.
> >
> > Fixes: 91fccdad72a2 ("netdev-dpdk: Free mempool only when no in-use
> > mbufs.")
> > Reported-by: Timothy Redaelli <tredaelli at redhat.com>
> > Reported-by: Markos Chandras <mchandras at suse.de>
> > Signed-off-by: Kevin Traynor <ktraynor at redhat.com>
> > ---
> >  lib/netdev-dpdk.c | 22 ++--------------------
> >  1 file changed, 2 insertions(+), 20 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > 87152a7..ede1e5c 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -527,22 +527,4 @@ ovs_rte_pktmbuf_init(struct rte_mempool *mp
> > OVS_UNUSED,  }
> >
> > -static int
> > -dpdk_mp_full(const struct rte_mempool *mp)
> > OVS_REQUIRES(dpdk_mp_mutex) -{
> > -    unsigned ring_count;
> > -    /* This logic is needed because rte_mempool_full() is not
> guaranteed to
> > -     * be atomic and mbufs could be moved from mempool cache -->
> mempool ring
> > -     * during the call. However, as no mbufs will be taken from the
> mempool
> > -     * at this time, we can work around it by also checking the ring
> entries
> > -     * separately and ensuring that they have not changed.
> > -     */
> > -    ring_count = rte_mempool_ops_get_count(mp);
> > -    if (rte_mempool_full(mp) && rte_mempool_ops_get_count(mp) ==
> ring_count) {
> > -        return 1;
> > -    }
> > -
> > -    return 0;
> > -}
> > -
> >  /* Free unused mempools. */
> >  static void
> > @@ -553,5 +535,5 @@ dpdk_mp_sweep(void)
> >      ovs_mutex_lock(&dpdk_mp_mutex);
> >      LIST_FOR_EACH_SAFE (dmp, next, list_node, &dpdk_mp_free_list) {
> > -        if (dpdk_mp_full(dmp->mp)) {
> > +        if (rte_mempool_full(dmp->mp)) {
> >              VLOG_DBG("Freeing mempool \"%s\"", dmp->mp->name);
> >              ovs_list_remove(&dmp->list_node); @@ -666,5 +648,5 @@
> > dpdk_mp_release(struct rte_mempool *mp)
> >
> >      ovs_mutex_lock(&dpdk_mp_mutex);
> > -    if (dpdk_mp_full(mp)) {
> > +    if (rte_mempool_full(mp)) {
> >          VLOG_DBG("Freeing mempool \"%s\"", mp->name);
> >          rte_mempool_free(mp);
> > --
> > 1.8.3.1


More information about the dev mailing list