On Tue, May 22, 2012 at 07:36:32PM +0200, ravi.ke...@telekom.com wrote:
> Attached MPLS and VLAN QinQ patch after rebasing to following commit
>
> commit 046f1f89e6d7716581de207dd0c54421926bc25b
> Author: Ethan Jackson <et...@nicira.com<mailto:et...@nicira.com>>
> Date: Mon May 21 13:20:18 2012 -0700
Here's some initial feedback for stuff I noticed in reading roughly
half of the MPLS patch. I'll send more later, but this should be
enough to get you going.
High-level comments
-------------------
OF1.3 adds the ability to match on the MPLS "BOS" bit. Should we add
that here too?
It occurs to me that we could drop the NXAST_SET_MPLS_LABEL and
NXAST_SET_MPLS_TC actions because these can be implemented through
"register load" actions. OpenFlow 1.2 also dropped the similar OFPAT_
actions, for the same reason, so maybe we shouldn't bother with NXAST_
actions for them as they don't have any additional benefit for
OpenFlow 1.0 with Nicira extensions.
I didn't read the datapath code.
include/linux/openvswitch.h
---------------------------
I'd indent the comments for the last three the same as for the first
three, below:
+ OVS_ACTION_ATTR_PUSH_MPLS, /* be16, ethertype */
+ OVS_ACTION_ATTR_POP_MPLS, /* be16, ethertype */
+ OVS_ACTION_ATTR_SET_MPLS_LSE, /* be32, mpls label stack entry */
+ OVS_ACTION_ATTR_DEC_MPLS_TTL, /* No argument */
+ OVS_ACTION_ATTR_COPY_TTL_OUT, /* No argument */
+ OVS_ACTION_ATTR_COPY_TTL_IN, /* No argument */
include/openflow/nicira-ext.h
-----------------------------
The comments should be slightly more explicit, e.g. change
ovs_be32 mpls_label; /* MPLS label */
to
ovs_be32 mpls_label; /* MPLS label in low 20 bits. */
and similarly for mpls_tc.
lib/flow.c
----------
Stray space here:
parse_remaining_mpls (struct ofpbuf *b)
and here:
parse_mpls (struct ofpbuf *b, struct flow *flow)
In parse_remaining_mpls(), this:
if (!(ntohl(flow->mpls_lse) & MPLS_STACK_MASK)) {
is better written as:
if (!(flow->mpls_lse & htonl(MPLS_STACK_MASK))) {
In parse_remaining_mpls() and parse_mpls(), there's an odd "+
sizeof(ovs_be16)" that I don't see any reason for. I think you must
have just copied that from parse_vlan(). It's in parse_vlan() because
the VLAN TCI must be followed by at least 16 bits (the Ethertype). I
don't think there's the same requirement following an MPLS header, so
I'd be inclined to drop it.
I think that you can greatly simplify parse_remaining_mpls() to just
this (not tested):
while (b->size >= sizeof(struct mpls_hdr)) {
struct mpls_hdr *mh = ofpbuf_pull(b, sizeof *mh);
if (mh->mpls_lse & htonl(MPLS_STACK_MASK)) {
break;
}
}
I don't think we should consider the MPLS label in
flow_hash_symmetric_l4(), because there's no assurance that the MPLS
labels would be the same in both directions. (Bruce Davie tells me
that in fact that's unlikely.)
The comment on flow_set_mpls_label() seems incomplete (it ends with a
colon).
lib/flow.h
----------
The comment should end with a period and a space:
ovs_be32 mpls_lse; /* MPLS label stack entry*/
lib/meta-flow.h
---------------
Please add mpls_label and mpls_tc to the list of exceptions in the
large comment inside struct mf_field.
lib/odp-util.c
--------------
In format_odp_action(), a few bits of code seem bigger than necessary, e.g.:
+ case OVS_ACTION_ATTR_PUSH_MPLS:
+ push_ethertype = nl_attr_get_be16(a);
+ ds_put_cstr(ds, "push_mpls(");
+ ds_put_format(ds, "eth_type=0x%04"PRIx16"", ntohs(push_ethertype));
+ ds_put_char(ds, ')');
+ break;
could be written as a single call to ds_put_format(), and that's what
I'd do. Similarly for OVS_ACTION_ATTR_POP_MPLS.
In parse_odp_action(), you write:
+ int etype = ETH_TYPE_MPLS;
in two places, but in each case the ETH_TYPE_MPLS value is always
overwritten, so I'd drop the initializers.
It looks to me that the changes to parse_8021q_onward() and
odp_flow_key_to_flow() are the same and that they could be integrated
into parse_l3_onward() to avoid code duplication.
In commit_mpls_push_action() and commit_mpls_pop_action(), I'm
surprised that we can possibly log errors at this late stage.
Shouldn't we catch these errors earlier?
lib/ofp-parse.c
---------------
In parse_named_action(), you can use the autogenerated action-specific
functions to put actions, instead of the generic ofputil_put_action(),
e.g. for OFPUTIL_NXAST_COPY_TTL_OUT, use
ofputil_put_NXAST_COPY_TTL_OUT(). Please follow the pattern in the
existing cases.
lib/packets.h
-------------
Drop the VLAN-related changes in this patch, please.
mpls_lse_to_stack() can be more simply written as:
return (mpls_lse & htonl(MPLS_STACK_MASK)) != 0;
tests/odp.at
------------
You should add tests for parsing and formatting the mpls keys and
actions here, following the existing pattern.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
