[ovs-dev] [PATCH 4/4] User-Space MPLS actions and matches

Simon Horman horms at verge.net.au
Tue Oct 16 04:21:11 UTC 2012


On Mon, Oct 15, 2012 at 10:40:12AM -0700, Ben Pfaff wrote:
> set_ethertype() states:
>         /* ethtype for VLAN packets is at L3_offset - 2 bytes. */
> but I believe that this is also true for non-VLAN packets, possibly
> allowing the code to be simplified.  Actually I guess that's not true
> if there's an L2.5 header.
> 
> I think that the location of the ethertype is, then:
>         just before the L2.5 header, if there is one;
>         otherwise just before the L3 header.
> get_ethertype() has essentially this logic, but if we wrote a function
> that returns a pointer to the ethertype, then set_ethertype() and
> get_ethertype() could be one-liners written in terms of that function.
> 
> In get_label_ttl_and_tc(), I don't think that the packet->size
> comparisons are entirely correct, because one must allow for VLAN and
> MPLS headers in the size calculations.  I think it would be better to
> compute the size past the l3 pointer, e.g. something like
> ofpbuf_tail(packet) - packet->l3 although there'd need to be some
> casts.
> 
> I think that the get_label_ttl_and_tc() interface would be easier to
> use if it just returned an MPLS label.  The caller can "or" in a BOS
> bit after it returns, or the caller could provide a BOS value for
> get_label_ttl_and_tc() to use.
> 
> I think set_new_mpls_lse() can be removed.  I don't see any benefit to
> it over setting the mpls_lse member directly.
> 
> copy_mpls_ttl_in() duplicates code from packet_set_ipv4() for setting
> the TTL.  It would be better to factor this out into a helper
> function.
> 
> It's not obvious to me that copy_mpls_ttl_in() and copy_mpls_ttl_out()
> are picky enough about packet length.
> 
> copy_mpls_ttl_out() has a comment that I don't understand
>     /* TTL sent from ofproto-dpif.c is not the correct one,
>      * hence ignore it. */
> 
> copy_mpls_ttl_in() and copy_mpls_ttl_out() both have comments that
> ->l2_5 must point to an MPLS header, but they still check for an MPLS
> ethertype.  Is the check unnecessary, or is the comment wrong, or is
> something else at work?

I have removed the ttl functions above, they are not used at this time.

> 
> pop_mpls() has an "if" without {} around its statement.
> 
> The IP6_TC macro that this introduces is not used anywhere.  It seems
> to me that, if we need this, it would be better written as a function
> so as to emphasize that its parameter must be in host byte order.
> 
> There is a spurious change to subfacet_should_install().
> 
> The new assertion in execute_controller_action() makes me say "ugh".
> I'd rather just delete it, I think.
> 
> Won't execute_controller_action() always push on an extra MPLS label,
> if there's to be one at all?  Presumably it should just modify the
> existing label if there is one.

Yes. I believe this is fixed by the stack depth tracking change
that Yamahata-san suggested.

> compose_mpls_push_action() would seem to be a good place to use
> set_mpls_lse_values().
> 
> In do_xlate_actions(), we could get rid of a couple of lines and a
> variable simply by calling ofpact_get_REG_LOAD() inside the argument
> of nxm_execute_reg_load().

I have dropped the change for REG_LOAD in do_xlate_actions(),
it seems spurious at this point.

> The tests don't actually run the test-mpls program.  Let's just get
> rid of it.

I will post a fresh version of the MPLS series that addresses
all of the issues you have raised above.



More information about the dev mailing list