[RFC,net-next] selftests: forwarding: add a test for MAC Merge layer (2023)

The MAC Merge layer (IEEE 802.3-2018 clause 99) does all the heavylifting for Frame Preemption (IEEE 802.1Q-2018 clause 6.7.2), a TSNfeature for minimizing latency.Preemptible traffic is different on the wire from normal traffic inincompatible ways. If we send a preemptible packet and the link partnerdoesn't support preemption, it will drop it as an error frame and wewill never know. The MAC Merge layer has a control plane of its own,which can be manipulated (using ethtool) in order to negotiate thiscapability with the link partner (through LLDP).Actually the TLV format for LLDP solves this problem only partly,because both partners only advertise:- if they support preemption (RX and TX)- if they have enabled preemption (TX)so we cannot tell the link partner what to do - we cannot force it toenable reception of our preemptible packets.That is fully solved by the verification feature, where the local devicegenerates some small probe frames which look like preemptible frameswith no useful content, and the link partner is obliged to respond tothem if it supports the standard. If the verification times out, we knowthat preemption isn't active in our TX direction on the link.Having clarified the definition, this selftest exercises the manual(ethtool) configuration path of 2 link partners (with and withoutverification), and the LLDP code path, using the openlldp project.This is not really a "forwarding" selftest, but I put it near the other"ethtool" selftests.Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>---This is more for the sake of the discussion, because neither the ethtoolnor the openlldp changes for MM were accepted yet.Not sure if this is the best way to integrate lldpad into a kselftest,given the fact that systemd starts it with a persistent configurationfile. .../testing/selftests/net/forwarding/Makefile | 1 + .../selftests/net/forwarding/ethtool_mm.sh | 174 ++++++++++++++++++ tools/testing/selftests/net/forwarding/lib.sh | 9 + 3 files changed, 184 insertions(+) create mode 100755 tools/testing/selftests/net/forwarding/ethtool_mm.sh

Vladimir Oltean <vladimir.oltean@nxp.com> writes:> +# Borrowed from mlxsw/qos_lib.sh, message adapted.> +bail_on_lldpad()> +{> +if systemctl is-active --quiet lldpad; then> +cat >/dev/stderr <<-EOF> +WARNING: lldpad is running> +> +lldpad will likely autoconfigure the MAC Merge layer,> +while this test will configure it manually. One of them> +is arbitrarily going to overwrite the other. That will> +cause spurious failures (or, unlikely, passes) of this> +test.> +EOF> +exit 1> +fi> +}This would be good to have unified. Can you make the function reusable,with a generic or parametrized message? I should be able to carve a bitof time later to move it to lib.sh, migrate the mlxsw selftests, anddrop the qos_lib.sh copy.

