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

[Andy Zhou]

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.

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.

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