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

[Simon Horman]

On Tue, Jan 22, 2013 at 03:41:37PM -0800, Ben Pfaff wrote:
> On Tue, Jan 22, 2013 at 10:47:17AM -0800, Ben Pfaff wrote:
> > On Fri, Jan 18, 2013 at 09:27:55AM +0900, Simon Horman wrote:
> > > This patch implements use-space datapath and non-datapath code
> > > to match and use the datapath API set out in Leo Alterman's patch
> > > "user-space datapath: Add basic MPLS support to kernel".
> > 
> > I'm still reading, I'll continue later.
> 
> Here I go.
> 
> I noticed just now that the code added to execute_set_action() in
> dpif-netdev.c is indented four spaces too far.
> 
> I get one build failure:
>     cc1: warnings being treated as errors
>     ../lib/ofp-actions.c: In function ‘ofpact_from_nxast’:
>     ../lib/ofp-actions.c:407: error: initialization from incompatible pointer type
> on this line:
>         struct nx_action_push_mpls *nxapm = (struct nx_action_push *)a;

Sorry for letting that slip through.

Somehow -Werror wasn't enabled. It is now.

> In packets.c, a lot of the new functions check that the packet is at
> least sizeof(struct eth_header) bytes long.  I think that we should
> simply make this a requirement and not bother checking for it.  There
> shouldn't be any code passing in packets that are not at least 14
> bytes long.  (Or if you know of any, let's fix them.)

Sure, I have made it so.
I'm not aware of any cases where this requirement is not met.

> push_mpls() is unnecessarily confusing because it has variables named
> 'ethtype' and 'eth_type'.  I'd drop the latter entirely and rewrite
>     if (!eth_type_mpls(eth_type)) {
> as
>     if (!eth_type_mpls(get_ethertype(packet))) {

Agreed, it was not 5 minutes ago when this most recently confused me.

> pop_mpls() has the same variable name confusion, please fix it there
> too.

Done.

> I think that we should try to maintain an invariant that l2_5 is
> nonnull if and only if the ethertype is an mpls ethertype.  Then we
> could simplify some of the code here by checking only for l2_5 !=
> NULL and not bothering with ethertypes.  Actually it looks like we
> already maintain this invariant.

Actually, I believe that is not true. As explained in my previous email
I believe that in the case where compose_flow() calls push_mpls()
for an MPLS frame then l2_5 is not set although the ethtype is an MPLS
ethtype. In my previous email I proposed a fix by having push_mpls()
ensure that l2_5 is set.

That said, I think it is easy enough to switch things around so that
invariant is true and make the simplification you suggest. I will see about
doing so.


> eth_mpls_depth() should take a "const" parameter.

Done.

> struct mpls_eth_header isn't used anywhere.  Do you think it is
> useful?

No, I don't think it is useful. I have removed it.

> IP_DSCP doesn't properly parenthesize its argument.  Also it isn't
> used anywhere.

Thanks, I have removed it.

> IP6_VERSION and IP6_VER aren't used anywhere.

Also removed.

> Why does compose_output_action__() save and restore mpls_lse and
> mpls_depth?  It saves nw_tos and vlan_tci because the output process
> might need to temporarily change those, for output ports with
> configured priorities and for VLAN splinters, respectively, but I
> don't know of anything similar for MPLS.
> 

This is most likely because I didn't understand the motivation for saving
nw_tos and vlan_tci, and decided to follow the same pattern for MPLS.
I will remove the offending code.

> Most of the functions called by do_xlate_actions() have "execute" or
> "xlate" in their names, so perhaps compose_mpls_push_action() and
> compose_mpls_pop_action() should follow this pattern.

Sure, I will s/compose_/execute_/

> In compose_mpls_push_action(), for the case where there's already an
> MPLS header, do you know why we're getting the LSE from base_flow?
> I'd think we could just keep the current one.

Yes, true. I will remove the following line:

	ctx->flow.mpls_lse = ctx->base_flow.mpls_lse;

> For the other case,
> this:
> +        ctx->flow.mpls_lse &= ~htonl(MPLS_LABEL_MASK | MPLS_TC_MASK |
> +                                     MPLS_TTL_MASK | MPLS_BOS_MASK);
> seems equivalent to just "ctx->flow.mpls_lse = htonl(0);".  But later
> on the code writes to mpls_lse without any intervening read, so maybe
> we can just delete the &= statement.  

Also true. I will delete the &= statement.

> I'm not sure why compose_mpls_pop_action() emits
> OVS_ACTION_ATTR_POP_MPLS instead of just manipulating mpls_depth and
> other flow fields.

Good point. I'll see about changing things as you suggest.

> I think I'm done reviewing this patch for now.

Thanks. I'll try and address all the points you raised here and elsewhere
and get a fresh revision to the mailing list soon.