Hi Petr,On Mon, Feb 13, 2023 at 11:51:37AM +0100, Petr Machata wrote:> Vladimir Oltean <vladimir.oltean@nxp.com> writes:> > > +# Borrowed from mlxsw/qos_lib.sh, message adapted.> > +bail_on_lldpad()> > +{> > +if systemctl is-active --quiet lldpad; then> > +cat >/dev/stderr <<-EOF> > +WARNING: lldpad is running> > +> > +lldpad will likely autoconfigure the MAC Merge layer,> > +while this test will configure it manually. One of them> > +is arbitrarily going to overwrite the other. That will> > +cause spurious failures (or, unlikely, passes) of this> > +test.> > +EOF> > +exit 1> > +fi> > +}> > This would be good to have unified. Can you make the function reusable,> with a generic or parametrized message? I should be able to carve a bit> of time later to move it to lib.sh, migrate the mlxsw selftests, and> drop the qos_lib.sh copy.Maybe like this?diff --git a/tools/testing/selftests/drivers/net/mlxsw/qos_headroom.sh b/tools/testing/selftests/drivers/net/mlxsw/qos_headroom.shindex 3569ff45f7d5..258b4c994bc3 100755--- a/tools/testing/selftests/drivers/net/mlxsw/qos_headroom.sh+++ b/tools/testing/selftests/drivers/net/mlxsw/qos_headroom.sh@@ -371,7 +371,7 @@ test_tc_int_buf() tc qdisc delete dev $swp root } -bail_on_lldpad+bail_on_lldpad "configure DCB" "configure Qdiscs" trap cleanup EXIT setup_waitdiff --git a/tools/testing/selftests/drivers/net/mlxsw/qos_lib.sh b/tools/testing/selftests/drivers/net/mlxsw/qos_lib.shindex faa51012cdac..5ad092b9bf10 100644--- a/tools/testing/selftests/drivers/net/mlxsw/qos_lib.sh+++ b/tools/testing/selftests/drivers/net/mlxsw/qos_lib.sh@@ -54,31 +54,3 @@ measure_rate() echo $ir $er return $ret }--bail_on_lldpad()-{-if systemctl is-active --quiet lldpad; then--cat >/dev/stderr <<-EOF-WARNING: lldpad is running--lldpad will likely configure DCB, and this test will-configure Qdiscs. mlxsw does not support both at the-same time, one of them is arbitrarily going to overwrite-the other. That will cause spurious failures (or,-unlikely, passes) of this test.-EOF--if [[ -z $ALLOW_LLDPAD ]]; then-cat >/dev/stderr <<-EOF--If you want to run the test anyway, please set-an environment variable ALLOW_LLDPAD to a-non-empty string.-EOF-exit 1-else-return-fi-fi-}diff --git a/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh b/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.shindex f9858e221996..2676fca18e06 100755--- a/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh+++ b/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh@@ -393,7 +393,7 @@ test_qos_pfc() log_test "PFC" } -bail_on_lldpad+bail_on_lldpad "configure DCB" "configure Qdiscs" trap cleanup EXIT setup_preparediff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_ets.shindex ceaa76b17a43..99495f5fa63d 100755--- a/tools/testing/selftests/drivers/net/mlxsw/sch_ets.sh+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_ets.sh@@ -78,5 +78,5 @@ collect_stats() done } -bail_on_lldpad+bail_on_lldpad "configure DCB" "configure Qdiscs" ets_rundiff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.shindex 0d01c7cd82a1..8ecddafa79b3 100755--- a/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh@@ -166,7 +166,7 @@ ecn_mirror_test() uninstall_qdisc } -bail_on_lldpad+bail_on_lldpad "configure DCB" "configure Qdiscs" trap cleanup EXIT setup_preparediff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.shindex 860205338e6f..159108d02895 100755--- a/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh@@ -73,7 +73,7 @@ red_mirror_test() uninstall_qdisc } -bail_on_lldpad+bail_on_lldpad "configure DCB" "configure Qdiscs" trap cleanup EXIT setup_preparediff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.shindex c6ce0b448bf3..bf57400e14ee 100755--- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh@@ -2,7 +2,7 @@ # SPDX-License-Identifier: GPL-2.0 source qos_lib.sh-bail_on_lldpad+bail_on_lldpad "configure DCB" "configure Qdiscs" lib_dir=$(dirname $0)/../../../net/forwarding TCFLAGS=skip_swdiff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.shindex 8d245f331619..b95e95e3b041 100755--- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh@@ -2,7 +2,7 @@ # SPDX-License-Identifier: GPL-2.0 source qos_lib.sh-bail_on_lldpad+bail_on_lldpad "configure DCB" "configure Qdiscs" lib_dir=$(dirname $0)/../../../net/forwarding TCFLAGS=skip_swdiff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.shindex 013886061f15..c495cb8d38f7 100755--- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh@@ -2,7 +2,7 @@ # SPDX-License-Identifier: GPL-2.0 source qos_lib.sh-bail_on_lldpad+bail_on_lldpad "configure DCB" "configure Qdiscs" lib_dir=$(dirname $0)/../../../net/forwarding TCFLAGS=skip_swdiff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.shindex 30311214f39f..ab1c2f839167 100755--- a/tools/testing/selftests/net/forwarding/lib.sh+++ b/tools/testing/selftests/net/forwarding/lib.sh@@ -1896,3 +1896,34 @@ mldv1_done_get() payload_template_expand_checksum "$hbh$icmpv6" $checksum }++bail_on_lldpad()+{+local reason1="$1"; shift+local reason2="$1"; shift++if systemctl is-active --quiet lldpad; then++cat >/dev/stderr <<-EOF+WARNING: lldpad is running++lldpad will likely $reason1, and this test will+$reason2. Both are not supported at the same time,+one of them is arbitrarily going to overwrite the+other. That will cause spurious failures (or, unlikely,+passes) of this test.+EOF++if [[ -z $ALLOW_LLDPAD ]]; then+cat >/dev/stderr <<-EOF++If you want to run the test anyway, please set+an environment variable ALLOW_LLDPAD to a+non-empty string.+EOF+exit 1+else+return+fi+fi+}

