Please see inline <rk>. I have removed the ones which I fixed it. Some questions/clarifications inline.
-----Original Message----- From: Ben Pfaff [mailto:b...@nicira.com] Sent: Wednesday, May 23, 2012 11:10 AM To: Kerur, Ravi Cc: dev@openvswitch.org Subject: Re: [ovs-dev] MPLS and VLAN QinQ patch 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? <rk> My plan was to first get to 1.1 and later on to 1.2/1.3. I think it is good to have a reference code for 1.1 just in case there is a request for that. I haven't followed the discussion initiated by Zoltan on this, will look into it. Do you think match capability for "mpls ttl" should be added as well since it provides similar purpose as layer-3? 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. <rk> Same argument as above. In this case it isn't quite clear to me how set_mpls_label and set_mpls_tc be implemented via reg_load. Any pointers? 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; } } <rk> I quite understand vlan implementation part, though mpls parsing implementation is similar to vlan, the reason I had it was for ovs to reject packets if there is nothing following mpls header. Initially I checked for ipv4/ipv6 header version in parse_mpls and later removed after discussion on mpls and non-IP packets. Though 2 bytes isn't enough for sanity check, I just left it as it is. 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.) <rk> I don't have strong opinion either way. 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. <rk> I will revert it back. Initially I just had a single call but don't remember why I changed it. Will fix it. 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? <rk> checks are performed in validate_action as well. 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. <rk> I will revert it. tests/odp.at ------------ You should add tests for parsing and formatting the mpls keys and actions here, following the existing pattern. <rk> have added tests for mpls. Thanks, Ravi _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
