On Tue, Jun 11, 2013 at 04:45:10PM -0700, Andy Zhou wrote:
> Added --mega option to ovs-dpctl command to allow mega flow to be
> specified and displayed. ovs-dpctl tool is mainly used as debugging
> tool.
>
> This patch also implements the low level user space routines to send
> and receive mega flow netlink messages. Those netlink support
> routines are required for forthcoming user space mega flow patches.
>
> Ethan contributed current version of ovs-dpctl mega flow output
> function.
>
> Co-authored-by: Ethan Jackson <et...@nicira.com>
> Signed-off-by: Ethan Jackson <et...@nicira.com>
> Signed-off-by: Andy Zhou <az...@nicira.com>
Thanks for doing this! I have a few comments.
First I have to echo Ethan's comments about --mega and --micro. Are
they needed?
> --- a/lib/dpif-linux.c
> +++ b/lib/dpif-linux.c
> @@ -105,6 +105,8 @@ struct dpif_linux_flow {
> * the Netlink version of the command, even if actions_len is zero. */
> const struct nlattr *key; /* OVS_FLOW_ATTR_KEY. */
> size_t key_len;
> + const struct nlattr *mask; /* OVS_FLOW_ATTR_KEY. */
OVS_FLOW_ATTR_MASK?
The indentation of the comment is off by one space.
It looks like dpif-netdev does not support masks. This will reduce
code coverage for "make check", because we will not be able to test
ofproto-dpif behavior with masks.
The comment on 'flow_dump_next' in lib/dpif-provider.h needs an
update.
dpif_flow_put() needs a comment update.
I don't see anything anywhere that says that a datapath may ignore a
field's wildcards, instead fixing the field's values. That's
important but not at all obvious from the interface, so I would think
that we should document this in the dpif interface and possibly in
datapath/README (check with Jesse on that).
format_odp_key_attr() has a loop that can be replaced by a call to
is_all_ones(), declared in util.h.
format_odp_key_attr() doesn't seem to check the length of the mask
attribute. I think it needs the same check as the key attribute.
The OVS_KEY_ATTR_PRIORITY and OVS_KEY_ATTR_SKB_MARK cases in
format_odp_key_attr() look identical. I think that you can combine
them.
In the OVS_KEY_ATTR_TUNNEL case in format_odp_key_attr(), I think that
we need to memset() tun_mask to zeros before we parse it, as we do for
tun_key just above.
In the same case, I think that the two casts to uint32_t are
unnecessary. I see that they were there before, but I do not know
why.
In the same case, the bit-mask for the tunnel flags is going to be
hard to interpret, because no one knows the bit assignments. I
suggest using format_flags() for the mask too. Maybe a format like
this: flags(df)/(df).
In format_odp_key_attr(), for the OVS_KEY_ATTR_VLAN case, the mask is
a be16, so it needs to be retrieved that way and byte-swapped before
printing. Similarly for OVS_KEY_ATTR_MPLS, except that there the
mask is encapsulated inside a struct ovs_key_mpls and should be
retrieved through that.
In the OVS_KEY_ATTR_IPV6 case in format_odp_key_attr(), nothing
initializes src_mask or dst_mask.
parse_odp_key_attr() has many instances of constants like 0xffffffff.
Please use UINT32_MAX (etc.) instead to make it easier to recognize
that the values are correct.
parse_odp_key_mask_attr() uses %x for many of the masks. Our usual
practice is to use %i, to allow users to specify input in any base.
This also will allow us later, if we choose, to switch to using a
tokenizing parser that doesn't have to guess based on context which
radix a number is in.
In parse_odp_key_mask_attr(), the tun_id parser looks like it doesn't
accept a mask.
Please add test cases for masks to tests/odp.at (this would have
caught the tun_id oversight).
In the tunnel parser, I don't think that any of the casts to uint16_t
are needed.
For vlan parsing, I don't think we need "unsigned long long int" for
vlan_mask. A vlan is 16 bits so an "int" will do (see the big comment
at the top of parse_odp_key_mask_attr()). Similarly for many of the
fields in the IPv4, IPv6, TCP, UDP, and other fields' parsing (look at
the sizes of the similar fields for the key).
I don't see support for parsing masked MPLS in
parse_odp_key_mask_attr().
parse_odp_key_attr() duplicates half the code in
parse_odp_key_mask_attr(). Couldn't both sets of functionality be
implemented together, without duplication, by passing a boolean flag
that, if not set, disallows masking? e.g. something like this for
skb_priority:
{
unsigned long long int priority;
unsigned long long int priority_mask;
int n = -1;
if (maskable && sscanf(s, "skb_priority(%llx/%llx)%n", &priority,
&priority_mask, &n) > 0 && n > 0) {
nl_msg_put_u32(key, OVS_KEY_ATTR_PRIORITY, priority);
nl_msg_put_u32(mask, OVS_KEY_ATTR_PRIORITY, priority_mask);
return n;
} else if (sscanf(s, "skb_priority(%llx)%n",
&priority, &n) > 0 && n > 0) {
nl_msg_put_u32(key, OVS_KEY_ATTR_PRIORITY, priority);
nl_msg_put_u32(mask, OVS_KEY_ATTR_PRIORITY, 0xffffffff);
return n;
}
}
The caller would always have to pass in a 'mask' ofpbuf, but it would
be easy for the caller to provide one and just discard it afterward.
And we'd get rid of tons of code duplication that is going to be hard
to maintain.
Instead of odp_micro_flow_key_from_string() and
odp_mega_flow_key_from_string(), I wonder whether it would be more
convenient to have a single odp_flow_key_from_string() with an
optional (possibly NULL) 'mask' parameter.
There's a typo here in ovs-dpctl.c ("meag"):
+ " --mega create a meag flow \n"
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