On Sat Feb 11 2023, Vladimir Oltean wrote:> The MAC Merge layer (IEEE 802.3-2018 clause 99) does all the heavy> lifting for Frame Preemption (IEEE 802.1Q-2018 clause 6.7.2), a TSN> feature for minimizing latency.>> Preemptible traffic is different on the wire from normal traffic in> incompatible ways. If we send a preemptible packet and the link partner> doesn't support preemption, it will drop it as an error frame and we> will never know. The MAC Merge layer has a control plane of its own,> which can be manipulated (using ethtool) in order to negotiate this> capability with the link partner (through LLDP).>> Actually the TLV format for LLDP solves this problem only partly,> because both partners only advertise:> - if they support preemption (RX and TX)> - if they have enabled preemption (TX)> so we cannot tell the link partner what to do - we cannot force it to> enable reception of our preemptible packets.>> That is fully solved by the verification feature, where the local device> generates some small probe frames which look like preemptible frames> with no useful content, and the link partner is obliged to respond to> them if it supports the standard. If the verification times out, we know> that preemption isn't active in our TX direction on the link.>> Having clarified the definition, this selftest exercises the manual> (ethtool) configuration path of 2 link partners (with and without> verification), and the LLDP code path, using the openlldp project.>> This is not really a "forwarding" selftest, but I put it near the other> "ethtool" selftests.>> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>> ---Looks good to me. Is there any way to test this? I see mm supportimplemented for enetc and Felix. However, I only have access to Inteli225 NIC(s) which support frame preemption.Thanks,Kurt

On Mon, Feb 13, 2023 at 12:42:04PM +0100, Kurt Kanzenbach wrote:> Looks good to me. Is there any way to test this?Only with driver level support implemented.> I see mm support implemented for enetc and Felix.Yes, those would be the Ethernet interfaces on the NXP LS1028A SoC.That is all hardware I have access to.> However, I only have access to Intel i225 NIC(s) which support frame> preemption.I suppose this patch set would need to be rebased to the current net-nextand adapted to the merged version of the ethtool API:https://patchwork.kernel.org/project/netdevbpf/cover/20220520011538.1098888-1-vinicius.gomes@intel.com/

Vladimir Oltean <vladimir.oltean@nxp.com> writes:> Hi Petr,>> On Mon, Feb 13, 2023 at 11:51:37AM +0100, Petr Machata wrote:>> Vladimir Oltean <vladimir.oltean@nxp.com> writes:>> >> > +# Borrowed from mlxsw/qos_lib.sh, message adapted.>> > +bail_on_lldpad()>> > +{>> > +if systemctl is-active --quiet lldpad; then>> > +cat >/dev/stderr <<-EOF>> > +WARNING: lldpad is running>> > +>> > +lldpad will likely autoconfigure the MAC Merge layer,>> > +while this test will configure it manually. One of them>> > +is arbitrarily going to overwrite the other. That will>> > +cause spurious failures (or, unlikely, passes) of this>> > +test.>> > +EOF>> > +exit 1>> > +fi>> > +}>> >> This would be good to have unified. Can you make the function reusable,>> with a generic or parametrized message? I should be able to carve a bit>> of time later to move it to lib.sh, migrate the mlxsw selftests, and>> drop the qos_lib.sh copy.>> Maybe like this?Yes, for most of them, but the issue is that...> diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh> index c6ce0b448bf3..bf57400e14ee 100755> --- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh> +++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh> @@ -2,7 +2,7 @@> # SPDX-License-Identifier: GPL-2.0> > source qos_lib.sh> -bail_on_lldpad> +bail_on_lldpad "configure DCB" "configure Qdiscs"... lib.sh isn't sourced at this point yet. `source$lib_dir/sch_tbf_ets.sh' brings that in later in the file, so the bailwould need to be below that. But if it is, it won't run until after thetest, which is useless.Maybe all it takes is to replace that `source qos_lib.sh' with`NUM_NETIFS=0 source $lib_dir/lib.sh', as we do in in_ns(), but I'llneed to check.That's why I proposed to do it myself, some of it is fiddly and having aswitch handy is going to be... um, handy.

