[ovs-dev] MPLS less-important comments (was: Re: Patch for MPLS)
Ben Pfaff
blp at nicira.com
Tue Mar 13 10:31:05 PDT 2012
Thanks, I look forward to the revised patch.
On Mon, Mar 12, 2012 at 10:53:05PM +0100, Ravi.Kerur at telekom.com wrote:
> Thanks again Ben for the review. Please see inline <rk>
>
> -----Original Message-----
> From: dev-bounces at openvswitch.org [mailto:dev-bounces at openvswitch.org] On Behalf Of Ben Pfaff
> Sent: Friday, March 09, 2012 11:21 AM
> To: Kerur, Ravi
> Cc: dev at openvswitch.org
> Subject: [ovs-dev] MPLS less-important comments (was: Re: Patch for MPLS)
>
> Here are my additional comments that point out less-important issues.
>
> include/openflow/nicira-ext.h
> -----------------------------
>
> The "format' in the definition of mpls_label isn't entirely clear to
> me:
>
> +/* The mpls_label in MPLS shim header.
> + *
> + * Prereqs: NXM_OF_ETH_TYPE must be either 0x8847 or 0x8848.
> + *
> + * Format: 32-bit integer, higher 20 bits
> + *
> + * Masking: Not maskable. */
>
> The "format" in OF1.2 looks clearer to me, can you use that instead?
> Here it is:
>
> * Format: 32-bit integer in network byte order with 12 most-significant
> * bits forced to 0. Only the lower 20 bits have meaning.
>
> Similarly for mpls_tc:
>
> * Format: 8-bit integer with 5 most-significant bits forced to 0.
> * Only the lower 3 bits have meaning.
>
>
> lib/flow.c
> ----------
>
> parse_mpls() is unused. Please either use it or remove it.
>
> I wonder whether we really need separate L2.5 and L3 in struct ofpbuf,
> since if MPLS is present then we never parse past it. We could treat
> MPLS as L3, if that is appropriate.
>
> <rk> Traditinally MPLS is treated as a L2.5. Would like to leave it
> like that.
>
> This assignment is unnecessary because the top-level memset() will
> already have zeroed mpls_lse:
> } else {
> flow->mpls_lse = htonl(0);
> }
>
> <rk> Removed the code.
>
> You can drop the parentheses around the == here, and in practice we
> normally do:
> - if (eth->eth_type == htons(ETH_TYPE_VLAN)) {
> + if ((eth->eth_type == htons(ETH_TYPE_VLAN_8021q)) ||
> + (eth->eth_type == htons(ETH_TYPE_VLAN_8021ad))) {
>
> <rk> Removed it.
>
> This code in flow_zero_wildcards() looks odd to me:
> if ((wc & FWW_MPLS_LABEL) || (wc & FWW_MPLS_TC)) {
> if (wc & FWW_MPLS_LABEL) {
> flow->mpls_lse &= ~htonl(MPLS_LABEL_MASK);
> }
> flow->mpls_lse &= ~htonl(MPLS_TTL_MASK);
> flow->mpls_lse &= ~htonl(MPLS_STACK_MASK);
> if (wc & FWW_MPLS_TC) {
> flow->mpls_lse &= ~htonl(MPLS_TC_MASK);
> }
> } else {
> flow->mpls_lse &= ~htonl(MPLS_TTL_MASK);
> flow->mpls_lse &= ~htonl(MPLS_STACK_MASK);
> }
> I think that you can write it as just:
> flow->mpls_lse &= ~htonl(MPLS_TTL_MASK | MPLS_STACK_MASK);
> if (wc & FWW_MPLS_LABEL) {
> flow->mpls_lse &= ~htonl(MPLS_LABEL_MASK);
> }
> if (wc & FWW_MPLS_TC) {
> flow->mpls_lse &= ~htonl(MPLS_TC_MASK);
> }
>
>
> <rk> Agreed and fixed it.
>
> lib/meta-flow.c
> ---------------
>
> Since each of the new fields has a width in bits less than its width
> in bytes, it would be good to add them to the comment titled "Size"
> within struct mf_field.
>
> Please fix the box. Instead of this:
> /* ## -- ## */
> /* ## L2.5 ## */
> /* ## -- ## */
> write:
> /* ## ---- ## */
> /* ## L2.5 ## */
> /* ## ---- ## */
>
>
> <rk> Fixed it.
>
> lib/nx-match.c
> --------------
>
> I think that nx_put_match() can use mpls_lse_to_label() and
> mpls_lse_to_tc().
>
> <rk> Agreed, fixed it.
>
>
> lib/odp-util.h
> --------------
>
> I don't think that ODPUTIL_FLOW_KEY_BYTES or the comment above it
> needs an update, because MPLS is always the last header parsed; that
> is, it will never appear along with the IPv6, ICMPv6, and ND headers
> in the list.
>
> <rk> Agreed. It was added during debugging when things were not working. Fixed it now.
>
> In parse_odp_key_attr() the outer parentheses here are unnecessarily doubled:
> + if ((sscanf(s, "mpls(label=%"SCNi32",tc=%i,ttl=%i)%n",
> + &mpls_label, &mpls_tc, &mpls_ttl, &n) > 0 && n > 0)) {
>
> <rk> Fixed it.
>
> In parse_mpls_onward(), I think that the call to parse_l3_onward() can
> change to just check_expectations(), because none of the "if"
> statements in parse_l3_onward() will ever successfully match.
>
> <rk> Fixed it.
>
> In parse_mpls_onward(), the outer parentheses here are unnecessarily doubled:
> + expected_attrs |= ((UINT64_C(1) << OVS_KEY_ATTR_MPLS));
>
>
> <rk> Fixed it.
>
> lib/ofp-parse.c
> ---------------
>
> In parse_named_action(), it would probably be a good idea to validate
> that the arguments to set_mpls_label, set_mpls_tc, and set_mpls_ttl
> are in the correct range. For push_mpls and pop_mpls, you can use
> str_to_u16().
>
>
> lib/ofp-util.c
> --------------
>
> A line is mis-indented in ofputil_normalize_rule().
>
> <rk> Fixed it.
>
>
> lib/ofp-util.h
> --------------
>
> The OFP_ACTION_* enum values are only used in ofproto-dpif.c, so I'd
> move them there.
>
> <rk> Mainly used for mpls delayed action. Removed it.
>
> lib/packets.c
> -------------
>
> The comment on set_mpls_lse_ttl() is wrong.
>
> I think that the set_mpls_lse_ttl() code is only correct because
> MPLS_TTL_SHIFT is 0.
>
> dec_mpls_ttl() , copy_mpls_ttl_in(), and copy_mpls_ttl_out() could use
> mpls_lse_to_ttl().
>
> The first and final cases in push_mpls() are identical except for the
> source of the TTL. I suggest using a helper function.
>
> In pop_mpls(), you can write:
> if ((ntohl(mh->mpls_lse) & MPLS_STACK_MASK) >> MPLS_STACK_SHIFT) {
> as
> if (mh->mpls_lse & htonl(MPLS_STACK_MASK))
>
> <rk> Userspace code is tested now and comments incorporated.
>
> lib/packets.h
> -------------
>
> Please don't use an OFP_ prefix for constants that aren't defined by
> OpenFlow. I don't see OFP_MPLS_ANY or OFP_MPLS_NONE in OpenFlow. I
> don't see any user of OFP_MPLS_ANY anywhere in the tree, either (where
> is it used?)
>
> If you do need them, please use #defines for these, because enum
> constants always have type "int" which means that these are actually
> negative values.
>
> <rk> Don't need them, removed.
>
> ofproto/ofproto-dpif.c
> ----------------------
>
> The change to get_features() is a no-op, please drop it from the
> patch.
>
> I don't think the facet_account() changes actually have any visible
> effect.
>
> Why do all the "compose_*_mpls" functions check for null ctx or
> ofproto? Can either one ever happen? All of these functions are
> one-liners, and they only have a single caller, so I'd inline them
> into their caller.
>
> <rk> Mostly safer coding. Not needed now hence removed.
>
> utilities/ovs-ofctl.8.in
> ------------------------
>
> I don't think the sentence that starts "If none specified" below is
> correct:
> +.IP \fBmpls_label=\fIlabel\fR
> +Matches MPLS Label when \fIethertype\fR is \fI0x8847\fR or \fI0x8848\fR.
> +Specify a number between 16 and 2^20-1, inclusive, as the 20-bit MPLS label
> +to match. If none specified, all packets which has \fIethertype\fR equal to
> +\fI0x8847\fR or \fI0x8848\fR are matched.
>
> The shorthand notation "mpls" (for dl_type=0x8847) should be mentioned
> in the list of shorthands.
>
> s/classe/class/ here:
> +Modifies the MPLS traffic-classe of a packet.
>
>
> <rk> I think it is true. If no label or tc is specified all packets which has ethertype=0x8847/0x8848 is matched.
>
> Thanks,
> Ravi
>
>
> Thanks,
>
> Ben.
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list