On Sat, Sep 21, 2013 at 10:34 PM, Simon Horman <ho...@verge.net.au> wrote: > On Thu, Sep 19, 2013 at 12:21:33PM -0500, Jesse Gross wrote: >> 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(). > > My assumption was that it would be initialised to zero, > primarily due to the behaviour of __alloc_skb_head(). > Perhaps the core code should be fixed to make my assumption true?
I misunderstood then - I think you can assume that it is initialized to zero, I though you meant that it was initialized to a protocol value. However, I then still have my original question - don't we need to set it here in both cases since we're not just 'initializing' it but actually setting a protocol? >> 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. > > Thanks, that is also not something that I had considered. > > I'm not sure, but I think that we can rely on skb->cb > not being clobbered between rounds of recirculation. > Or at the very least I think we could save and restore it > as necessary. Yes, it should be safe to assume this. > So I think if we could be careful to make sure that inner_protocol > is in a sane state the first time we see the skb but not > each time it is recirculated then I think things should work out. > > In my current implementation of recirculation the datapath > side is driven ovs_dp_process_received_packet(). So by my reasoning > above I think it would make sense to reset the inner_protocol there > and in ovs_packet_cmd_execute() rather than in ovs_execute_actions() > which each of those functions call. I think that would work, however, I wonder if it's the right place in general, independent of this compatibility issue. I guess it still seems like the ideal thing to do is to move this as close to where it is necessary as possible, specifically in mpls_push(). Is there a reason to not put it there (again, other than the out-of-tree compatibility issues)? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