Hi Petr,On Tue, Feb 14, 2023 at 12:53:06PM +0100, Petr Machata wrote:> > Vladimir Oltean <vladimir.oltean@nxp.com> writes:> > > Hi Petr,> >> > On Mon, Feb 13, 2023 at 11:51:37AM +0100, Petr Machata wrote:> >> Vladimir Oltean <vladimir.oltean@nxp.com> writes:> >> > >> > +# Borrowed from mlxsw/qos_lib.sh, message adapted.> >> > +bail_on_lldpad()> >> > +{> >> > +if systemctl is-active --quiet lldpad; then> >> > +cat >/dev/stderr <<-EOF> >> > +WARNING: lldpad is running> >> > +> >> > +lldpad will likely autoconfigure the MAC Merge layer,> >> > +while this test will configure it manually. One of them> >> > +is arbitrarily going to overwrite the other. That will> >> > +cause spurious failures (or, unlikely, passes) of this> >> > +test.> >> > +EOF> >> > +exit 1> >> > +fi> >> > +}> >> > >> This would be good to have unified. Can you make the function reusable,> >> with a generic or parametrized message? I should be able to carve a bit> >> of time later to move it to lib.sh, migrate the mlxsw selftests, and> >> drop the qos_lib.sh copy.> >> > Maybe like this?> > Yes, for most of them, but the issue is that...> > > diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh> > index c6ce0b448bf3..bf57400e14ee 100755> > --- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh> > +++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh> > @@ -2,7 +2,7 @@> > # SPDX-License-Identifier: GPL-2.0> > > > source qos_lib.sh> > -bail_on_lldpad> > +bail_on_lldpad "configure DCB" "configure Qdiscs"> > ... lib.sh isn't sourced at this point yet. `source> $lib_dir/sch_tbf_ets.sh' brings that in later in the file, so the bail> would need to be below that. But if it is, it won't run until after the> test, which is useless.> > Maybe all it takes is to replace that `source qos_lib.sh' with> `NUM_NETIFS=0 source $lib_dir/lib.sh', as we do in in_ns(), but I'll> need to check.> > That's why I proposed to do it myself, some of it is fiddly and having a> switch handy is going to be... um, handy.Are you planning to take a look and see whether the callers of bail_on_lldpad()can also source $lib_dir/lib.sh? Should I resend a blind change here?

Petr Machata <petrm@nvidia.com> writes:> Vladimir Oltean <vladimir.oltean@nxp.com> writes:>>> Hi Petr,>>>> On Mon, Feb 13, 2023 at 11:51:37AM +0100, Petr Machata wrote:>>> Vladimir Oltean <vladimir.oltean@nxp.com> writes:>>> >>> > +# Borrowed from mlxsw/qos_lib.sh, message adapted.>>> > +bail_on_lldpad()>>> > +{>>> > +if systemctl is-active --quiet lldpad; then>>> > +cat >/dev/stderr <<-EOF>>> > +WARNING: lldpad is running>>> > +>>> > +lldpad will likely autoconfigure the MAC Merge layer,>>> > +while this test will configure it manually. One of them>>> > +is arbitrarily going to overwrite the other. That will>>> > +cause spurious failures (or, unlikely, passes) of this>>> > +test.>>> > +EOF>>> > +exit 1>>> > +fi>>> > +}>>> >>> This would be good to have unified. Can you make the function reusable,>>> with a generic or parametrized message? I should be able to carve a bit>>> of time later to move it to lib.sh, migrate the mlxsw selftests, and>>> drop the qos_lib.sh copy.>>>> Maybe like this?>> Yes, for most of them, but the issue is that...>>> diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh>> index c6ce0b448bf3..bf57400e14ee 100755>> --- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh>> +++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh>> @@ -2,7 +2,7 @@>> # SPDX-License-Identifier: GPL-2.0>> >> source qos_lib.sh>> -bail_on_lldpad>> +bail_on_lldpad "configure DCB" "configure Qdiscs">> ... lib.sh isn't sourced at this point yet. `source> $lib_dir/sch_tbf_ets.sh' brings that in later in the file, so the bail> would need to be below that. But if it is, it won't run until after the> test, which is useless.>> Maybe all it takes is to replace that `source qos_lib.sh' with> `NUM_NETIFS=0 source $lib_dir/lib.sh', as we do in in_ns(), but I'll> need to check.>> That's why I proposed to do it myself, some of it is fiddly and having a> switch handy is going to be... um, handy.Sorry, this completely slipped my ming. I'm looking into it.

