<div dir="ltr">Thanks for the review. I will make the suggested changes. 2 comments inline: <br><br><div><div class="gmail_extra"><div class="gmail_quote">On Fri, Jun 7, 2013 at 4:43 PM, Jesse Gross <span dir="ltr"><<a href="mailto:jesse@nicira.com" target="_blank">jesse@nicira.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">OK, here's the second half:<br>
<div class="im"><br>
On Wed, Jun 5, 2013 at 10:46 PM, Andy Zhou <<a href="mailto:azhou@nicira.com">azhou@nicira.com</a>> wrote:<br>
</div><div class="im">> diff --git a/datapath/flow.c b/datapath/flow.c<br>
> index 7f897bd..fc8fb86 100644<br>
> --- a/datapath/flow.c<br>
> +++ b/datapath/flow.c<br>
</div><div class="im">> static struct kmem_cache *flow_cache;<br>
> +static LIST_HEAD(mask_list);<br>
<br>
</div>We should add a comment indicating that this is protected by ovs_mutex.<br>
<div class="im"><br>
> +static void update_desc__(struct sw_flow_match *match,<br>
> + size_t offset, size_t size, bool is_mask)<br>
> +{<br>
> + struct sw_flow_desc *desc = NULL;<br>
> + size_t start= offset;<br>
> + size_t end= offset + size;<br>
<br>
</div>Can you put spaces on both sides of the equals sign?<br>
<div class="im"><br>
> +bool ovs_match_validate(const struct sw_flow_match *match,<br>
> + u64 key_attrs, u64 mask_attrs)<br>
<br>
</div>I think the halves of this function (expectations and prerequisites)<br>
are basically two aspects of the same problem and should be handled<br>
together. For example, if the ICMPv6 type is not<br>
NDISC_NEIGHBOUR_SOLICITATION or NDSIC_NEIGHBOUR_ADVERTISEMENT then we<br>
shouldn't have a corresponding attribute for that. Doing it in a<br>
single pass would make it less likely that we miss something. I think<br>
we could handle it this way:<br>
- When extracting match values, record attributes that are all<br>
wildcards and remove then from the value attributes completely.<br>
- When computing expected attributes during validation, when we have<br>
a protocol value check, also verify that the mask is exact match.<br>
- AND the key_attributes with the mask values so we get a list of<br>
fields that have some matches in them.<br>
- Check that the attributes with matches is exactly the same as the<br>
expected attributes (not a subset in either direction).<br>
<br>
One downside of this is that it won't validate attributes that aren't<br>
used for matching, which doesn't really matter to the kernel but if we<br>
get an invalid flow then we won't detect it at setup time. Instead<br>
we'll just silently return a different ID when we serialize back to<br>
Netlink. To avoid this, we may want to continue to do a check on the<br>
unmasked attributes.<br>
<div class="im"><br>
> +static void flow_key_mask(struct sw_flow_key *dst,<br>
> + const struct sw_flow_key *src,<br>
> + const struct sw_flow_mask *mfm)<br>
> +{<br>
> + u8 *m = (u8 *)&mfm->key + mfm->desc.start;<br>
> + u8 *s = (u8 *)src + mfm->desc.start;<br>
> + u8 *d = (u8 *)dst + mfm->desc.start;<br>
<br>
</div>You could improve the performance here by using longs instead of chars.<br></blockquote><div>What about CPU architectures requires longs to be aligned? What is the conventional #ifdef style here? <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> +static bool __flow_cmp_key_mask(struct sw_flow *flow, u32 hash, u8 *key,<br>
> + int key_start, int key_len,<br>
> + struct sw_flow_mask *mfm)<br>
> +{<br>
> + return (flow->hash == hash && (flow->mfm == mfm) &&<br>
> + !memcmp((u8 *)&flow->key + key_start, key, (key_len - key_start)));<br>
> +}<br>
<br>
</div>Since the flows are non-overlapping, do we need to compare the masks?<br></blockquote><div>Yes, we are comparing masked key, which is no longer unique without comparing the mask. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> +struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *table,<br>
> + const struct sw_flow_key *flow_key,<br>
> + struct sw_flow_mask *mfm,<br>
> + int key_len)<br>
> +{<br>
> + const struct sw_flow_key *key;<br>
> + struct sw_flow *flow;<br>
> + struct hlist_head *head;<br>
> + u8 *_key;<br>
> + int key_start = mfm->desc.start;<br>
> + u32 hash;<br>
> + struct sw_flow_key masked_key;<br>
> + int end_roundup = mfm->desc.start<br>
> + + sw_flow_mask_roundup_size(mfm);<br>
<br>
</div>Do we have to round up the size at this point? It seems like the<br>
individual components might be able to do this based on their own<br>
needs (for example, ovs_flow_hash() already rounds up to 4 bytes).<br>
<div class="im"><br>
> +<br>
> + if (end_roundup < key_len)<br>
> + key_len = end_roundup;<br>
<br>
</div>Can't we just always use end_roundup?<br>
<br>
I'm not sure that it makes sense to calculate key_len in<br>
ovs_flow_extract() anymore. At this point, it's only used as a quick<br>
check to see whether a mask could potentially ever match. However,<br>
it's a pretty coarse filter though and I suspect that we could come up<br>
with something (if necessary) that is less complicated.<br>
<div class="im"><br>
> + flow_key_mask(&masked_key, flow_key, mfm);<br>
> + key = &masked_key;<br>
> + hash = ovs_flow_hash(key, key_start, key_len);<br>
> + _key = (u8 *) key + key_start;<br>
> head = find_bucket(table, hash);<br>
> hlist_for_each_entry_rcu(flow, head, hash_node[table->node_ver]) {<br>
> -<br>
> - if (flow->hash == hash &&<br>
> - !memcmp((u8 *)&flow->key + key_start, _key, key_len - key_start)) {<br>
> + if (__flow_cmp_key_mask(flow, hash, _key, key_start, key_len, mfm))<br>
<br>
</div>I'm not sure that we need to pass in both _key and key_start - they<br>
are more or less equivalent.<br>
<div class="im"><br>
> return flow;<br>
> - }<br>
> +<br>
> }<br>
> return NULL;<br>
> }<br>
><br>
> +<br>
<br>
</div>Extra blank line here.<br>
<div class="im"><br>
> void ovs_flow_tbl_insert(struct flow_table *table, struct sw_flow *flow,<br>
> - struct sw_flow_key *key, int key_len)<br>
> + const struct sw_flow_key *key, int key_len)<br>
> {<br>
> - flow->hash = ovs_flow_hash(key, flow_key_start(key), key_len);<br>
> - memcpy(&flow->key, key, sizeof(flow->key));<br>
> - __flow_tbl_insert(table, flow);<br>
> + flow->user_id = *key;<br>
> + flow->id_hash = ovs_flow_hash(key, flow_key_start(key), key_len);<br>
> + flow_key_mask(&flow->key, &flow->user_id, flow->mfm);<br>
<br>
</div>We should annotate the definition of flow->mfm with __rcu so that<br>
sparse would complain here (since we're not using ovsl_dereference,<br>
which we should be).<br>
<div class="im"><br>
> @@ -1012,37 +1159,48 @@ int ipv4_tun_from_nlattr(const struct nlattr *attr,<br>
</div>[...]<br>
<div class="im">> }<br>
> + if (is_mask) {<br>
> + SW_FLOW_KEY_PUT(match, tun_key.tun_flags, 0xffff, is_mask);<br>
> + }else {<br>
> + SW_FLOW_KEY_PUT(match, tun_key.tun_flags, tun_flags, is_mask);<br>
> + }<br>
> +<br>
<br>
</div>Do we need to force the tunnel flags to be exact match? Can't we allow<br>
userspace to specify which flags to mask using the usual logic?<br>
<br>
You could also drop the braces above and add a blank line before the<br>
if statement.<br>
<br>
> int ipv4_tun_to_nlattr(struct sk_buff *skb,<br>
[...]<br>
<div class="im">> + if (tun_key == output) {<br>
<br>
</div>Similar to the other direction, I think it's OK to just directly<br>
translate the flags even for masks.<br>
<div class="im"><br>
> +static int __metadata_from_nlattrs(struct sw_flow_match *match, u64 *attrs,<br>
> + const struct nlattr **a, bool is_mask)<br>
<br>
</div>I'm not sure that the underscores at the beginning are necessary since<br>
this isn't the internal version of any function.<br>
<div class="im"><br>
> +/**<br>
> + * ovs_match_from_nlattrs - parses Netlink attributes into a flow key and<br>
> + * mask. In case the mask is specified (mask == NULL), the flow is treated<br>
<br>
</div>I think this should say if the mask is NOT specified.<br>
<div><div class="h5"><br>
> +int ovs_match_from_nlattrs(struct sw_flow_match *match,<br>
> + const struct nlattr *key,<br>
> + const struct nlattr *mask)<br>
> +{<br>
> + const struct nlattr *a[OVS_KEY_ATTR_MAX + 1];<br>
> + const struct nlattr *m[OVS_KEY_ATTR_MAX + 1];<br>
> + const struct nlattr *encap;<br>
> + u64 key_attrs = 0;<br>
> + u64 mask_attrs = 0;<br>
> + bool encap_valid = false;<br>
> + int err;<br>
> +<br>
> + err = parse_flow_nlattrs(key, a, &key_attrs);<br>
> + if (err)<br>
> + return err;<br>
> +<br>
> + if (key_attrs & 1ULL << OVS_KEY_ATTR_ENCAP) {<br>
> + encap = a[OVS_KEY_ATTR_ENCAP];<br>
> + key_attrs &= ~(1ULL << OVS_KEY_ATTR_ENCAP);<br>
> + if (nla_len(encap)) {<br>
> + __be16 tci = 0;<br>
> + __be16 eth_type = 0; /* ETH_P_8021Q */<br>
> +<br>
> + if (a[OVS_KEY_ATTR_ETHERTYPE])<br>
> + eth_type = nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]);<br>
<br>
</div></div>It might be good to mask out this EtherType because we've effectively<br>
consumed it and are expecting a new one.<br>
<div class="im"><br>
> + if (a[OVS_KEY_ATTR_VLAN])<br>
> + tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);<br>
<br>
</div>We may want to start enforcing the condition that we don't have<br>
duplicate attributes in parse_flow_nlattrs(). Otherwise, an inner vlan<br>
(which we don't support currently) could replace the value from the<br>
outer tag here and we would silently accept it.<br>
<div class="im"><br>
> + if ( (eth_type == htons(ETH_P_8021Q))<br>
> + && (tci & htons(VLAN_TAG_PRESENT)) ) {<br>
<br>
</div>I would probably move the check for VLAN_TAG_PRESENT to<br>
ovs_key_from_nlattrs() and then just check for OVS_KEY_ATTR_VLAN here.<br>
<br>
It's somewhat unusual in kernel style to have extra spaces around the<br>
parentheses.<br>
<div class="im"><br>
> + encap_valid = true;<br>
> + err = parse_flow_nlattrs(encap, a, &key_attrs);<br>
> + }<br>
> + else<br>
> + err = -EINVAL;<br>
<br>
</div>Similarly, it's normal to have the else on the same line as the closing brace.<br>
<div class="im"><br>
> + if (ovs_match_validate(match, key_attrs, mask_attrs) == false)<br>
> return -EINVAL;<br>
<br>
</div>My usual comment about NULL/false checks.<br>
<div class="im"><br>
> +struct sw_flow_mask *sw_flow_mask_alloc(void)<br>
> +{<br>
> + struct sw_flow_mask *mfm;<br>
> +<br>
> + mfm = kmalloc(sizeof *mfm, GFP_KERNEL);<br>
> + if (mfm) {<br>
> + kref_init(&mfm->kref);<br>
> + INIT_LIST_HEAD(&mfm->list);<br>
<br>
</div>You should have to init this member, since it's not a list head.<br>
<div class="im"><br>
> +/**<br>
> + * Set 'desc' fileds in the mask to the value of 'val'.<br>
> + */<br>
> +void sw_flow_mask_set(struct sw_flow_mask *mfm,<br>
> + struct sw_flow_desc *desc, u8 val)<br>
<br>
</div>Can this be static?<br>
<br>
In ovs_flow_cmd_new_or_set(), I think we should have a check to make<br>
sure that the mask is not changed when updating an existing flow.<br>
</blockquote></div><br></div></div></div>