[ovs-dev] [PATCH] lib/dpif-netdev: Remove recursive use of emc_mutex.

[Jarno Rajahalme]

On Sep 8, 2014, at 1:09 PM, Daniele Di Proietto <ddiproietto at 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" <jrajahalme at 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 <jrajahalme at 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