Petr Machata <petrm@nvidia.com> writes:> Petr Machata <petrm@nvidia.com> writes:>>> Vladimir Oltean <vladimir.oltean@nxp.com> writes:>>>>> diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh>>> index c6ce0b448bf3..bf57400e14ee 100755>>> --- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh>>> +++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh>>> @@ -2,7 +2,7 @@>>> # SPDX-License-Identifier: GPL-2.0>>> >>> source qos_lib.sh>>> -bail_on_lldpad>>> +bail_on_lldpad "configure DCB" "configure Qdiscs">>>> ... lib.sh isn't sourced at this point yet. `source>> $lib_dir/sch_tbf_ets.sh' brings that in later in the file, so the bail>> would need to be below that. But if it is, it won't run until after the>> test, which is useless.I added a shim as shown below. Comments welcome. Your patch then needs abit of adaptation, plus I've dropped all the now-useless imports ofqos_lib.sh. I'll pass this through our regression, and if nothingexplodes, I'll point you at a branch tomorrow, and you can make the twopatches a part of your larger patchset?Subject: [PATCH net-next mlxsw] selftests: forwarding: sch_tbs_*: Add a pre-run hookThe driver-specific wrappers of these selftests invoke bail_on_lldpad tomake sure that LLDPAD doesn't trample the configuration. The functionbail_on_lldpad is going to move to lib.sh in the next patch. With that, itwon't be visible for the wrappers before sourcing the framework script. Andafter sourcing it, it is too late: the selftest will have run by then.One option might be to source NUM_NETIFS=0 lib.sh from the wrapper, buteven if that worked (it might, it might not), that seems cumbersome. lib.shis doing fair amount of stuff, and even if it works today, it does not lookparticularly solid as a solution.Instead, introduce a hook, sch_tbf_pre_hook(), that when available, getsinvoked. Move the bail to the hook.Signed-off-by: Petr Machata <petrm@nvidia.com>--- tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh | 6 +++++- tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh | 6 +++++- tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh | 6 +++++- tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh | 4 ++++ tools/testing/selftests/net/forwarding/sch_tbf_root.sh | 4 ++++ 5 files changed, 23 insertions(+), 3 deletions(-)diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.shindex c6ce0b448bf3..b9b4cdf14ceb 100755--- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh@@ -2,7 +2,11 @@ # SPDX-License-Identifier: GPL-2.0 source qos_lib.sh-bail_on_lldpad++sch_tbf_pre_hook()+{+bail_on_lldpad+} lib_dir=$(dirname $0)/../../../net/forwarding TCFLAGS=skip_swdiff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.shindex 8d245f331619..dff9810ee04f 100755--- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh@@ -2,7 +2,11 @@ # SPDX-License-Identifier: GPL-2.0 source qos_lib.sh-bail_on_lldpad++sch_tbf_pre_hook()+{+bail_on_lldpad+} lib_dir=$(dirname $0)/../../../net/forwarding TCFLAGS=skip_swdiff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.shindex 013886061f15..75406bd7036e 100755--- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh@@ -2,7 +2,11 @@ # SPDX-License-Identifier: GPL-2.0 source qos_lib.sh-bail_on_lldpad++sch_tbf_pre_hook()+{+bail_on_lldpad+} lib_dir=$(dirname $0)/../../../net/forwarding TCFLAGS=skip_swdiff --git a/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh b/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.shindex 75a37c189ef3..df9bcd6a811a 100644--- a/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh+++ b/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh@@ -57,6 +57,10 @@ tbf_root_test() tc qdisc del dev $swp2 root } +if type -t sch_tbf_pre_hook >/dev/null; then+sch_tbf_pre_hook+fi+ trap cleanup EXIT setup_preparediff --git a/tools/testing/selftests/net/forwarding/sch_tbf_root.sh b/tools/testing/selftests/net/forwarding/sch_tbf_root.shindex 72aa21ba88c7..96c997be0d03 100755--- a/tools/testing/selftests/net/forwarding/sch_tbf_root.sh+++ b/tools/testing/selftests/net/forwarding/sch_tbf_root.sh@@ -23,6 +23,10 @@ tbf_test() tc qdisc del dev $swp2 root } +if type -t sch_tbf_pre_hook >/dev/null; then+sch_tbf_pre_hook+fi+ trap cleanup EXIT setup_prepare

