On Mon, Mar 24, 2014 at 11:56 AM, Jarno Rajahalme <[email protected]> wrote: > ovs_flow_cmd_del() now allocates reply (if needed) after the flow has > already been removed from the flow table. If the reply allocation > fails, a netlink error is signaled with netlink_set_err(), as is > already done in ovs_flow_cmd_new_or_set() in the similar situation. > This patch does two different optimizations which are not related. Can you separate them?
> Signed-off-by: Jarno Rajahalme <[email protected]> > --- > datapath/datapath.c | 70 > +++++++++++++++++++++++++-------------------------- > 1 file changed, 34 insertions(+), 36 deletions(-) > > diff --git a/datapath/datapath.c b/datapath/datapath.c > index f933831..e7cfd40 100644 > --- a/datapath/datapath.c > +++ b/datapath/datapath.c > @@ -664,7 +664,7 @@ static size_t ovs_flow_cmd_msg_size(const struct > sw_flow_actions *acts) > } > > /* Called with ovs_mutex or RCU read lock. */ > -static int ovs_flow_cmd_fill_info(struct sw_flow *flow, struct datapath *dp, > +static int ovs_flow_cmd_fill_info(struct sw_flow *flow, int dp_ifindex, > struct sk_buff *skb, u32 portid, > u32 seq, u32 flags, u8 cmd) > { > @@ -681,7 +681,7 @@ static int ovs_flow_cmd_fill_info(struct sw_flow *flow, > struct datapath *dp, > if (!ovs_header) > return -EMSGSIZE; > > - ovs_header->dp_ifindex = get_dpifindex(dp); > + ovs_header->dp_ifindex = dp_ifindex; > > /* Fill flow key. */ > nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY); > @@ -767,9 +767,8 @@ static struct sk_buff *ovs_flow_cmd_alloc_info(struct > sw_flow *flow, > > /* Must be called with ovs_mutex. */ > static struct sk_buff *ovs_flow_cmd_build_info(struct sw_flow *flow, > - struct datapath *dp, > - struct genl_info *info, > - u8 cmd) > + int dp_ifindex, > + struct genl_info *info, u8 cmd) > { > struct sk_buff *skb; > int retval; > @@ -778,8 +777,9 @@ static struct sk_buff *ovs_flow_cmd_build_info(struct > sw_flow *flow, > if (!skb) > return ERR_PTR(-ENOMEM); > > - retval = ovs_flow_cmd_fill_info(flow, dp, skb, info->snd_portid, > - info->snd_seq, 0, cmd); > + retval = ovs_flow_cmd_fill_info(flow, dp_ifindex, skb, > + info->snd_portid, info->snd_seq, 0, > + cmd); > BUG_ON(retval < 0); > return skb; > } > @@ -860,7 +860,9 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, > struct genl_info *info) > goto err_flow_free; > > if (ovs_must_build_reply(info, &ovs_dp_flow_multicast_group)) > - reply = ovs_flow_cmd_build_info(flow, dp, info, > + reply = ovs_flow_cmd_build_info(flow, > + > ovs_header->dp_ifindex, > + info, > OVS_FLOW_CMD_NEW); > } else { > /* We found a matching flow. */ > @@ -889,7 +891,9 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, > struct genl_info *info) > } > > if (ovs_must_build_reply(info, &ovs_dp_flow_multicast_group)) > - reply = ovs_flow_cmd_build_info(flow, dp, info, > + reply = ovs_flow_cmd_build_info(flow, > + > ovs_header->dp_ifindex, > + info, > OVS_FLOW_CMD_NEW); > > /* Clear stats. */ > @@ -952,7 +956,8 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct > genl_info *info) > goto unlock; > } > > - reply = ovs_flow_cmd_build_info(flow, dp, info, OVS_FLOW_CMD_NEW); > + reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex, info, > + OVS_FLOW_CMD_NEW); > if (IS_ERR(reply)) { > err = PTR_ERR(reply); > goto unlock; > @@ -970,57 +975,50 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct > genl_info *info) > struct nlattr **a = info->attrs; > struct ovs_header *ovs_header = info->userhdr; > struct sw_flow_key key; > - struct sk_buff *reply = NULL; > struct sw_flow *flow; > struct datapath *dp; > struct sw_flow_match match; > int err; > > + if (a[OVS_FLOW_ATTR_KEY]) { > + ovs_match_init(&match, &key, NULL); > + err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL); > + if (err) > + return err; > + } > + > ovs_lock(); > dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex); > if (!dp) { > err = -ENODEV; > goto unlock; > } > - > if (!a[OVS_FLOW_ATTR_KEY]) { > err = ovs_flow_tbl_flush(&dp->table); > goto unlock; > } > - > - ovs_match_init(&match, &key, NULL); > - err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL); > - if (err) > - goto unlock; > - > flow = ovs_flow_tbl_lookup(&dp->table, &key); > if (!flow || !ovs_flow_cmp_unmasked_key(flow, &match)) { > err = -ENOENT; > goto unlock; > } > + ovs_flow_tbl_remove(&dp->table, flow); > + ovs_unlock(); > > if (ovs_must_build_reply(info, &ovs_dp_flow_multicast_group)) { > - reply = ovs_flow_cmd_alloc_info(flow, info); > - if (!reply) { > - err = -ENOMEM; > - goto unlock; > - } > - } > - > - ovs_flow_tbl_remove(&dp->table, flow); > + struct sk_buff *reply; > > - if (reply) { > - err = ovs_flow_cmd_fill_info(flow, dp, reply, > info->snd_portid, > - info->snd_seq, 0, > - OVS_FLOW_CMD_DEL); > - BUG_ON(err < 0); > + reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex, > + info, OVS_FLOW_CMD_DEL); > + if (!IS_ERR(reply)) > + ovs_notify(reply, info, &ovs_dp_flow_multicast_group); > + else > + netlink_set_err(sock_net(skb->sk)->genl_sock, 0, > + ovs_dp_flow_multicast_group.id, > + PTR_ERR(reply)); > } > > ovs_flow_free(flow, true); > - ovs_unlock(); This patch is hitting assert in ovs_flow_free(). Plus I am seeing RCU checks failing in function building reply. Simplest fix would be not moving unlock function call. > - > - if (reply) > - ovs_notify(reply, info, &ovs_dp_flow_multicast_group); > return 0; > unlock: > ovs_unlock(); > @@ -1051,7 +1049,7 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb, > struct netlink_callback *cb) > if (!flow) > break; > > - if (ovs_flow_cmd_fill_info(flow, dp, skb, > + if (ovs_flow_cmd_fill_info(flow, ovs_header->dp_ifindex, skb, > NETLINK_CB(cb->skb).portid, > cb->nlh->nlmsg_seq, NLM_F_MULTI, > OVS_FLOW_CMD_NEW) < 0) > -- > 1.7.10.4 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
