| Message ID | 20230802075118.409395-11-idosch@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | selftests: forwarding: Various fixes | expand |
Hi Ido, On Wed, Aug 02, 2023 at 10:51:11AM +0300, Ido Schimmel wrote: > MAC Merge cannot be tested with veth pairs, resulting in failures: > > # ./ethtool_mm.sh > [...] > TEST: Manual configuration with verification: swp1 to swp2 [FAIL] > Verification did not succeed > > Fix by skipping the test when used with veth pairs. > > Fixes: e6991384ace5 ("selftests: forwarding: add a test for MAC Merge layer") > Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> > Closes: https://lore.kernel.org/netdev/adc5e40d-d040-a65e-eb26-edf47dac5b02@alu.unizg.hr/ > Signed-off-by: Ido Schimmel <idosch@nvidia.com> > Reviewed-by: Petr Machata <petrm@nvidia.com> > Tested-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> > --- That will skip the selftest just for veth pairs. This will skip it for any device that doesn't support the MAC Merge layer: diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh index c580ad623848..5432848a3c59 100755 --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh @@ -224,6 +224,8 @@ h1_create() hw 1 ethtool --set-mm $h1 pmac-enabled on tx-enabled off verify-enabled off + + h1_created=yes } h2_create() @@ -236,10 +238,16 @@ h2_create() queues 1@0 1@1 1@2 1@3 \ fp P E E E \ hw 1 + + h2_created=yes } h1_destroy() { + if ! [[ $h1_created = yes ]]; then + return + fi + ethtool --set-mm $h1 pmac-enabled off tx-enabled off verify-enabled off tc qdisc del dev $h1 root @@ -249,6 +257,10 @@ h1_destroy() h2_destroy() { + if ! [[ $h2_created = yes ]]; then + return + fi + tc qdisc del dev $h2 root ethtool --set-mm $h2 pmac-enabled off tx-enabled off verify-enabled off @@ -266,6 +278,14 @@ setup_prepare() h1=${NETIFS[p1]} h2=${NETIFS[p2]} + for netif in ${NETIFS[@]}; do + ethtool --show-mm $netif 2>&1 &> /dev/null + if [[ $? -ne 0 ]]; then + echo "SKIP: $netif does not support MAC Merge" + exit $ksft_skip + fi + done + h1_create h2_create } -- 2.34.1 I assume that both situations are equally problematic, and not just veth?
Vladimir Oltean <vladimir.oltean@nxp.com> writes: > Hi Ido, > > On Wed, Aug 02, 2023 at 10:51:11AM +0300, Ido Schimmel wrote: >> MAC Merge cannot be tested with veth pairs, resulting in failures: >> >> # ./ethtool_mm.sh >> [...] >> TEST: Manual configuration with verification: swp1 to swp2 [FAIL] >> Verification did not succeed >> >> Fix by skipping the test when used with veth pairs. >> >> Fixes: e6991384ace5 ("selftests: forwarding: add a test for MAC Merge layer") >> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> >> Closes: https://lore.kernel.org/netdev/adc5e40d-d040-a65e-eb26-edf47dac5b02@alu.unizg.hr/ >> Signed-off-by: Ido Schimmel <idosch@nvidia.com> >> Reviewed-by: Petr Machata <petrm@nvidia.com> >> Tested-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> >> --- > > That will skip the selftest just for veth pairs. This will skip it for > any device that doesn't support the MAC Merge layer: > > diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh > index c580ad623848..5432848a3c59 100755 > --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh > +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh > @@ -224,6 +224,8 @@ h1_create() > hw 1 > > ethtool --set-mm $h1 pmac-enabled on tx-enabled off verify-enabled off > + > + h1_created=yes > } > > h2_create() > @@ -236,10 +238,16 @@ h2_create() > queues 1@0 1@1 1@2 1@3 \ > fp P E E E \ > hw 1 > + > + h2_created=yes > } > > h1_destroy() > { > + if ! [[ $h1_created = yes ]]; then > + return > + fi > + > ethtool --set-mm $h1 pmac-enabled off tx-enabled off verify-enabled off > > tc qdisc del dev $h1 root > @@ -249,6 +257,10 @@ h1_destroy() > > h2_destroy() > { > + if ! [[ $h2_created = yes ]]; then > + return > + fi > + > tc qdisc del dev $h2 root > > ethtool --set-mm $h2 pmac-enabled off tx-enabled off verify-enabled off > @@ -266,6 +278,14 @@ setup_prepare() > h1=${NETIFS[p1]} > h2=${NETIFS[p2]} > > + for netif in ${NETIFS[@]}; do > + ethtool --show-mm $netif 2>&1 &> /dev/null > + if [[ $? -ne 0 ]]; then > + echo "SKIP: $netif does not support MAC Merge" > + exit $ksft_skip > + fi > + done > + Ido, if you decide to go this route, just hoist the loop to the global scope before registering the trap, then you don't need tho hX_created business. > h1_create > h2_create > }
On Wed, Aug 02, 2023 at 02:27:49PM +0200, Petr Machata wrote: > > Vladimir Oltean <vladimir.oltean@nxp.com> writes: > > > Hi Ido, > > > > On Wed, Aug 02, 2023 at 10:51:11AM +0300, Ido Schimmel wrote: > >> MAC Merge cannot be tested with veth pairs, resulting in failures: > >> > >> # ./ethtool_mm.sh > >> [...] > >> TEST: Manual configuration with verification: swp1 to swp2 [FAIL] > >> Verification did not succeed > >> > >> Fix by skipping the test when used with veth pairs. > >> > >> Fixes: e6991384ace5 ("selftests: forwarding: add a test for MAC Merge layer") > >> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> > >> Closes: https://lore.kernel.org/netdev/adc5e40d-d040-a65e-eb26-edf47dac5b02@alu.unizg.hr/ > >> Signed-off-by: Ido Schimmel <idosch@nvidia.com> > >> Reviewed-by: Petr Machata <petrm@nvidia.com> > >> Tested-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> > >> --- > > > > That will skip the selftest just for veth pairs. This will skip it for > > any device that doesn't support the MAC Merge layer: > > > > diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh > > index c580ad623848..5432848a3c59 100755 > > --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh > > +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh > > @@ -224,6 +224,8 @@ h1_create() > > hw 1 > > > > ethtool --set-mm $h1 pmac-enabled on tx-enabled off verify-enabled off > > + > > + h1_created=yes > > } > > > > h2_create() > > @@ -236,10 +238,16 @@ h2_create() > > queues 1@0 1@1 1@2 1@3 \ > > fp P E E E \ > > hw 1 > > + > > + h2_created=yes > > } > > > > h1_destroy() > > { > > + if ! [[ $h1_created = yes ]]; then > > + return > > + fi > > + > > ethtool --set-mm $h1 pmac-enabled off tx-enabled off verify-enabled off > > > > tc qdisc del dev $h1 root > > @@ -249,6 +257,10 @@ h1_destroy() > > > > h2_destroy() > > { > > + if ! [[ $h2_created = yes ]]; then > > + return > > + fi > > + > > tc qdisc del dev $h2 root > > > > ethtool --set-mm $h2 pmac-enabled off tx-enabled off verify-enabled off > > @@ -266,6 +278,14 @@ setup_prepare() > > h1=${NETIFS[p1]} > > h2=${NETIFS[p2]} > > > > + for netif in ${NETIFS[@]}; do > > + ethtool --show-mm $netif 2>&1 &> /dev/null > > + if [[ $? -ne 0 ]]; then > > + echo "SKIP: $netif does not support MAC Merge" > > + exit $ksft_skip > > + fi > > + done > > + > > Ido, if you decide to go this route, just hoist the loop to the global > scope before registering the trap, then you don't need tho hX_created > business. I think the idea was to run this check after verifying that ethtool supports MAC Merge in setup_prepare(). How about moving all these checks before doing any configuration and registering a trap handler? diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh index 4331e2161e8d..39e736f30322 100755 --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh @@ -258,11 +258,6 @@ h2_destroy() setup_prepare() { - check_ethtool_mm_support - check_tc_fp_support - require_command lldptool - bail_on_lldpad "autoconfigure the MAC Merge layer" "configure it manually" - h1=${NETIFS[p1]} h2=${NETIFS[p2]} @@ -278,7 +273,18 @@ cleanup() h1_destroy } -skip_on_veth +check_ethtool_mm_support +check_tc_fp_support +require_command lldptool +bail_on_lldpad "autoconfigure the MAC Merge layer" "configure it manually" + +for netif in ${NETIFS[@]}; do + ethtool --show-mm $netif 2>&1 &> /dev/null + if [[ $? -ne 0 ]]; then + echo "SKIP: $netif does not support MAC Merge" + exit $ksft_skip + fi +done trap cleanup EXIT > > > h1_create > > h2_create > > } > >
On Wed, Aug 02, 2023 at 04:30:30PM +0300, Ido Schimmel wrote: > On Wed, Aug 02, 2023 at 02:27:49PM +0200, Petr Machata wrote: > > Ido, if you decide to go this route, just hoist the loop to the global > > scope before registering the trap, then you don't need tho hX_created > > business. > > I think the idea was to run this check after verifying that ethtool > supports MAC Merge in setup_prepare(). How about moving all these checks > before doing any configuration and registering a trap handler? That should work.
Ido Schimmel <idosch@idosch.org> writes: > On Wed, Aug 02, 2023 at 02:27:49PM +0200, Petr Machata wrote: >> >> Vladimir Oltean <vladimir.oltean@nxp.com> writes: >> >> > @@ -266,6 +278,14 @@ setup_prepare() >> > h1=${NETIFS[p1]} >> > h2=${NETIFS[p2]} >> > >> > + for netif in ${NETIFS[@]}; do >> > + ethtool --show-mm $netif 2>&1 &> /dev/null >> > + if [[ $? -ne 0 ]]; then >> > + echo "SKIP: $netif does not support MAC Merge" >> > + exit $ksft_skip >> > + fi >> > + done >> > + >> >> Ido, if you decide to go this route, just hoist the loop to the global >> scope before registering the trap, then you don't need the hX_created >> business. > > I think the idea was to run this check after verifying that ethtool > supports MAC Merge in setup_prepare(). How about moving all these checks True, I missed that. > before doing any configuration and registering a trap handler? > > diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh > index 4331e2161e8d..39e736f30322 100755 > --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh > +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh > @@ -258,11 +258,6 @@ h2_destroy() > > setup_prepare() > { > - check_ethtool_mm_support > - check_tc_fp_support > - require_command lldptool > - bail_on_lldpad "autoconfigure the MAC Merge layer" "configure it manually" > - > h1=${NETIFS[p1]} > h2=${NETIFS[p2]} > > @@ -278,7 +273,18 @@ cleanup() > h1_destroy > } > > -skip_on_veth > +check_ethtool_mm_support > +check_tc_fp_support > +require_command lldptool > +bail_on_lldpad "autoconfigure the MAC Merge layer" "configure it manually" > + > +for netif in ${NETIFS[@]}; do > + ethtool --show-mm $netif 2>&1 &> /dev/null > + if [[ $? -ne 0 ]]; then > + echo "SKIP: $netif does not support MAC Merge" > + exit $ksft_skip > + fi > +done > > trap cleanup EXIT > Looks good. These checks are usually placed right after sourcing the libraries, but I don't care much one way or another.
diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh index c580ad623848..4331e2161e8d 100755 --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh @@ -278,6 +278,8 @@ cleanup() h1_destroy } +skip_on_veth + trap cleanup EXIT setup_prepare
MAC Merge cannot be tested with veth pairs, resulting in failures: # ./ethtool_mm.sh [...] TEST: Manual configuration with verification: swp1 to swp2 [FAIL] Verification did not succeed Fix by skipping the test when used with veth pairs. Fixes: e6991384ace5 ("selftests: forwarding: add a test for MAC Merge layer") Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> Closes: https://lore.kernel.org/netdev/adc5e40d-d040-a65e-eb26-edf47dac5b02@alu.unizg.hr/ Signed-off-by: Ido Schimmel <idosch@nvidia.com> Reviewed-by: Petr Machata <petrm@nvidia.com> Tested-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> --- Cc: vladimir.oltean@nxp.com --- tools/testing/selftests/net/forwarding/ethtool_mm.sh | 2 ++ 1 file changed, 2 insertions(+)