Stephen, thanks for reviewing this.
On Tue, 6 Nov 2018 21:00:18 -0800
Stephen Hemminger <step...@networkplumber.org> wrote:
> On Tue, 6 Nov 2018 22:38:59 +0100
> Stefano Brivio <sbri...@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
