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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
