Thank you for the patch! I have some comments. I'm dividing them
into two groups. The first group, in this email, is comments that
point out likely bugs. The second group, which I will send
separately, is comments that point out less important issues.
I didn't review the kernel code. That will have to wait for Jesse.
include/linux/openvswitch.h
---------------------------
We don't usually create structs for the arguments when the arguments
are simple types such as a single integer. Instead, just indicate in
the comment on the side the type and in the big comment on "enum
ovs_action_attr" the effect of the action. Actually every new action
needs to be explained in the big comment.
Your change renumbers OVS_ACTION_ATTR_SAMPLE but we can't do that
because it's part of the ABI.
lib/dpif-netdev.c
-----------------
OVS_ACTION_ATTR_SET_MPLS_LABEL takes an entire MPLS LSE as its
argument, and the odp-util.c code parses an entire MPLS LSE (except
the bottom-of-stack bit, which I've commented on elsewhere), but the
dpif-netdev.c code only sets the "label" bits from the LSE into the
packet. The "datapath" version of the code, on the other hand, does
something more complicated (it appears to set all the fields within
the LSE that are nonzero into the packet). This looks very
inconsistent and probably wrong.
lib/flow.c
----------
This adds some support for 802.1ad but I'd prefer to keep that
separate from MPLS support, so please remove the 802.1ad stuff from
this patch.
flow_extract() now has code to skip over inner VLAN tags. What's the
rationale for that? It seems unwise to me, because it makes it
impossible for a controller to distinguish a packet with one VLAN tag
from one with a dozen. The implementation is also not correct as far
as I can tell, because it can overrun the end of the packet.
For the same reason I wonder whether we should really skip over all
the MPLS labels?
I found the flow_extract() treatment confusing. It skips past all the
MPLS labels, which would imply that we are going to look at packet
data beyond them, but that will in fact never happen (because the
dl_type is always MPLS). We do want to skip the MPLS header so that
we can set the l3 pointer properly.
flow_is_mpls_label_valid() looks wrong to me, because I think that
the left-shift in computing 'tmp_label' could discard some high 1-bits
that aren't really valid. I would consider writing it as:
return !(mpls_label & htonl(MPLS_LABEL_MASK >> MPLS_LABEL_SHIFT));
which I think has the desired effect.
flow_format() doesn't show the "bottom of stack" bit. Should it? (It
looks like the kernel populates it from the packet, so I would expect
so.)
I have a concern about flow_hash_symmetric_l4(). This function is not
well-documented, but it's intent is that packets in either direction
in a pair of related flows (e.g. the two flows that make up a TCP
connection) will have the same hash. Will two flows related in that
way have the same MPLS LSE? This function should only hash the
mpls_lse if the answer is "yes".
lib/meta-flow.c
---------------
I guess that MFF_MPLS_LABEL and MFP_MPLS_TC should have a
prerequisite, because the MPLS label may only be matched if the
Ethernet type indicates that the packet is a MPLS packet (right?). So
I would add an MFP_MPLS and appropriate support for it.
I think that the definitions in mf_is_value_valid() and mf_get_value()
are contradictory. mf_is_value_valid() insists that the label be in
the least significant 20 bits of value->be32, but mf_get_value() puts
it in the most significant 20 bits. I suggest that both should use
the least significant 20 bits.
I think that mf_random_value() has the same problem.
lib/nx-match.c
--------------
Like flow_format(), I would expect format_mpls_label() to print the
"bottom of stack" bit. Similarly with the parsing code in
parse_odp_actions() and parse_odp_key_attr().
format_odp_action() and other code expects that
OVS_ACTION_ATTR_POP_MPLS has an ethertype argument, but
parse_odp_action() doesn't supply one.
lib/odp-util.h
--------------
In parse_odp_action(), the number here should be 16, not 11, in two
places:
+ if (!strncmp(s, "copy_mpls_ttl_in", 11)) {
+ nl_msg_put_flag(actions, OVS_ACTION_ATTR_COPY_TTL_IN);
+ return 11;
+ }
and here it should be 17, not 12, in two places:
+ if (!strncmp(s, "copy_mpls_ttl_out", 12)) {
+ nl_msg_put_flag(actions, OVS_ACTION_ATTR_COPY_TTL_OUT);
+ return 12;
+ }
lib/ofp-parse.c
---------------
In parse_protocol(), there are two entries for "mpls". Only the first
one will have an effect, the second one would need a different name.
lib/ofp-print.c
---------------
In ofp_print_action(), please put "0x" into the following formats.
Otherwise, the values printed will be parsed as decimal if they are
fed back into the parser:
+ case OFPUTIL_NXAST_PUSH_MPLS:
+ nampush = (const struct nx_action_push_mpls *) a;
+ ds_put_format(s, "push_mpls:%"PRIx16, ntohs(nampush->ethertype));
+ break;
+
+ case OFPUTIL_NXAST_POP_MPLS:
+ nampop = (const struct nx_action_pop_mpls *) a;
+ ds_put_format(s, "pop_mpls:%"PRIx16, ntohs(nampop->ethertype));
+ break;
In ofp_match_to_string(), I don't think it's a good idea to abbreviate
both MPLS ethertypes the same way. Otherwise, the shorthand loses
information.
lib/packets.c
-------------
dec_mpls_ttl() will decrement a ttl of 0 to 255. Is that acceptable
behavior? Perhaps the comment should mention it.
copy_mpls_ttl_in() and copy_mpls_ttl_out() assume that the l3 header
is present and that it is IPv4, or alternatively that an inner MPLS
header is present, without checking.
set_mpls_lse_tc() is buggy. It needs to do the shift before the mask.
set_mps_lse_stack() is buggy the same way.
push_mpls() reads the IP header without first checking that it is
present.
It appears that push_mpls() refuses to add an MPLS header if there is
a VLAN tag and the inner Ethertype is not IPv4. Why?
ofproto/ofproto-dpif.c
----------------------
Did you carefully work out how the actions are implemented? Does the
right thing happen (a no-op) if an OpenFlow flow has a "push mpls"
action followed by a "pop mpls" action? etc.
I'm not sure, in fact, that it makes sense to try to delay committing
MPLS actions. It seems tricky, to me, at least. I'd be tempted to
write them to the ODP actions immediately rather than trying to defer
it. The code would be simpler and more obviously correct, at any rate.
tests/test-mpls.c
-----------------
I don't understand why this program is here. You should be able to
write unit tests for this feature without actually opening raw
sockets, etc. We have sufficient tools to test packets through the
"dummy" interfaces, which some of the existing unit tests. At any
rate I believe that this test program requires "root" privileges to
work.
You didn't add any unit tests, just this test program. That means
that "make check" doesn't actually test anything MPLS related. We
need at least basic unit tests.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