Petr Machata <petrm@nvidia.com> writes:> Petr Machata <petrm@nvidia.com> writes:>>> Petr Machata <petrm@nvidia.com> writes:>>>>> Vladimir Oltean <vladimir.oltean@nxp.com> writes:>>>>>>> diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh>>>> index c6ce0b448bf3..bf57400e14ee 100755>>>> --- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh>>>> +++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh>>>> @@ -2,7 +2,7 @@>>>> # SPDX-License-Identifier: GPL-2.0>>>> >>>> source qos_lib.sh>>>> -bail_on_lldpad>>>> +bail_on_lldpad "configure DCB" "configure Qdiscs">>>>>> ... lib.sh isn't sourced at this point yet. `source>>> $lib_dir/sch_tbf_ets.sh' brings that in later in the file, so the bail>>> would need to be below that. But if it is, it won't run until after the>>> test, which is useless.>> I added a shim as shown below. Comments welcome. Your patch then needs a> bit of adaptation, plus I've dropped all the now-useless imports of> qos_lib.sh. I'll pass this through our regression, and if nothing> explodes, I'll point you at a branch tomorrow, and you can make the two> patches a part of your larger patchset?(I only pushed this to our regression today. The patches are the top twoones here: https://github.com/pmachata/linux_mlxsw/commits/petrm_selftests_bail_on_lldpad_moveI'll let you know on Monday whether anything exploded in regression.)

On Fri, Mar 17, 2023 at 04:35:58PM +0100, Petr Machata wrote:> > Petr Machata <petrm@nvidia.com> writes:> > > Petr Machata <petrm@nvidia.com> writes:> >> >> Petr Machata <petrm@nvidia.com> writes:> >>> >>> Vladimir Oltean <vladimir.oltean@nxp.com> writes:> >>>> >>>> diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh> >>>> index c6ce0b448bf3..bf57400e14ee 100755> >>>> --- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh> >>>> +++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh> >>>> @@ -2,7 +2,7 @@> >>>> # SPDX-License-Identifier: GPL-2.0> >>>> > >>>> source qos_lib.sh> >>>> -bail_on_lldpad> >>>> +bail_on_lldpad "configure DCB" "configure Qdiscs"> >>>> >>> ... lib.sh isn't sourced at this point yet. `source> >>> $lib_dir/sch_tbf_ets.sh' brings that in later in the file, so the bail> >>> would need to be below that. But if it is, it won't run until after the> >>> test, which is useless.> >> > I added a shim as shown below. Comments welcome. Your patch then needs a> > bit of adaptation, plus I've dropped all the now-useless imports of> > qos_lib.sh. I'll pass this through our regression, and if nothing> > explodes, I'll point you at a branch tomorrow, and you can make the two> > patches a part of your larger patchset?> > (I only pushed this to our regression today. The patches are the top two> ones here:> > https://github.com/pmachata/linux_mlxsw/commits/petrm_selftests_bail_on_lldpad_move> > I'll let you know on Monday whether anything exploded in regression.)Thanks.Side question. Your bail_on_lldpad logic wants to avoid lldpad frombeing involved at all in the selftest. In my case, I want to make surethat the service is disabled system-wide, so that the selftest can startit by itself. Is that the best way to make use of lldpad in general inthe kselftest framework?

