Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755744AbaLJGLl (ORCPT ); Wed, 10 Dec 2014 01:11:41 -0500 Received: from na3sys009aog132.obsmtp.com ([74.125.149.250]:54848 "HELO na3sys009aog132.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751351AbaLJGLj (ORCPT ); Wed, 10 Dec 2014 01:11:39 -0500 MIME-Version: 1.0 In-Reply-To: References: <1417575363-13770-1-git-send-email-joestringer@nicira.com> <1417575363-13770-2-git-send-email-joestringer@nicira.com> Date: Tue, 9 Dec 2014 22:11:38 -0800 Message-ID: Subject: Re: [PATCHv11 net-next 2/2] openvswitch: Add support for unique flow IDs. From: Pravin Shelar To: Joe Stringer Cc: netdev , LKML , "dev@openvswitch.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Dec 9, 2014 at 4:25 PM, Joe Stringer wrote: > On 9 December 2014 at 10:32, Pravin Shelar wrote: >> On Tue, Dec 2, 2014 at 6:56 PM, Joe Stringer wrote: >>> @@ -662,11 +664,18 @@ static void get_dp_stats(const struct datapath *dp, struct ovs_dp_stats *stats, >>> } >>> } >>> + error = ovs_nla_copy_ufid(a[OVS_FLOW_ATTR_UFID], &ufid, log); >>> + if (error) >>> + return error; >>> + if (a[OVS_FLOW_ATTR_KEY]) { >>> + ovs_match_init(&match, &key, &mask); >>> + error = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], >>> + a[OVS_FLOW_ATTR_MASK], log); >>> + } else if (!ufid) { >>> OVS_NLERR(log, "Flow key attribute not present in set flow."); >>> - goto error; >>> + error = -EINVAL; >>> } >>> - >>> - ovs_match_init(&match, &key, &mask); >>> - error = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], >>> - a[OVS_FLOW_ATTR_MASK], log); >>> if (error) >>> goto error; >>> >>> - /* Validate actions. */ >>> - if (a[OVS_FLOW_ATTR_ACTIONS]) { >>> - acts = get_flow_actions(a[OVS_FLOW_ATTR_ACTIONS], &key, &mask, >>> - log); >>> - if (IS_ERR(acts)) { >>> - error = PTR_ERR(acts); >>> - goto error; >>> - } >>> - >>> - /* Can allocate before locking if have acts. */ >>> - reply = ovs_flow_cmd_alloc_info(acts, info, false); >>> - if (IS_ERR(reply)) { >>> - error = PTR_ERR(reply); >>> - goto err_kfree_acts; >>> - } >>> - } >>> - >> Why are you moving this action validation under ovs-lock? > > The thought was that flow_cmd_set may receive UFID and not key/mask. > One could imagine a command that sends a UFID and actions, telling OVS > kmod to update just the actions. Masked key is required for > ovs_nla_copy_actions(), so in this case a lookup would be required to > get a masked key. > > Perhaps the better alternative for the moment is to still require flow > key and mask for this command (just as we do for flow_cmd_new). That > would simplify this change greatly, and doesn't affect current OVS > userspace. > sounds good. >>> @@ -1194,9 +1254,18 @@ unlock: >>> >>> static int ovs_flow_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb) >>> { >>> + struct nlattr *a[__OVS_FLOW_ATTR_MAX]; >>> struct ovs_header *ovs_header = genlmsg_data(nlmsg_data(cb->nlh)); >>> struct table_instance *ti; >>> struct datapath *dp; >>> + u32 ufid_flags; >>> + int err; >>> + >>> + err = nlmsg_parse(cb->nlh, GENL_HDRLEN + dp_flow_genl_family.hdrsize, >>> + a, dp_flow_genl_family.maxattr, flow_policy); >> >> Can you add genl helper function for this? > > OK. > >>> @@ -1235,6 +1304,8 @@ static const struct nla_policy flow_policy[OVS_FLOW_ATTR_MAX + 1] = { >>> [OVS_FLOW_ATTR_ACTIONS] = { .type = NLA_NESTED }, >>> [OVS_FLOW_ATTR_CLEAR] = { .type = NLA_FLAG }, >>> [OVS_FLOW_ATTR_PROBE] = { .type = NLA_FLAG }, >>> + [OVS_FLOW_ATTR_UFID] = { .type = NLA_UNSPEC }, >>> + [OVS_FLOW_ATTR_UFID_FLAGS] = { .type = NLA_U32 }, >>> }; >>> >>> static const struct genl_ops dp_flow_genl_ops[] = { >>> diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h >>> index a8b30f3..7f31dbf 100644 >>> --- a/net/openvswitch/flow.h >>> +++ b/net/openvswitch/flow.h >>> @@ -197,6 +197,13 @@ struct sw_flow_match { >>> struct sw_flow_mask *mask; >>> }; >>> >>> +#define MAX_UFID_LENGTH 256 >>> + >> For now we can limit this to 128 bits. > > Is there a reason beyond trying to avoid the warning in flow_cmd_set()? > I suppose that its purpose as an identifier means that it's unlikely to > need to be much bigger in future (indeed, the larger it gets, the more > it would trade off the performance gains from using it). > I am also not sure why would we need ID larger than 128 bit. In such unlikely scenario I think we can increase it later if we need it. >>> @@ -213,13 +220,16 @@ struct flow_stats { >>> >>> struct sw_flow { >>> struct rcu_head rcu; >>> - struct hlist_node hash_node[2]; >>> - u32 hash; >>> + struct { >>> + struct hlist_node node[2]; >>> + u32 hash; >>> + } flow_hash, ufid_hash; >> I am not sure about _hash suffix here, Can you explain it? I think >> _table is better. > > Agreed, table is better. > >>> int stats_last_writer; /* NUMA-node id of the last writer on >>> * 'stats[0]'. >>> */ >>> struct sw_flow_key key; >>> - struct sw_flow_key unmasked_key; >>> + struct sw_flow_id *ufid; >> Lets move this structure inside sw_flow, so that we can avoid one >> kmalloc during flow-install in case of UFID. something like: >> >> struct { >> u32 ufid_len; >> union { >> u32 ufid[MAX_UFID_LENGTH / 4]; >> struct sw_flow_key *unmasked_key; >> } >> } id; > > Agreed. > >>> @@ -424,10 +475,9 @@ static struct sw_flow *masked_flow_lookup(struct table_instance *ti, >>> ovs_flow_mask_key(&masked_key, unmasked, mask); >>> hash = flow_hash(&masked_key, key_start, key_end); >>> head = find_bucket(ti, hash); >>> - hlist_for_each_entry_rcu(flow, head, hash_node[ti->node_ver]) { >>> - if (flow->mask == mask && flow->hash == hash && >>> - flow_cmp_masked_key(flow, &masked_key, >>> - key_start, key_end)) >>> + hlist_for_each_entry_rcu(flow, head, flow_hash.node[ti->node_ver]) { >>> + if (flow->mask == mask && flow->flow_hash.hash == hash && >>> + flow_cmp_masked_key(flow, &masked_key, key_start, key_end)) >>> return flow; >>> } >>> return NULL; >>> @@ -469,7 +519,40 @@ struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl, >>> /* Always called under ovs-mutex. */ >>> list_for_each_entry(mask, &tbl->mask_list, list) { >>> flow = masked_flow_lookup(ti, match->key, mask); >>> - if (flow && ovs_flow_cmp_unmasked_key(flow, match)) /* Found */ >>> + if (flow && !flow->ufid && >> why not NULL check for flow->unmasked_key here rather than ufid? > > In this version, I tried to consistently use flow->ufid as the switch > for whether UFID exists or not. In the next version, this statement > would refer to flow->id->ufid_len. > > The current approach means that ovs_flow_tbl_lookup_exact() is really > ovs_flow_tbl_lookup_unmasked_key(). Do you think this should remain > specific to unmasked key or should it be made to check that the > identifier (ufid OR unmasked key) is the same? It is easier to read code if we check for flow->unmasked_key here. ovs_flow_cmp_unmasked_key() has assert on ufid anyways. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/