On Thu, Sep 19, 2013 at 10:57 AM, Simon Horman <ho...@verge.net.au> wrote: > On Mon, Sep 16, 2013 at 03:38:21PM -0500, Jesse Gross wrote: >> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <ho...@verge.net.au> wrote: >> > @@ -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?) > > I believe there are several problems here. > > The first one, which my comment was written around is that I think that if > inner_protocol is a field of struct sk_buff then we can rely on it already > being initialised. However, if we are using compatibility code, where > inner_protcol is called in the callback field of struct sk_buff then I > think that OVS needs to initialise it.
I'm not sure that it's true that inner_protocol is already initialized - I grepped the tree and the only assignment that I found is in skbuff.c in __copy_skb_header(). > A second problem is one that you raise which I had not considered > which is how to handle things if inner_protocol is already set. > > I believe this should only occur in the case where inner_protocol > is a field of struct sk_buff and I think it would be most convenient > to set it conditionally in ovs_skb_reset_inner_protocol(). > I think that if it is not set it should be zero but it should be > safe to check for values less than ETH_P_802_3_MIN. It's probably OK to check for values less than ETH_P_802_3_MIN but I'm not sure that it's the most correct thing to do since skb->protocol could contain these values (such as ETH_P_802_2). It's unlikely that they will be GSO packets but it seems better to use the more strict check against zero. One other consideration in the OVS case - with recirculation we may hit this code multiple times and the difference in behavior could be surprising. However, on the other hand, we need to be careful because skb->cb is not guaranteed to be initialized to zero. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
