On 05/23/2018 01:05 PM, Stokes, Ian wrote:
>> 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?
>>>>
Thanks for suggestion. Added in v2. The nicest thing about this
suggestion is that the patch now applies to the other branches :-)
>>>
>>> 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.
>>
>
> +1
@Ian, just to re-iterate the above, the v2 patch applies cleanly on
branches 2.6/2.7/2.8/2.9,
thanks,
Kevin.
>
>> 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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
