From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Brivio Subject: Re: [PATCH net-next 03/11] vxlan: Allow configuration of DF behaviour Date: Wed, 7 Nov 2018 11:35:48 +0100 Message-ID: <20181107113548.17ec930d@redhat.com> References: <1922d1e13074e73435724523f901f2c97eb3a764.1541533786.git.sbrivio@redhat.com> <20181106210018.08f45eb0@shemminger-XPS-13-9360> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: "David S. Miller" , Sabrina Dubroca , Xin Long , netdev@vger.kernel.org To: Stephen Hemminger Return-path: Received: from mx1.redhat.com ([209.132.183.28]:42710 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726225AbeKGUFl (ORCPT ); Wed, 7 Nov 2018 15:05:41 -0500 In-Reply-To: <20181106210018.08f45eb0@shemminger-XPS-13-9360> Sender: netdev-owner@vger.kernel.org List-ID: Stephen, thanks for reviewing this. On Tue, 6 Nov 2018 21:00:18 -0800 Stephen Hemminger wrote: > On Tue, 6 Nov 2018 22:38:59 +0100 > Stefano Brivio wrote: > > > df = htons(IP_DF); > > } > > > > + if (!df) { > > + if (vxlan->cfg.df == VXLAN_DF_SET) { > > + df = htons(IP_DF); > > I am confused, this looks like this new flag is duplicating the exiting tunnel DF flag. > (in info->key.tun.flags). Why is another flag needed? The reason is that for non-lwt tunnels the tunnel key is not used in VXLAN, and this patch is intended for non-lwt tunnels only, as external control planes already have a way to set the DF bit. But now I see that the way I wrote this is misleading: this should really be in an if (rdst) or if (!info) clause. I'll place this into the if (!info) block just above. TTL and TOS are handled in a similar way, using the if (rdst) clause above. Functionally, it's equivalent, because external control planes will never set non-default values for vxlan->cfg.df, but still not a good reason to write it this way. Similar story for GENEVE, I will place that under if (!geneve->collect_md) instead. With a notable difference, though: GENEVE already uses struct ip_tunnel_key for some of its interface configuration, so I had already thought about adding a TUNNEL_DONT_FRAGMENT_INHERIT flag that could be used in tun_flags. However, I would use the last available bit there, and this wouldn't be terribly useful: an external control plane already has access to the inner DF bit, and would most likely decide on its own whether to set DF or not, by setting that in tun_flags. So I'd rather keep that in struct geneve_dev. -- Stefano