Looks good. Acked-by: Andy Zhou <[email protected]> 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 <[email protected]> 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 <[email protected]> > --- > 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 > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
