On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <ho...@verge.net.au> wrote: > diff --git a/datapath/actions.c b/datapath/actions.c > index 6741d81..2335014 100644 > --- a/datapath/actions.c > +++ b/datapath/actions.c > +/* Push MPLS after the ethernet header. */ > +static int push_mpls(struct sk_buff *skb, > + const struct ovs_action_push_mpls *mpls) [...] > + /* update mac_len for subsequent pop_mpls actions. */ > + skb->mac_len += VLAN_HLEN;
Is this supposed to be part of put_vlan()? [...] > + if (skb->ip_summed == CHECKSUM_COMPLETE) > + skb->csum = csum_add(skb->csum, csum_partial(new_mpls_lse, > + MPLS_HLEN, 0)); > + > + hdr = (struct ethhdr *)(skb_mac_header(skb)); I think you could simplify this by using eth_hdr(). > @@ -616,6 +736,13 @@ int ovs_execute_actions(struct datapath *dp, struct > sk_buff *skb) > goto out_loop; > } > > + /* Needed to initialise inner protocol on kernels older > + * than v3.11 where skb->inner_protocol is not present > + * and compatibility code uses the OVS_CB(skb) to store > + * the inner protocol. > + */ > + ovs_skb_set_inner_protocol(skb, skb->protocol); The comment makes it sound like this code should just be deleted when upstreaming. However, I believe that we still need to initialize this field, right? Is this the best place do it or should it be conditional on adding a first MPLS header? (i.e. what happens if inner_protocol is already set and the packet simply passes through OVS?) > diff --git a/datapath/vport-netdev.c b/datapath/vport-netdev.c > index 215a47e..69f24d1 100644 > --- a/datapath/vport-netdev.c > +++ b/datapath/vport-netdev.c > @@ -287,8 +291,17 @@ static int netdev_send(struct vport *vport, struct > sk_buff *skb) > + inner_protocol = ovs_skb_get_inner_protocol(skb); > + if (eth_p_mpls(skb->protocol) && !eth_p_mpls(inner_protocol)) > + mpls = true; > + > + if (vlan_tx_tag_present(skb) && !dev_supports_vlan_tx(skb->dev)) > + vlan = true; A couple of thoughts on this: - I'm not sure that the check on inner_protocol is right since I don't think it will be set to an MPLS EtherType in any real situation. I think you can just drop it. - Can we simplify the loop by just inserting the vlan tag (if necessary), calling skb_gso_segment(), and sending the packet? I think it should be possible now that our skb_gso_segment() backport is more robust and it would make things easier to read. - Can we have some kind of version check for MPLS, similar to what we do for VLAN, so that we don't call into this on kernels >= 3.11? These are all pretty targeted comments though, as was the one in the previous kernel patch so I am basically happy with this overall. To move forward with this: - Pravin, would you mind double checking the GSO compat code? - Ben, do you want to take over for the userspace portions of the series on the assumption that the above comments can be fixed fairly easily? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
