On Tue, May 21, 2013 at 4:39 PM, Jesse Gross <je...@nicira.com> wrote: > On Mon, May 20, 2013 at 2:33 PM, Pravin B Shelar <pshe...@nicira.com> wrote: >> Following patch restructures ovs tunneling and gre vport >> implementation to make ovs tunneling more in sync with >> upstream kernel tunneling. Doing this tunneling code is >> simplified as most of protocol processing on send and >> recv is pushed to kernel tunneling. For external ovs >> module the code is moved to kernel compatibility code. >> >> Signed-off-by: Pravin B Shelar <pshe...@nicira.com> > > A couple more comments (just on the non-compat portions): > >> diff --git a/datapath/vport-gre.c b/datapath/vport-gre.c >> index add17d9..f551976 100644 >> --- a/datapath/vport-gre.c >> +++ b/datapath/vport-gre.c >> -static __be32 be64_get_high32(__be64 x) >> +static __be16 ovs_tnl_flags_to_gre_flags(u16 tun_flags) > [...] >> + __be16 flags = 0; >> + if (tun_flags & OVS_TNL_F_CSUM) >> + flags |= TUNNEL_CSUM; >> + if (tun_flags & OVS_TNL_F_KEY) >> + flags |= TUNNEL_KEY; >> + return flags; >> } > > Is there any way that we can unify the tunnel flags values to avoid > needing to do this conversion? I know the existing flags are based on > the GRE values, so maybe we should push the conversion down there. > OVS can directly use TUNNEL_* flags, we just need to define OVS_TNL_F_DONT_FRAGMENT equivalent there.
>> /* Called with rcu_read_lock and BH disabled. */ >> -static int gre_rcv(struct sk_buff *skb) >> +static int gre_rcv(struct sk_buff *skb, >> + const struct tnl_ptk_info *tpi) > [...] >> + hdr_len = _parse_header(tpi, &tnl_flags, &key); > > I would probably start the function name with a double underscore, > just to be more consistent. > ok >> ovs_net = net_generic(dev_net(skb->dev), ovs_net_id); >> - if (is_gre64) >> + if (tpi->flags & TUNNEL_SEQ) > > I think the GRE64 check is not quite the same as before - it > previously required both the key and the sequence number to be set, > which seems more ideal to me. > ok. >> vport = rcu_dereference(ovs_net->vport_net.gre64_vport); >> else >> vport = rcu_dereference(ovs_net->vport_net.gre_vport); > > If neither of these vports are set, then should we return the packet > back to the handler instead of accepting and freeing it? If so, then > we might also want to move the check earlier in the function since it > doesn't depend on the previous results. ok, I ovs can return skb if there are no gre port. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