Vladimir Oltean <vladimir.oltean@nxp.com> writes:> On Fri, Mar 17, 2023 at 04:35:58PM +0100, Petr Machata wrote:>> >> Petr Machata <petrm@nvidia.com> writes:>> >> > Petr Machata <petrm@nvidia.com> writes:>> >>> >> Petr Machata <petrm@nvidia.com> writes:>> >>>> >>> Vladimir Oltean <vladimir.oltean@nxp.com> writes:>> >>>>> >>>> diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh>> >>>> index c6ce0b448bf3..bf57400e14ee 100755>> >>>> --- a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh>> >>>> +++ b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_ets.sh>> >>>> @@ -2,7 +2,7 @@>> >>>> # SPDX-License-Identifier: GPL-2.0>> >>>> >> >>>> source qos_lib.sh>> >>>> -bail_on_lldpad>> >>>> +bail_on_lldpad "configure DCB" "configure Qdiscs">> >>>>> >>> ... lib.sh isn't sourced at this point yet. `source>> >>> $lib_dir/sch_tbf_ets.sh' brings that in later in the file, so the bail>> >>> would need to be below that. But if it is, it won't run until after the>> >>> test, which is useless.>> >>> > I added a shim as shown below. Comments welcome. Your patch then needs a>> > bit of adaptation, plus I've dropped all the now-useless imports of>> > qos_lib.sh. I'll pass this through our regression, and if nothing>> > explodes, I'll point you at a branch tomorrow, and you can make the two>> > patches a part of your larger patchset?>> >> (I only pushed this to our regression today. The patches are the top two>> ones here:>> >> https://github.com/pmachata/linux_mlxsw/commits/petrm_selftests_bail_on_lldpad_move>> >> I'll let you know on Monday whether anything exploded in regression.)>> Thanks.>> Side question. Your bail_on_lldpad logic wants to avoid lldpad from> being involved at all in the selftest. In my case, I want to make sure> that the service is disabled system-wide, so that the selftest can start> it by itself. Is that the best way to make use of lldpad in general in> the kselftest framework?^o^ I don't see why not. You could bail_on_lldpad, then proceed to startit, then stop it after the test again. Maybe the error message needstweaking to work well with this use-case though.

