[ovs-dev] [PATCH] datapath: Convert dp rcu read operation to locked operations

[Pravin Shelar]

On Tue, Nov 4, 2014 at 12:39 PM, Andy Zhou <azhou at nicira.com> wrote:
> Looks good.  Acked-by: Andy Zhou <azhou at nicira.com>
>
> It would be to nice add more details to the commit message. The actual
> problem occurs when ovs_dp_name() is called.
>
I pushed this fix to master.

> In the long run, it would be nice to add lock depends to
> lookup_datapath. It seems all the use cases requires mutex to be held
> to prevent related data structures, such as vport list, from changing.
>

I agree, I will send followup patch.
Thanks.

> On Wed, Oct 29, 2014 at 2:45 AM, Pravin B Shelar <pshelar at nicira.com> wrote:
>> dp read operations depends on ovs_dp_cmd_fill_info(). This API
>> needs to looup vport to find dp name, but vport lookup can
>> fail. Therefore to keep vport reference alive we need to
>> take ovs lock.
>>
>> Found by code inspection.
>>
>> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
>> ---
>>  datapath/datapath.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index 6820a95..5fc036c 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -1332,7 +1332,7 @@ static size_t ovs_dp_cmd_msg_size(void)
>>         return msgsize;
>>  }
>>
>> -/* Called with ovs_mutex or RCU read lock. */
>> +/* Called with ovs_mutex. */
>>  static int ovs_dp_cmd_fill_info(struct datapath *dp, struct sk_buff *skb,
>>                                 u32 portid, u32 seq, u32 flags, u8 cmd)
>>  {
>> @@ -1618,7 +1618,7 @@ static int ovs_dp_cmd_get(struct sk_buff *skb, struct genl_info *info)
>>         if (!reply)
>>                 return -ENOMEM;
>>
>> -       rcu_read_lock();
>> +       ovs_lock();
>>         dp = lookup_datapath(sock_net(skb->sk), info->userhdr, info->attrs);
>>         if (IS_ERR(dp)) {
>>                 err = PTR_ERR(dp);
>> @@ -1627,7 +1627,7 @@ static int ovs_dp_cmd_get(struct sk_buff *skb, struct genl_info *info)
>>         err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
>>                                    info->snd_seq, 0, OVS_DP_CMD_NEW);
>>         BUG_ON(err < 0);
>> -       rcu_read_unlock();
>> +       ovs_unlock();
>>
>>         return genlmsg_reply(reply, info);
>>
>> @@ -1644,8 +1644,8 @@ static int ovs_dp_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
>>         int skip = cb->args[0];
>>         int i = 0;
>>
>> -       rcu_read_lock();
>> -       list_for_each_entry_rcu(dp, &ovs_net->dps, list_node) {
>> +       ovs_lock();
>> +       list_for_each_entry(dp, &ovs_net->dps, list_node) {
>>                 if (i >= skip &&
>>                     ovs_dp_cmd_fill_info(dp, skb, NETLINK_CB(cb->skb).portid,
>>                                          cb->nlh->nlmsg_seq, NLM_F_MULTI,
>> @@ -1653,7 +1653,7 @@ static int ovs_dp_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
>>                         break;
>>                 i++;
>>         }
>> -       rcu_read_unlock();
>> +       ovs_unlock();
>>
>>         cb->args[0] = i;
>>
>> --
>> 1.9.3
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev