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 <[email protected]>
---
This is more for the sake of the discussion, because neither the ethtool
nor 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 configuration
file.
.../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
diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
index 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.sh
new file mode 100755
index 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_STATUS
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 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
--
2.34.1
Vladimir Oltean <[email protected]> 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.
Hi Petr,
On Mon, Feb 13, 2023 at 11:51:37AM +0100, Petr Machata wrote:
> Vladimir Oltean <[email protected]> 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.sh
index 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_wait
diff --git a/tools/testing/selftests/drivers/net/mlxsw/qos_lib.sh b/tools/testing/selftests/drivers/net/mlxsw/qos_lib.sh
index 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.sh
index 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_prepare
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_ets.sh
index 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_run
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh
index 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_prepare
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh
index 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_prepare
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_dir=$(dirname $0)/../../../net/forwarding
TCFLAGS=skip_sw
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh
index 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_sw
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh
index 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_sw
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 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 <[email protected]>
> ---
Looks good to me. Is there any way to test this? I see mm support
implemented for enetc and Felix. However, I only have access to Intel
i225 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-next
and adapted to the merged version of the ethtool API:
https://patchwork.kernel.org/project/netdevbpf/cover/[email protected]/
Vladimir Oltean <[email protected]> writes:
> Hi Petr,
>
> On Mon, Feb 13, 2023 at 11:51:37AM +0100, Petr Machata wrote:
>> Vladimir Oltean <[email protected]> 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.
Hi Petr,
On Tue, Feb 14, 2023 at 12:53:06PM +0100, Petr Machata wrote:
>
> Vladimir Oltean <[email protected]> writes:
>
> > Hi Petr,
> >
> > On Mon, Feb 13, 2023 at 11:51:37AM +0100, Petr Machata wrote:
> >> Vladimir Oltean <[email protected]> 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 <[email protected]> writes:
> Vladimir Oltean <[email protected]> writes:
>
>> Hi Petr,
>>
>> On Mon, Feb 13, 2023 at 11:51:37AM +0100, Petr Machata wrote:
>>> Vladimir Oltean <[email protected]> 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 <[email protected]> writes:
> Petr Machata <[email protected]> writes:
>
>> Vladimir Oltean <[email protected]> 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?
Subject: [PATCH net-next mlxsw] selftests: forwarding: sch_tbs_*: Add a
pre-run hook
The driver-specific wrappers of these selftests invoke bail_on_lldpad to
make sure that LLDPAD doesn't trample the configuration. The function
bail_on_lldpad is going to move to lib.sh in the next patch. With that, it
won't be visible for the wrappers before sourcing the framework script. And
after 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, but
even if that worked (it might, it might not), that seems cumbersome. lib.sh
is doing fair amount of stuff, and even if it works today, it does not look
particularly solid as a solution.
Instead, introduce a hook, sch_tbf_pre_hook(), that when available, gets
invoked. Move the bail to the hook.
Signed-off-by: Petr Machata <[email protected]>
---
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.sh
index 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_sw
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_prio.sh
index 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_sw
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_tbf_root.sh
index 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_sw
diff --git a/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh b/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh
index 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_prepare
diff --git a/tools/testing/selftests/net/forwarding/sch_tbf_root.sh b/tools/testing/selftests/net/forwarding/sch_tbf_root.sh
index 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
--
2.39.0