Petr Machata <petrm@nvidia.com> writes:> Petr Machata <petrm@nvidia.com> writes:>>> I added a shim as shown below. Comments welcome. Your patch then needs a>> bit of adaptation, plus I've dropped all the now-useless imports of>> qos_lib.sh. I'll pass this through our regression, and if nothing>> explodes, I'll point you at a branch tomorrow, and you can make the two>> patches a part of your larger patchset?>> (I only pushed this to our regression today. The patches are the top two> ones here:>> https://github.com/pmachata/linux_mlxsw/commits/petrm_selftests_bail_on_lldpad_move>> I'll let you know on Monday whether anything exploded in regression.)Nothing _relevant_ seems to have exploded :)I've pushed a new version of the patches with Danielle's (CC'd) R-b tagsand a fix of a typo that she noticed. Feel free to take these andintegrate / adjust / etc.

Patch

diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefileindex 91201ab3c4fc..7d95990e18ce 100644--- a/tools/testing/selftests/net/forwarding/Makefile+++ b/tools/testing/selftests/net/forwarding/Makefile@@ -15,6 +15,7 @@  TEST_PROGS = bridge_igmp.sh \ custom_multipath_hash.sh \ dual_vxlan_bridge.sh \ ethtool_extended_state.sh \+ethtool_mm.sh \ ethtool.sh \ gre_custom_multipath_hash.sh \ gre_inner_v4_multipath.sh \diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.shnew file mode 100755index 000000000000..b66db9419a27--- /dev/null+++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh@@ -0,0 +1,174 @@ +#!/bin/bash+# SPDX-License-Identifier: GPL-2.0++ALL_TESTS="+manual_with_verification+manual_without_verification+lldp+"++NUM_NETIFS=2+REQUIRE_MZ=no+source lib.sh++# Borrowed from mlxsw/qos_lib.sh, message adapted.+bail_on_lldpad()+{+if systemctl is-active --quiet lldpad; then+cat >/dev/stderr <<-EOF+WARNING: lldpad is running++lldpad will likely autoconfigure the MAC Merge layer,+while this test will configure it manually. One of them+is arbitrarily going to overwrite the other. That will+cause spurious failures (or, unlikely, passes) of this+test.+EOF+exit 1+fi+}++manual_with_verification()+{+check_ethtool_mm_support+bail_on_lldpad++ethtool --set-mm $h1 verify-enabled on tx-enabled on++# Wait for verification to finish+sleep 1++ethtool --json --show-mm $h1 | jq -r '.[]."verify-status"' | \+grep -q 'SUCCEEDED'+check_err "$?" "Verification did not succeed"++ethtool --json --show-mm $h1 | jq -r '.[]."tx-active"' | grep -q 'true'+check_err "$?" "pMAC TX is not active"++log_test "Manual configuration with verification"+}++manual_without_verification()+{+check_ethtool_mm_support+bail_on_lldpad++ethtool --set-mm $h2 verify-enabled off tx-enabled on++ethtool --json --show-mm $h2 | jq -r '.[]."verify-status"' | \+grep -q 'DISABLED'+check_err "$?" "Verification is not disabled"++ethtool --json --show-mm $h2 | jq -r '.[]."tx-active"' | grep -q 'true'+check_err "$?" "pMAC TX is not active"++log_test "Manual configuration without verification"+}++lldp_change_add_frag_size()+{+local add_frag_size=$1++lldptool -T -i $h1 -V addEthCaps addFragSize=$add_frag_size >/dev/null+# Wait for TLVs to be received+sleep 1+lldptool -i $h2 -t -n -V addEthCaps | \+grep -q "Additional fragment size: $add_frag_size"+}++lldp()+{+require_command lldptool+bail_on_lldpad++systemctl start lldpad++# Configure the interfaces to receive and transmit LLDPDUs+lldptool -L -i $h1 adminStatus=rxtx >/dev/null+lldptool -L -i $h2 adminStatus=rxtx >/dev/null++# Enable the transmission of Additional Ethernet Capabilities TLV+lldptool -T -i $h1 -V addEthCaps enableTx=yes >/dev/null+lldptool -T -i $h2 -V addEthCaps enableTx=yes >/dev/null++# Wait for TLVs to be received+sleep 1++lldptool -i $h1 -t -n -V addEthCaps | \+grep -q "Preemption capability active"+check_err "$?" "$h1 pMAC TX is not active"++lldptool -i $h2 -t -n -V addEthCaps | \+grep -q "Preemption capability active"+check_err "$?" "$h2 pMAC TX is not active"++lldp_change_add_frag_size 0+check_err "$?" "addFragSize 0"++lldp_change_add_frag_size 1+check_err "$?" "addFragSize 1"++lldp_change_add_frag_size 2+check_err "$?" "addFragSize 2"++lldp_change_add_frag_size 3+check_err "$?" "addFragSize 3"++systemctl stop lldpad++log_test "LLDP"+}++h1_create()+{+ip link set dev $h1 up++ethtool --set-mm $h1 pmac-enabled on tx-enabled off verify-enabled off+}++h2_create()+{+ip link set dev $h2 up++ethtool --set-mm $h2 pmac-enabled on tx-enabled off verify-enabled off+}++h1_destroy()+{+ethtool --set-mm $h1 pmac-enabled off tx-enabled off verify-enabled off++ip link set dev $h1 down+}++h2_destroy()+{+ethtool --set-mm $h2 pmac-enabled off tx-enabled off verify-enabled off++ip link set dev $h2 down+}++setup_prepare()+{+h1=${NETIFS[p1]}+h2=${NETIFS[p2]}++h1_create+h2_create+}++cleanup()+{+pre_cleanup++h2_destroy+h1_destroy+}++trap cleanup EXIT++setup_prepare+setup_wait++tests_run++exit $EXIT_STATUSdiff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.shindex d47499ba81c7..30311214f39f 100755--- a/tools/testing/selftests/net/forwarding/lib.sh+++ b/tools/testing/selftests/net/forwarding/lib.sh@@ -129,6 +129,15 @@  check_ethtool_lanes_support() fi } +check_ethtool_mm_support()+{+ethtool --help 2>&1| grep -- '--show-mm' &> /dev/null+if [[ $? -ne 0 ]]; then+echo "SKIP: ethtool too old; it is missing MAC Merge layer support"+exit $ksft_skip+fi+}+ check_locked_port_support() { if ! bridge -d link show | grep -q " locked"; then