Re: [PATCH net] Revert "vxlan: fix tos value before xmit"

From: Hangbin Liu <liuhangbin@gmail.com>
To: David Miller <davem@davemloft.net>
Cc: gnault@redhat.com, netdev@vger.kernel.org, pmachata@gmail.com,
	roopa@cumulusnetworks.com, dsahern@kernel.org, akaris@redhat.com
Subject: Re: [PATCH net] Revert "vxlan: fix tos value before xmit"
Date: Wed, 12 Aug 2020 10:28:29 +0800	[thread overview]
Message-ID: <20200812022829.GP2531@dhcp-12-153.nay.redhat.com> (raw)
In-Reply-To: <20200811.170223.1397578654908672695.davem@davemloft.net>

On Tue, Aug 11, 2020 at 05:02:23PM -0700, David Miller wrote:
> From: Hangbin Liu <liuhangbin@gmail.com>
> Date: Thu, 6 Aug 2020 10:52:41 +0800
> 
> > The rough steps are setting vxlan tunnel on OVS. set inner packet tos to
> > 1011 1010 (0xba) and outer vxlan to 1111 1100(0xfc). The outer packet's tos
> > should be 0xfe at latest as it inherit the inner ECN bit. But with RT_TOS(tos)
> > We actually got tos 0x1e as the first 3 bits are omitted.
> > 
> > Now here is detailed testing steps:
> 
> This explains why we need to revert the RT_TOS() change.
> 
> I'm asking what testing you did on the original change that added
> RT_TOS(), which we reverted, and which didn't fix anything.

Oh, I know what you mean now.
> 
> I want to know how we got into this situation in the first place,
> adding a change that only added negative effects.

The reason is still based on the definition of RT_TOS. I have a report
about the difference tos action between geneve and vxlan.

For geneve:

geneve_get_v4_rt()
  - fl4->flowi4_tos = RT_TOS(tos);
geneve_xmit_skb()
  - tos = ip_tunnel_ecn_encap(fl4.flowi4_tos, ip_hdr(skb), skb);

For vxlan:

vxlan_xmit_one()
  - tos = ip_tunnel_ecn_encap(tos, old_iph, skb);

So geneve will use RT_TOS(tos) when xmit, while vxlan will take all tos bits.
At that time I only read the code and thought we should obey the RT_TOS rule,
So I submit the previous patch.

Later Petr Machata remind me that we need to take care of DSCP fields. So I
asked you if we should change RT_TOS() to DSCP_TOS()[1]. You replied

"""
The RT_TOS() value elides the two lowest bits so that we can store other
pieces of binary state into those two lower bits.

So you can't just blindly change the RT_TOS() definition without breaking
a bunch of things.
"""

I'm sorry I didn't take more time to think about the your reply and just
give up my thoughts. Since we bring up this topic again. Would you please
help explain about what "The RT_TOS() value elides the two lowest bits"
means? I'm not sure if you are talking about ECN or not.

[1] https://www.spinics.net/lists/netdev/msg631249.html

Thanks
Hangbin