[ovs-dev] [PATCH v12 0/5] dpcls func ptrs & optimizations

Ilya Maximets i.maximets at samsung.com
Thu Jul 18 11:29:59 UTC 2019


On 17.07.2019 19:33, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets at samsung.com]
>> Sent: Wednesday, July 17, 2019 5:26 PM
>> To: Van Haaren, Harry <harry.van.haaren at intel.com>; dev at openvswitch.org
>> Cc: echaudro at redhat.com; malvika.gupta at arm.com; Stokes, Ian
>> <ian.stokes at intel.com>
>> Subject: Re: [PATCH v12 0/5] dpcls func ptrs & optimizations
> 
> <snip>
> 
>>>> I performed a few tests with v11 of this patch-set on my usual setup to
>>>> check
>>>> performance of the new generic (non-optimized) implementation. The result
>> is
>>>> that
>>>> new generic implementation is ~5% faster for me than current master (it
>> was
>>>> 12%
>>>> for optimized lookup functions) which is good.
>>>> The code looks OK for me in general. I still don't really like the fact
>> that
>>>> dpcls depends on the internal structure of a flowmap, but we, probably,
>>>> could
>>>> deal with that while we have a build time assertion. Hope, we'll have
>> some
>>>> better
>>>> implementation with the same level of performance in the future.
>>>
>>>
>>> Thanks for the feedback on performance Ilya - good to see that we're going
>>> in the right direction performance wise anyway.
>>>
>>> I have one item I'd still like to improve in this patchset, and its
>> regarding
>>> where the blocks scratch array is being stored. I'll rework and find a
>> better
>>> solution than the current, and post that as the lucky patchset v13 :)
>>
>> As an idea, I could suggest DEFINE_STATIC_PER_THREAD_DATA inside the dpif-
>> netdev-lookup-generic.c.
> 
> 
> Thanks for the suggestion - that would make the blocks scratch pointer available
> on a per-thread basis, and be initialized to NULL correct?
> 
> As a result, we would must a (predictable) branch to check if the pointer is
> NULL, to allocate the required space on first usage of the subtable_lookup?
> 
> Or is there a better way to initialize it for all threads that call dpcls_lookup()?
> My concern here is what if one thread creates a subtable (and allocs blocks_scratch for itself),
> but then the PMD is polled by another thread - which uses its own blocks_scratch which is NULL.
> 
> Hence, I think the runtime check + alloc is probably the best/safest way,
> but I'd appreciate your input on the above threading logic Ilya :)
> 


Sorry, I was already out of office when this message arrived. I'll send my comments
on your v13.

Best regards, Ilya Maximets.


More information about the dev mailing list