| Message ID | 20210319143314.2731608-1-liuhangbin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net] selftests: forwarding: vxlan_bridge_1d: Fix vxlan ecn decapsulate value | expand |
On Fri, Mar 19, 2021 at 10:33:14PM +0800, Hangbin Liu wrote: > The ECN bit defines ECT(1) = 1, ECT(0) = 2. So inner 0x02 + outer 0x01 > should be inner ECT(0) + outer ECT(1). Based on the description of > __INET_ECN_decapsulate, the final decapsulate value should be > ECT(1). So fix the test expect value to 0x01. > > Before the fix: > TEST: VXLAN: ECN decap: 01/02->0x02 [FAIL] > Expected to capture 10 packets, got 0. > > After the fix: > TEST: VXLAN: ECN decap: 01/02->0x01 [ OK ] > > Fixes: a0b61f3d8ebf ("selftests: forwarding: vxlan_bridge_1d: Add an ECN decap test") > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> Fixes: b723748750ec ("tunnel: Propagate ECT(1) when decapsulating as recommended by RFC6040") The commit you cited is from 2018 whereas this one is from 2020. The test stopped working after the latter. The reason I didn't see it is because this commit only changed one caller of __INET_ECN_decapsulate(). Another caller is mlxsw which uses the function to understand how to program the hardware to perform decapsulation. See commit 28e450333d4d ("inet: Refactor INET_ECN_decapsulate()"). After your patch I get: TEST: VXLAN: ECN decap: 01/02->0x01 [FAIL] Expected to capture 10 packets, got 0. Fixed by: diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ipip.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ipip.c index b8b08a6a1d10..61eb34e20fde 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ipip.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ipip.c @@ -341,7 +341,12 @@ static int mlxsw_sp_ipip_ecn_decap_init_one(struct mlxsw_sp *mlxsw_sp, u8 new_inner_ecn; trap_en = __INET_ECN_decapsulate(outer_ecn, inner_ecn, &set_ce); - new_inner_ecn = set_ce ? INET_ECN_CE : inner_ecn; + if (set_ce) + new_inner_ecn = INET_ECN_CE; + else if (outer_ecn == INET_ECN_ECT_1) + new_inner_ecn = INET_ECN_ECT_1; + else + new_inner_ecn = inner_ecn; mlxsw_reg_tidem_pack(tidem_pl, outer_ecn, inner_ecn, new_inner_ecn, trap_en, trap_en ? MLXSW_TRAP_ID_DECAP_ECN0 : 0); diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve.c index e5ec595593f4..74f2c4ce7063 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve.c @@ -913,7 +913,12 @@ static int __mlxsw_sp_nve_ecn_decap_init(struct mlxsw_sp *mlxsw_sp, u8 new_inner_ecn; trap_en = !!__INET_ECN_decapsulate(outer_ecn, inner_ecn, &set_ce); - new_inner_ecn = set_ce ? INET_ECN_CE : inner_ecn; + if (set_ce) + new_inner_ecn = INET_ECN_CE; + else if (outer_ecn == INET_ECN_ECT_1) + new_inner_ecn = INET_ECN_ECT_1; + else + new_inner_ecn = inner_ecn; mlxsw_reg_tndem_pack(tndem_pl, outer_ecn, inner_ecn, new_inner_ecn, trap_en, trap_en ? MLXSW_TRAP_ID_DECAP_ECN0 : 0); I will prepare a patch > --- > tools/testing/selftests/net/forwarding/vxlan_bridge_1d.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/net/forwarding/vxlan_bridge_1d.sh b/tools/testing/selftests/net/forwarding/vxlan_bridge_1d.sh > index ce6bea9675c0..0ccb1dda099a 100755 > --- a/tools/testing/selftests/net/forwarding/vxlan_bridge_1d.sh > +++ b/tools/testing/selftests/net/forwarding/vxlan_bridge_1d.sh > @@ -658,7 +658,7 @@ test_ecn_decap() > # In accordance with INET_ECN_decapsulate() > __test_ecn_decap 00 00 0x00 > __test_ecn_decap 01 01 0x01 > - __test_ecn_decap 02 01 0x02 > + __test_ecn_decap 02 01 0x01 > __test_ecn_decap 01 03 0x03 > __test_ecn_decap 02 03 0x03 > test_ecn_decap_error > -- > 2.26.2 >
On Sun, Mar 21, 2021 at 07:20:55PM +0200, Ido Schimmel wrote: > On Fri, Mar 19, 2021 at 10:33:14PM +0800, Hangbin Liu wrote: > > The ECN bit defines ECT(1) = 1, ECT(0) = 2. So inner 0x02 + outer 0x01 > > should be inner ECT(0) + outer ECT(1). Based on the description of > > __INET_ECN_decapsulate, the final decapsulate value should be > > ECT(1). So fix the test expect value to 0x01. > > > > Before the fix: > > TEST: VXLAN: ECN decap: 01/02->0x02 [FAIL] > > Expected to capture 10 packets, got 0. > > > > After the fix: > > TEST: VXLAN: ECN decap: 01/02->0x01 [ OK ] > > > > Fixes: a0b61f3d8ebf ("selftests: forwarding: vxlan_bridge_1d: Add an ECN decap test") > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > > Fixes: b723748750ec ("tunnel: Propagate ECT(1) when decapsulating as recommended by RFC6040") > > The commit you cited is from 2018 whereas this one is from 2020. The > test stopped working after the latter. The reason I didn't see it is > because this commit only changed one caller of __INET_ECN_decapsulate(). > Another caller is mlxsw which uses the function to understand how to > program the hardware to perform decapsulation. See commit 28e450333d4d > ("inet: Refactor INET_ECN_decapsulate()"). Hi Ido, Thanks for this info. I don't have mlxsw card in hand, so I never run the driver test before. Cheers Hangbin > > After your patch I get: > > TEST: VXLAN: ECN decap: 01/02->0x01 [FAIL] > Expected to capture 10 packets, got 0. > > Fixed by: > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ipip.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ipip.c > index b8b08a6a1d10..61eb34e20fde 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ipip.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ipip.c > @@ -341,7 +341,12 @@ static int mlxsw_sp_ipip_ecn_decap_init_one(struct mlxsw_sp *mlxsw_sp, > u8 new_inner_ecn; > > trap_en = __INET_ECN_decapsulate(outer_ecn, inner_ecn, &set_ce); > - new_inner_ecn = set_ce ? INET_ECN_CE : inner_ecn; > + if (set_ce) > + new_inner_ecn = INET_ECN_CE; > + else if (outer_ecn == INET_ECN_ECT_1) > + new_inner_ecn = INET_ECN_ECT_1; > + else > + new_inner_ecn = inner_ecn; > > mlxsw_reg_tidem_pack(tidem_pl, outer_ecn, inner_ecn, new_inner_ecn, > trap_en, trap_en ? MLXSW_TRAP_ID_DECAP_ECN0 : 0); > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve.c > index e5ec595593f4..74f2c4ce7063 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve.c > @@ -913,7 +913,12 @@ static int __mlxsw_sp_nve_ecn_decap_init(struct mlxsw_sp *mlxsw_sp, > u8 new_inner_ecn; > > trap_en = !!__INET_ECN_decapsulate(outer_ecn, inner_ecn, &set_ce); > - new_inner_ecn = set_ce ? INET_ECN_CE : inner_ecn; > + if (set_ce) > + new_inner_ecn = INET_ECN_CE; > + else if (outer_ecn == INET_ECN_ECT_1) > + new_inner_ecn = INET_ECN_ECT_1; > + else > + new_inner_ecn = inner_ecn; > > mlxsw_reg_tndem_pack(tndem_pl, outer_ecn, inner_ecn, new_inner_ecn, > trap_en, trap_en ? MLXSW_TRAP_ID_DECAP_ECN0 : 0); > > I will prepare a patch
diff --git a/tools/testing/selftests/net/forwarding/vxlan_bridge_1d.sh b/tools/testing/selftests/net/forwarding/vxlan_bridge_1d.sh index ce6bea9675c0..0ccb1dda099a 100755 --- a/tools/testing/selftests/net/forwarding/vxlan_bridge_1d.sh +++ b/tools/testing/selftests/net/forwarding/vxlan_bridge_1d.sh @@ -658,7 +658,7 @@ test_ecn_decap() # In accordance with INET_ECN_decapsulate() __test_ecn_decap 00 00 0x00 __test_ecn_decap 01 01 0x01 - __test_ecn_decap 02 01 0x02 + __test_ecn_decap 02 01 0x01 __test_ecn_decap 01 03 0x03 __test_ecn_decap 02 03 0x03 test_ecn_decap_error
The ECN bit defines ECT(1) = 1, ECT(0) = 2. So inner 0x02 + outer 0x01 should be inner ECT(0) + outer ECT(1). Based on the description of __INET_ECN_decapsulate, the final decapsulate value should be ECT(1). So fix the test expect value to 0x01. Before the fix: TEST: VXLAN: ECN decap: 01/02->0x02 [FAIL] Expected to capture 10 packets, got 0. After the fix: TEST: VXLAN: ECN decap: 01/02->0x01 [ OK ] Fixes: a0b61f3d8ebf ("selftests: forwarding: vxlan_bridge_1d: Add an ECN decap test") Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- tools/testing/selftests/net/forwarding/vxlan_bridge_1d.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)