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

Kevin Traynor ktraynor at redhat.com
Wed May 23 12:02:49 UTC 2018


On 05/23/2018 12:53 PM, Stokes, Ian wrote:
>>> 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.
> 

Yes, it's a reasonable thing to do.

> 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.
> 

I'll send a new set, as I think it's easier to go from the previous code
than add back in the function in an incrementals. As you've validated
the other patches already, I think code review should be enough as it's
a very minor change.

Kevin.

> 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