On Sep 8, 2014, at 1:09 PM, Daniele Di Proietto <ddiproie...@vmware.com> wrote:
> Hi Jarno,
>
> Thanks for figuring out this bug.
> I erroneously thought that since commit 623540e4617e (dpif-netdev:
> Streamline miss handling.) dpif_netdev_execute() could not be called
> anymore inside a dp_netdev_input() frame.
>
>
> I added the exact match cache on the non pmd-thread path for two reasons:
>
> - Avoiding a branch on the fast path
> - Testing the exact match cache
>
> I thought about it a little and I do not think there¹s a way to avoid the
> branch now, so I¹m ok with the approach. I¹ve tested the DPDK throughput
> with the patch and it has not changed noticeably.
>
> One comment below:
>
> On 9/8/14, 10:07 AM, "Jarno Rajahalme" <jrajaha...@nicira.com> wrote:
>
>> dpif_netdev_execute may be called while doing upcall processing.
>> Since the context of the input port is not tracked upto this point, we
>> used the chared dp->emc_cache for packet execution. Execution needs
>> to be able to pass the cache to recirculation code.
>>
>> Typically the pmd threads use their own emc_cache, so there is little
>> value in using the shared emc_cache while processing recirculation
>> during packet execution. Also, whenever the shared emc_cache was
>> already used while doing the upcall, the emc_mutex is already held,
>> and would be locked recursively in dpif_netdev_execute(). Rather than
>> changing the mutex to a recursive type, it seems more proper to not
>> use any emc_cache in dpif_netdev_execute.
>>
>> Forthcoming new unit tests will fail with the current lock recursion.
>>
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>> ---
>> lib/dpif-netdev.c | 42 ++++++++++++++++++------------------------
>> 1 file changed, 18 insertions(+), 24 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 869fb55..846c329 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
> (...)
>>
>>
>> @@ -2240,6 +2242,10 @@ emc_processing(struct dp_netdev *dp, struct
>> emc_cache *flow_cache,
>> size_t n_batches, i;
>> size_t notfound_cnt = 0;
>>
>> + if (OVS_UNLIKELY(!flow_cache)) {
>> + return cnt;
>> + }
>> +
>
> emc_processing() is also supposed to extract the miniflows of the packets.
> Returning at this point without extracting the miniflow causes undefined
> behavior.
>
>
I’d rather not complicate the code any more, so let’s just use a recursive
mutex for now. Pravin promised we get rid of the need to pass the emc_cache to
action execution code later, so we can get rid of the mutex recursion at that
time.
I’ll test and send a v2 soon.
Jarno
>
> Daniele
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
