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

[Jarno Rajahalme]

Here’s the updated description:

    lib/dpif-netdev: Make emc_mutex recursive.
    
    dpif_netdev_execute may be called while doing upcall processing.
    Since the context of the input port is not tracked upto this point, we
    use the shared dp->emc_cache for packet execution, where the emc_cache
    is needed for recirculation.
    
    While recursive mutexes can make thread safety analysis hard, for now
    we change emc_mutex to be recursive.  Forthcoming new unit tests will
    fail with the current non-recursive mutex.  Later improvements may
    remove the need for this recursion.
    
    Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>

On Sep 8, 2014, at 4:06 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:

> Forgot to update the description,
> 
>  Jarno
> 
>> On Sep 8, 2014, at 3:37 PM, 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 |   32 ++++++++++----------------------
>> 1 file changed, 10 insertions(+), 22 deletions(-)
>> 
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 409c9bf..072ed5d 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -380,10 +380,9 @@ static void dp_netdev_execute_actions(struct dp_netdev *dp,
>>                                      struct emc_cache *flow_cache,
>>                                      const struct nlattr *actions,
>>                                      size_t actions_len);
>> -static void dp_netdev_port_input(struct dp_netdev *dp,
>> -                                 struct emc_cache *flow_cache,
>> -                                 struct dpif_packet **packets, int cnt,
>> -                                 odp_port_t port_no);
>> +static void dp_netdev_input(struct dp_netdev *, struct emc_cache *,
>> +                            struct dpif_packet **, int cnt,
>> +                            struct pkt_metadata *);
>> 
>> static void dp_netdev_set_pmd_threads(struct dp_netdev *, int n);
>> static void dp_netdev_disable_upcall(struct dp_netdev *);
>> @@ -560,7 +559,7 @@ create_dp_netdev(const char *name, const struct dpif_class *class,
>>        return error;
>>    }
>> 
>> -    ovs_mutex_init(&dp->emc_mutex);
>> +    ovs_mutex_init_recursive(&dp->emc_mutex);
>>    emc_cache_init(&dp->flow_cache);
>> 
>>    *dpp = dp;
>> @@ -1799,14 +1798,15 @@ dp_netdev_process_rxq_port(struct dp_netdev *dp,
>> 
>>    error = netdev_rxq_recv(rxq, packets, &cnt);
>>    if (!error) {
>> -        dp_netdev_port_input(dp, flow_cache, packets, cnt, port->port_no);
>> +        struct pkt_metadata md = PKT_METADATA_INITIALIZER(port->port_no);
>> +
>> +        *recirc_depth_get() = 0;
>> +        dp_netdev_input(dp, flow_cache, packets, cnt, &md);
>>    } else if (error != EAGAIN && error != EOPNOTSUPP) {
>> -        static struct vlog_rate_limit rl
>> -            = VLOG_RATE_LIMIT_INIT(1, 5);
>> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>> 
>>        VLOG_ERR_RL(&rl, "error receiving data from %s: %s",
>> -                    netdev_get_name(port->netdev),
>> -                    ovs_strerror(error));
>> +                    netdev_get_name(port->netdev), ovs_strerror(error));
>>    }
>> }
>> 
>> @@ -2404,18 +2404,6 @@ dp_netdev_input(struct dp_netdev *dp, struct emc_cache *flow_cache,
>>    }
>> }
>> 
>> -
>> -static void
>> -dp_netdev_port_input(struct dp_netdev *dp, struct emc_cache *flow_cache,
>> -                     struct dpif_packet **packets, int cnt, odp_port_t port_no)
>> -{
>> -    uint32_t *recirc_depth = recirc_depth_get();
>> -    struct pkt_metadata md = PKT_METADATA_INITIALIZER(port_no);
>> -
>> -    *recirc_depth = 0;
>> -    dp_netdev_input(dp, flow_cache, packets, cnt, &md);
>> -}
>> -
>> struct dp_netdev_execute_aux {
>>    struct dp_netdev *dp;
>>    struct emc_cache *flow_cache;
>> -- 
>> 1.7.10.4
>>