Re: [PATCH net-next 03/11] vxlan: Allow configuration of DF behaviour

[Stefano Brivio <sbrivio@redhat.com>]

Stephen, thanks for reviewing this.

On Tue, 6 Nov 2018 21:00:18 -0800
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Tue,  6 Nov 2018 22:38:59 +0100
> Stefano Brivio <sbrivio@redhat.com> 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