All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] selftests: forwarding: add Per-Stream Filtering and Policing test for Ocelot
@ 2022-04-28 20:48 Vladimir Oltean
  2022-04-29  6:32 ` Kurt Kanzenbach
  0 siblings, 1 reply; 8+ messages in thread
From: Vladimir Oltean @ 2022-04-28 20:48 UTC (permalink / raw)
  To: netdev
  Cc: Jakub Kicinski, David S. Miller, Paolo Abeni, Eric Dumazet,
	Florian Fainelli, Vivien Didelot, Andrew Lunn, Claudiu Manoil,
	Alexandre Belloni, UNGLinuxDriver, Vinicius Costa Gomes,
	Gerhard Engleder, Y . b . Lu, Xiaoliang Yang, Richard Cochran,
	Sebastian Andrzej Siewior, Kurt Kanzenbach, Yannick Vignon,
	Rui Sousa, Jiri Pirko, Ido Schimmel

The Felix VSC9959 switch in NXP LS1028A supports the tc-gate action
which enforced time-based access control per stream. A stream as seen by
this switch is identified by {MAC DA, VID}.

We use the standard forwarding selftest topology with 2 host interfaces
and 2 switch interfaces. The host ports must require timestamping non-IP
packets and supporting tc-etf offload, for isochron to work. The
isochron program monitors network sync status (ptp4l, phc2sys) and
deterministically transmits packets to the switch such that the tc-gate
action either (a) always accepts them based on its schedule, or
(b) always drops them.

I tried to keep as much of the logic that isn't specific to the NXP
LS1028A in a new tsn_lib.sh, for future reuse. This covers
synchronization using ptp4l and phc2sys, and isochron.

The cycle-time chosen for this selftest isn't particularly impressive
(and the focus is the functionality of the switch), but I didn't really
know what to do better, considering that it will mostly be run during
debugging sessions, various kernel bloatware would be enabled, like
lockdep, KASAN, etc, and we certainly can't run any races with those on.

I tried to look through the kselftest framework for other real time
applications and didn't really find any, so I'm not sure how better to
prepare the environment in case we want to go for a lower cycle time.
At the moment, the only thing the selftest is ensuring is that dynamic
frequency scaling is disabled on the CPU that isochron runs on. It would
probably be useful to have a blacklist of kernel config options (checked
through zcat /proc/config.gz) and some cyclictest scripts to run
beforehand, but I saw none of those.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 .../selftests/drivers/net/ocelot/psfp.sh      | 282 ++++++++++++++++++
 .../selftests/net/forwarding/tsn_lib.sh       | 219 ++++++++++++++
 2 files changed, 501 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/ocelot/psfp.sh
 create mode 100644 tools/testing/selftests/net/forwarding/tsn_lib.sh

diff --git a/tools/testing/selftests/drivers/net/ocelot/psfp.sh b/tools/testing/selftests/drivers/net/ocelot/psfp.sh
new file mode 100755
index 000000000000..27800a4552d7
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/ocelot/psfp.sh
@@ -0,0 +1,282 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright 2021-2022 NXP
+
+# Note: On LS1028A, in lack of enough user ports, this setup requires patching
+# the device tree to use the second CPU port as a user port
+
+WAIT_TIME=1
+NUM_NETIFS=4
+STABLE_MAC_ADDRS=yes
+NETIF_CREATE=no
+lib_dir=$(dirname $0)/../../../net/forwarding
+source $lib_dir/tc_common.sh
+source $lib_dir/lib.sh
+source $lib_dir/tsn_lib.sh
+
+UDS_ADDRESS_H1="/var/run/ptp4l_h1"
+UDS_ADDRESS_SWP1="/var/run/ptp4l_swp1"
+
+# Tunables
+NUM_PKTS=1000
+STREAM_VID=100
+STREAM_PRIO=6
+# Use a conservative cycle of 10 ms to allow the test to still pass when the
+# kernel has some extra overhead like lockdep etc
+CYCLE_TIME_NS=10000000
+# Create two Gate Control List entries, one OPEN and one CLOSE, of equal
+# durations
+GATE_DURATION_NS=$((${CYCLE_TIME_NS} / 2))
+# Give 2/3 of the cycle time to user space and 1/3 to the kernel
+FUDGE_FACTOR=$((${CYCLE_TIME_NS} / 3))
+# Shift the isochron base time by half the gate time, so that packets are
+# always received by swp1 close to the middle of the time slot, to minimize
+# inaccuracies due to network sync
+SHIFT_TIME_NS=$((${GATE_DURATION_NS} / 2))
+
+h1=${NETIFS[p1]}
+swp1=${NETIFS[p2]}
+swp2=${NETIFS[p3]}
+h2=${NETIFS[p4]}
+
+H1_IPV4="192.0.2.1"
+H2_IPV4="192.0.2.2"
+H1_IPV6="2001:db8:1::1"
+H2_IPV6="2001:db8:1::2"
+
+# Chain number exported by the ocelot driver for
+# Per-Stream Filtering and Policing filters
+PSFP()
+{
+	echo 30000
+}
+
+psfp_chain_create()
+{
+	local if_name=$1
+
+	tc qdisc add dev $if_name clsact
+
+	tc filter add dev $if_name ingress chain 0 pref 49152 flower \
+		skip_sw action goto chain $(PSFP)
+}
+
+psfp_chain_destroy()
+{
+	local if_name=$1
+
+	tc qdisc del dev $if_name clsact
+}
+
+psfp_filter_check()
+{
+	local expected_matches=$1
+	local packets=""
+	local drops=""
+	local stats=""
+
+	stats=$(tc -j -s filter show dev ${swp1} ingress chain $(PSFP) pref 1)
+	packets=$(echo ${stats} | jq ".[1].options.actions[].stats.packets")
+	drops=$(echo ${stats} | jq ".[1].options.actions[].stats.drops")
+
+	if ! [ "${packets}" = "${expected_matches}" ]; then
+		echo "Expected filter to match on ${expected_matches} packets but matched on ${packets} instead"
+	fi
+
+	echo "Hardware filter reports ${drops} drops"
+}
+
+h1_create()
+{
+	simple_if_init $h1 $H1_IPV4/24 $H1_IPV6/64
+}
+
+h1_destroy()
+{
+	simple_if_fini $h1 $H1_IPV4/24 $H1_IPV6/64
+}
+
+h2_create()
+{
+	simple_if_init $h2 $H2_IPV4/24 $H2_IPV6/64
+}
+
+h2_destroy()
+{
+	simple_if_fini $h2 $H2_IPV4/24 $H2_IPV6/64
+}
+
+switch_create()
+{
+	local h2_mac_addr=$(mac_get $h2)
+
+	ip link set ${swp1} up
+	ip link set ${swp2} up
+
+	ip link add br0 type bridge vlan_filtering 1
+	ip link set ${swp1} master br0
+	ip link set ${swp2} master br0
+	ip link set br0 up
+
+	bridge vlan add dev ${swp2} vid ${STREAM_VID}
+	bridge vlan add dev ${swp1} vid ${STREAM_VID}
+	# PSFP on Ocelot requires the filter to also be added to the bridge
+	# FDB, and not be removed
+	bridge fdb add dev ${swp2} \
+		${h2_mac_addr} vlan ${STREAM_VID} static master
+
+	psfp_chain_create ${swp1}
+
+	tc filter add dev ${swp1} ingress chain $(PSFP) pref 1 \
+		protocol 802.1Q flower skip_sw \
+		dst_mac ${h2_mac_addr} vlan_id ${STREAM_VID} \
+		action gate base-time 0.000000000 \
+		sched-entry OPEN  ${GATE_DURATION_NS} -1 -1 \
+		sched-entry CLOSE ${GATE_DURATION_NS} -1 -1
+}
+
+switch_destroy()
+{
+	psfp_chain_destroy ${swp1}
+	ip link del br0
+}
+
+txtime_setup()
+{
+	local if_name=$1
+
+	tc qdisc add dev ${if_name} clsact
+	# Classify PTP on TC 7 and isochron on TC 6
+	tc filter add dev ${if_name} egress protocol 0x88f7 \
+		flower action skbedit priority 7
+	tc filter add dev ${if_name} egress protocol 802.1Q \
+		flower vlan_ethtype 0xdead action skbedit priority 6
+	tc qdisc add dev ${if_name} handle 100: parent root mqprio num_tc 8 \
+		queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 \
+		map 0 1 2 3 4 5 6 7 \
+		hw 1
+	# Set up TC 6 for SO_TXTIME
+	tc qdisc replace dev ${if_name} parent 100:6 etf \
+		clockid CLOCK_TAI offload delta ${FUDGE_FACTOR}
+}
+
+txtime_cleanup()
+{
+	local if_name=$1
+
+	tc qdisc del dev ${if_name} root
+	tc qdisc del dev ${if_name} clsact
+}
+
+setup_prepare()
+{
+	vrf_prepare
+
+	h1_create
+	h2_create
+	switch_create
+
+	txtime_setup ${h1}
+
+	# Set up swp1 as a master PHC for h1, synchronized to the local
+	# CLOCK_REALTIME.
+	phc2sys_start ${swp1} ${UDS_ADDRESS_SWP1}
+
+	# Assumption true for LS1028A: h1 and h2 use the same PHC. So by
+	# synchronizing h1 to swp1 via PTP, h2 is also implicitly synchronized
+	# to swp1 (and both to CLOCK_REALTIME).
+	ptp4l_start ${h1} true ${UDS_ADDRESS_H1}
+	ptp4l_start ${swp1} false ${UDS_ADDRESS_SWP1}
+
+	# Make sure there are no filter matches at the beginning of the test
+	psfp_filter_check 0
+}
+
+cleanup()
+{
+	pre_cleanup
+
+	ptp4l_stop ${swp1}
+	ptp4l_stop ${h1}
+	phc2sys_stop
+	isochron_recv_stop
+
+	txtime_cleanup ${h1}
+
+	h2_destroy
+	h1_destroy
+	switch_destroy
+
+	vrf_cleanup
+}
+
+run_test()
+{
+	local base_time=$1
+	local expected=$2
+	local test_name=$3
+	local isochron_dat="$(mktemp)"
+	local extra_args=""
+	local received
+
+	isochron_do \
+		"${h1}" \
+		"${h2}" \
+		"${UDS_ADDRESS_H1}" \
+		"" \
+		"${base_time}" \
+		"${CYCLE_TIME_NS}" \
+		"${SHIFT_TIME_NS}" \
+		"${NUM_PKTS}" \
+		"${STREAM_VID}" \
+		"${STREAM_PRIO}" \
+		"" \
+		"${isochron_dat}"
+
+	# Count all received seqid's
+	received=$(isochron report --quiet \
+		--input-file "${isochron_dat}" \
+		--printf-format "%u\n" --printf-args "q" | \
+		wc -l)
+
+	if [ "${received}" = "${expected}" ]; then
+		RET=0
+	else
+		RET=1
+		echo "Expected isochron to receive ${expected} packets but received ${received}"
+	fi
+
+	rm ${isochron_dat} 2> /dev/null
+
+	log_test "${test_name}"
+}
+
+test_gate_in_band()
+{
+	# Send packets in-band with the OPEN gate entry
+	run_test 0.000000000 ${NUM_PKTS} "In band"
+
+	psfp_filter_check ${NUM_PKTS}
+}
+
+test_gate_out_of_band()
+{
+	# Send packets in-band with the CLOSE gate entry
+	run_test 0.005000000 0 "Out of band"
+
+	psfp_filter_check $((2 * ${NUM_PKTS}))
+}
+
+trap cleanup EXIT
+
+ALL_TESTS="
+	test_gate_in_band
+	test_gate_out_of_band
+"
+
+setup_prepare
+setup_wait
+
+tests_run
+
+exit $EXIT_STATUS
diff --git a/tools/testing/selftests/net/forwarding/tsn_lib.sh b/tools/testing/selftests/net/forwarding/tsn_lib.sh
new file mode 100644
index 000000000000..efac5badd5a0
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/tsn_lib.sh
@@ -0,0 +1,219 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright 2021-2022 NXP
+
+# Tunables
+UTC_TAI_OFFSET=37
+ISOCHRON_CPU=1
+
+# https://github.com/vladimiroltean/tsn-scripts
+# WARNING: isochron versions pre-1.0 are unstable,
+# always use the latest version
+require_command isochron
+require_command phc2sys
+require_command ptp4l
+
+phc2sys_start()
+{
+	local if_name=$1
+	local uds_address=$2
+	local extra_args=""
+
+	if ! [ -z "${uds_address}" ]; then
+		extra_args="${extra_args} -z ${uds_address}"
+	fi
+
+	phc2sys_log="$(mktemp)"
+
+	chrt -f 10 phc2sys -m \
+		-c ${if_name} \
+		-s CLOCK_REALTIME \
+		-O ${UTC_TAI_OFFSET} \
+		--step_threshold 0.00002 \
+		--first_step_threshold 0.00002 \
+		${extra_args} \
+		> "${phc2sys_log}" 2>&1 &
+	phc2sys_pid=$!
+
+	echo "phc2sys logs to ${phc2sys_log} and has pid ${phc2sys_pid}"
+
+	sleep 1
+}
+
+phc2sys_stop()
+{
+	{ kill ${phc2sys_pid} && wait ${phc2sys_pid}; } 2> /dev/null
+	rm "${phc2sys_log}" 2> /dev/null
+}
+
+ptp4l_start()
+{
+	local if_name=$1
+	local slave_only=$2
+	local uds_address=$3
+	local log="ptp4l_log_${if_name}"
+	local pid="ptp4l_pid_${if_name}"
+	local extra_args=""
+
+	if [ "${slave_only}" = true ]; then
+		extra_args="${extra_args} -s"
+	fi
+
+	# declare dynamic variables ptp4l_log_${if_name} and ptp4l_pid_${if_name}
+	# as global, so that they can be referenced later
+	declare -g "${log}=$(mktemp)"
+
+	chrt -f 10 ptp4l -m -2 -P \
+		-i ${if_name} \
+		--step_threshold 0.00002 \
+		--first_step_threshold 0.00002 \
+		--tx_timestamp_timeout 100 \
+		--uds_address="${uds_address}" \
+		${extra_args} \
+		> "${!log}" 2>&1 &
+	declare -g "${pid}=$!"
+
+	echo "ptp4l for interface ${if_name} logs to ${!log} and has pid ${!pid}"
+
+	sleep 1
+}
+
+ptp4l_stop()
+{
+	local if_name=$1
+	local log="ptp4l_log_${if_name}"
+	local pid="ptp4l_pid_${if_name}"
+
+	{ kill ${!pid} && wait ${!pid}; } 2> /dev/null
+	rm "${!log}" 2> /dev/null
+}
+
+cpufreq_max()
+{
+	local cpu=$1
+	local freq="cpu${cpu}_freq"
+	local governor="cpu${cpu}_governor"
+
+	# declare dynamic variables cpu${cpu}_freq and cpu${cpu}_governor as
+	# global, so they can be referenced later
+	declare -g "${freq}=$(cat /sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_min_freq)"
+	declare -g "${governor}=$(cat /sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_governor)"
+
+	cat /sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_max_freq > \
+		/sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_min_freq
+	echo -n "performance" > \
+		/sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_governor
+}
+
+cpufreq_restore()
+{
+	local cpu=$1
+	local freq="cpu${cpu}_freq"
+	local governor="cpu${cpu}_governor"
+
+	echo "${!freq}" > /sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_min_freq
+	echo -n "${!governor}" > \
+		/sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_governor
+}
+
+isochron_recv_start()
+{
+	local if_name=$1
+	local uds=$2
+	local extra_args=$3
+
+	if ! [ -z "${uds}" ]; then
+		extra_args="--unix-domain-socket ${uds}"
+	fi
+
+	isochron rcv \
+		--interface ${if_name} \
+		--sched-priority 98 \
+		--sched-rr \
+		--utc-tai-offset ${UTC_TAI_OFFSET} \
+		--quiet \
+		${extra_args} & \
+	isochron_pid=$!
+
+	sleep 1
+}
+
+isochron_recv_stop()
+{
+	{ kill ${isochron_pid} && wait ${isochron_pid}; } 2> /dev/null
+}
+
+isochron_do()
+{
+	local sender_if_name=$1; shift
+	local receiver_if_name=$1; shift
+	local sender_uds=$1; shift
+	local receiver_uds=$1; shift
+	local base_time=$1; shift
+	local cycle_time=$1; shift
+	local shift_time=$1; shift
+	local num_pkts=$1; shift
+	local vid=$1; shift
+	local priority=$1; shift
+	local dst_ip=$1; shift
+	local isochron_dat=$1; shift
+	local extra_args=""
+	local receiver_extra_args=""
+	local vrf="$(master_name_get ${sender_if_name})"
+	local use_l2="true"
+
+	if ! [ -z "${dst_ip}" ]; then
+		use_l2="false"
+	fi
+
+	if ! [ -z "${vrf}" ]; then
+		dst_ip="${dst_ip}%${vrf}"
+	fi
+
+	if ! [ -z "${vid}" ]; then
+		vid="--vid=${vid}"
+	fi
+
+	if [ -z "${receiver_uds}" ]; then
+		extra_args="${extra_args} --omit-remote-sync"
+	fi
+
+	if ! [ -z "${shift_time}" ]; then
+		extra_args="${extra_args} --shift-time=${shift_time}"
+	fi
+
+	if [ "${use_l2}" = "true" ]; then
+		extra_args="${extra_args} --l2 --etype=0xdead ${vid}"
+		receiver_extra_args="--l2 --etype=0xdead"
+	else
+		extra_args="${extra_args} --l4 --ip-destination=${dst_ip}"
+		receiver_extra_args="--l4"
+	fi
+
+	cpufreq_max ${ISOCHRON_CPU}
+
+	isochron_recv_start "${h2}" "${receiver_uds}" "${receiver_extra_args}"
+
+	isochron send \
+		--interface ${sender_if_name} \
+		--unix-domain-socket ${sender_uds} \
+		--priority ${priority} \
+		--base-time ${base_time} \
+		--cycle-time ${cycle_time} \
+		--num-frames ${num_pkts} \
+		--frame-size 64 \
+		--txtime \
+		--utc-tai-offset ${UTC_TAI_OFFSET} \
+		--cpu-mask $((1 << ${ISOCHRON_CPU})) \
+		--sched-fifo \
+		--sched-priority 98 \
+		--client 127.0.0.1 \
+		--sync-threshold 5000 \
+		--output-file ${isochron_dat} \
+		${extra_args} \
+		--quiet
+
+	isochron_recv_stop
+
+	cpufreq_restore ${ISOCHRON_CPU}
+}
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] selftests: forwarding: add Per-Stream Filtering and Policing test for Ocelot
  2022-04-28 20:48 [PATCH net-next] selftests: forwarding: add Per-Stream Filtering and Policing test for Ocelot Vladimir Oltean
@ 2022-04-29  6:32 ` Kurt Kanzenbach
  2022-04-29  9:38   ` Vladimir Oltean
  0 siblings, 1 reply; 8+ messages in thread
From: Kurt Kanzenbach @ 2022-04-29  6:32 UTC (permalink / raw)
  To: Vladimir Oltean, netdev
  Cc: Jakub Kicinski, David S. Miller, Paolo Abeni, Eric Dumazet,
	Florian Fainelli, Vivien Didelot, Andrew Lunn, Claudiu Manoil,
	Alexandre Belloni, UNGLinuxDriver, Vinicius Costa Gomes,
	Gerhard Engleder, Y . b . Lu, Xiaoliang Yang, Richard Cochran,
	Sebastian Andrzej Siewior, Yannick Vignon, Rui Sousa, Jiri Pirko,
	Ido Schimmel

[-- Attachment #1: Type: text/plain, Size: 5852 bytes --]

Hi Vladimir,

On Thu Apr 28 2022, Vladimir Oltean wrote:
> The Felix VSC9959 switch in NXP LS1028A supports the tc-gate action
> which enforced time-based access control per stream. A stream as seen by
> this switch is identified by {MAC DA, VID}.
>
> We use the standard forwarding selftest topology with 2 host interfaces
> and 2 switch interfaces. The host ports must require timestamping non-IP
> packets and supporting tc-etf offload, for isochron to work. The
> isochron program monitors network sync status (ptp4l, phc2sys) and
> deterministically transmits packets to the switch such that the tc-gate
> action either (a) always accepts them based on its schedule, or
> (b) always drops them.
>
> I tried to keep as much of the logic that isn't specific to the NXP
> LS1028A in a new tsn_lib.sh, for future reuse. This covers
> synchronization using ptp4l and phc2sys, and isochron.

For running this selftest `isochron` tool is required. That's neither
packaged on Linux distributions or available in the kernel source. I
guess, it has to be built from your Github account/repository?

>
> The cycle-time chosen for this selftest isn't particularly impressive
> (and the focus is the functionality of the switch), but I didn't really
> know what to do better, considering that it will mostly be run during
> debugging sessions, various kernel bloatware would be enabled, like
> lockdep, KASAN, etc, and we certainly can't run any races with those on.
>
> I tried to look through the kselftest framework for other real time
> applications and didn't really find any, so I'm not sure how better to
> prepare the environment in case we want to go for a lower cycle time.
> At the moment, the only thing the selftest is ensuring is that dynamic
> frequency scaling is disabled on the CPU that isochron runs on. It would
> probably be useful to have a blacklist of kernel config options (checked
> through zcat /proc/config.gz) and some cyclictest scripts to run
> beforehand, but I saw none of those.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[snip]

> diff --git a/tools/testing/selftests/net/forwarding/tsn_lib.sh b/tools/testing/selftests/net/forwarding/tsn_lib.sh
> new file mode 100644
> index 000000000000..efac5badd5a0
> --- /dev/null
> +++ b/tools/testing/selftests/net/forwarding/tsn_lib.sh
> @@ -0,0 +1,219 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright 2021-2022 NXP
> +
> +# Tunables
> +UTC_TAI_OFFSET=37

Why do you need the UTC to TAI offset? isochron could just use CLOCK_TAI
as clockid for the task scheduling.

> +ISOCHRON_CPU=1

Seems reasonable to assume two cpus.

> +
> +# https://github.com/vladimiroltean/tsn-scripts
> +# WARNING: isochron versions pre-1.0 are unstable,
> +# always use the latest version
> +require_command isochron
> +require_command phc2sys
> +require_command ptp4l
> +
> +phc2sys_start()
> +{
> +	local if_name=$1
> +	local uds_address=$2
> +	local extra_args=""
> +
> +	if ! [ -z "${uds_address}" ]; then
> +		extra_args="${extra_args} -z ${uds_address}"
> +	fi
> +
> +	phc2sys_log="$(mktemp)"
> +
> +	chrt -f 10 phc2sys -m \
> +		-c ${if_name} \
> +		-s CLOCK_REALTIME \
> +		-O ${UTC_TAI_OFFSET} \
> +		--step_threshold 0.00002 \
> +		--first_step_threshold 0.00002 \
> +		${extra_args} \
> +		> "${phc2sys_log}" 2>&1 &
> +	phc2sys_pid=$!
> +
> +	echo "phc2sys logs to ${phc2sys_log} and has pid ${phc2sys_pid}"
> +
> +	sleep 1
> +}
> +
> +phc2sys_stop()
> +{
> +	{ kill ${phc2sys_pid} && wait ${phc2sys_pid}; } 2> /dev/null
> +	rm "${phc2sys_log}" 2> /dev/null
> +}
> +
> +ptp4l_start()
> +{
> +	local if_name=$1
> +	local slave_only=$2
> +	local uds_address=$3
> +	local log="ptp4l_log_${if_name}"
> +	local pid="ptp4l_pid_${if_name}"
> +	local extra_args=""
> +
> +	if [ "${slave_only}" = true ]; then
> +		extra_args="${extra_args} -s"
> +	fi
> +
> +	# declare dynamic variables ptp4l_log_${if_name} and ptp4l_pid_${if_name}
> +	# as global, so that they can be referenced later
> +	declare -g "${log}=$(mktemp)"
> +
> +	chrt -f 10 ptp4l -m -2 -P \
> +		-i ${if_name} \
> +		--step_threshold 0.00002 \
> +		--first_step_threshold 0.00002 \
> +		--tx_timestamp_timeout 100 \
> +		--uds_address="${uds_address}" \
> +		${extra_args} \
> +		> "${!log}" 2>&1 &
> +	declare -g "${pid}=$!"
> +
> +	echo "ptp4l for interface ${if_name} logs to ${!log} and has pid ${!pid}"
> +
> +	sleep 1
> +}
> +
> +ptp4l_stop()
> +{
> +	local if_name=$1
> +	local log="ptp4l_log_${if_name}"
> +	local pid="ptp4l_pid_${if_name}"
> +
> +	{ kill ${!pid} && wait ${!pid}; } 2> /dev/null
> +	rm "${!log}" 2> /dev/null
> +}
> +
> +cpufreq_max()
> +{
> +	local cpu=$1
> +	local freq="cpu${cpu}_freq"
> +	local governor="cpu${cpu}_governor"
> +
> +	# declare dynamic variables cpu${cpu}_freq and cpu${cpu}_governor as
> +	# global, so they can be referenced later
> +	declare -g "${freq}=$(cat /sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_min_freq)"
> +	declare -g "${governor}=$(cat /sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_governor)"
> +
> +	cat /sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_max_freq > \
> +		/sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_min_freq
> +	echo -n "performance" > \
> +		/sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_governor
> +}
> +
> +cpufreq_restore()
> +{
> +	local cpu=$1
> +	local freq="cpu${cpu}_freq"
> +	local governor="cpu${cpu}_governor"
> +
> +	echo "${!freq}" > /sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_min_freq
> +	echo -n "${!governor}" > \
> +		/sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_governor
> +}
> +
> +isochron_recv_start()
> +{
> +	local if_name=$1
> +	local uds=$2
> +	local extra_args=$3
> +
> +	if ! [ -z "${uds}" ]; then
> +		extra_args="--unix-domain-socket ${uds}"
> +	fi
> +
> +	isochron rcv \
> +		--interface ${if_name} \
> +		--sched-priority 98 \
> +		--sched-rr \

Why SCHED_RR?

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] selftests: forwarding: add Per-Stream Filtering and Policing test for Ocelot
  2022-04-29  6:32 ` Kurt Kanzenbach
@ 2022-04-29  9:38   ` Vladimir Oltean
  2022-04-29 10:15     ` Kurt Kanzenbach
  0 siblings, 1 reply; 8+ messages in thread
From: Vladimir Oltean @ 2022-04-29  9:38 UTC (permalink / raw)
  To: Kurt Kanzenbach
  Cc: netdev, Jakub Kicinski, David S. Miller, Paolo Abeni,
	Eric Dumazet, Florian Fainelli, Vivien Didelot, Andrew Lunn,
	Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Vinicius Costa Gomes, Gerhard Engleder, Y.B. Lu, Xiaoliang Yang,
	Richard Cochran, Sebastian Andrzej Siewior, Yannick Vignon,
	Rui Sousa, Jiri Pirko, Ido Schimmel

Hi Kurt,

Thanks for reviewing.

On Fri, Apr 29, 2022 at 08:32:22AM +0200, Kurt Kanzenbach wrote:
> Hi Vladimir,
> 
> On Thu Apr 28 2022, Vladimir Oltean wrote:
> > The Felix VSC9959 switch in NXP LS1028A supports the tc-gate action
> > which enforced time-based access control per stream. A stream as seen by
> > this switch is identified by {MAC DA, VID}.
> >
> > We use the standard forwarding selftest topology with 2 host interfaces
> > and 2 switch interfaces. The host ports must require timestamping non-IP
> > packets and supporting tc-etf offload, for isochron to work. The
> > isochron program monitors network sync status (ptp4l, phc2sys) and
> > deterministically transmits packets to the switch such that the tc-gate
> > action either (a) always accepts them based on its schedule, or
> > (b) always drops them.
> >
> > I tried to keep as much of the logic that isn't specific to the NXP
> > LS1028A in a new tsn_lib.sh, for future reuse. This covers
> > synchronization using ptp4l and phc2sys, and isochron.
> 
> For running this selftest `isochron` tool is required. That's neither
> packaged on Linux distributions or available in the kernel source. I
> guess, it has to be built from your Github account/repository?

This is slightly inconvenient, yes. But for this selftest in particular,
a more specialized setup is required anyway, as it only runs on an NXP
LS1028A based board. So I guess it's only the smaller of several
inconveniences?

A few years ago when I decided to work on isochron, I searched for an
application for detailed network latency testing and I couldn't find
one. I don't think the situation has improved a lot since then. If
isochron is useful for a larger audience, I can look into what I can do
about distribution. It's license-compatible with the kernel, but it's a
large-ish program to just toss into tools/testing/selftests/, plus I
still commit rather frequently to it, and I'd probably annoy the crap
out of everyone if I move its development to netdev@vger.kernel.org.

> >
> > The cycle-time chosen for this selftest isn't particularly impressive
> > (and the focus is the functionality of the switch), but I didn't really
> > know what to do better, considering that it will mostly be run during
> > debugging sessions, various kernel bloatware would be enabled, like
> > lockdep, KASAN, etc, and we certainly can't run any races with those on.
> >
> > I tried to look through the kselftest framework for other real time
> > applications and didn't really find any, so I'm not sure how better to
> > prepare the environment in case we want to go for a lower cycle time.
> > At the moment, the only thing the selftest is ensuring is that dynamic
> > frequency scaling is disabled on the CPU that isochron runs on. It would
> > probably be useful to have a blacklist of kernel config options (checked
> > through zcat /proc/config.gz) and some cyclictest scripts to run
> > beforehand, but I saw none of those.
> >
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> [snip]
> 
> > diff --git a/tools/testing/selftests/net/forwarding/tsn_lib.sh b/tools/testing/selftests/net/forwarding/tsn_lib.sh
> > new file mode 100644
> > index 000000000000..efac5badd5a0
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/forwarding/tsn_lib.sh
> > @@ -0,0 +1,219 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright 2021-2022 NXP
> > +
> > +# Tunables
> > +UTC_TAI_OFFSET=37
> 
> Why do you need the UTC to TAI offset? isochron could just use CLOCK_TAI
> as clockid for the task scheduling.

isochron indeed works in CLOCK_TAI (this is done so that all timestamps
are chronologically ordered when everything is synchronized).

However, not all the input it has to work with is in CLOCK_TAI. For
example, software PTP timestamps are collected by the kernel using
__net_timestamp() -> ktime_get_real(), and that is in CLOCK_REALTIME
domain. So user space converts the CLOCK_REALTIME timestamps to
CLOCK_TAI by factoring in the UTC-to-TAI offset.

I am not in love with specifying this offset via a tunable script value
either. The isochron program has the ability to detect the kernel's TAI
offset and run with that, but sadly, phc2sys in non-automatic mode wants
the "-O" argument to be supplied externally. So regardless, I have to
come up with an offset to give to phc2sys which it will apply when
disciplining the PHC. So I figured why not just supply 37, the current
value.

I'm not sure what you're suggesting?

> > +ISOCHRON_CPU=1
> 
> Seems reasonable to assume two cpus.
> 
> > +
> > +# https://github.com/vladimiroltean/tsn-scripts
> > +# WARNING: isochron versions pre-1.0 are unstable,
> > +# always use the latest version
> > +require_command isochron
> > +require_command phc2sys
> > +require_command ptp4l
> > +
> > +phc2sys_start()
> > +{
> > +	local if_name=$1
> > +	local uds_address=$2
> > +	local extra_args=""
> > +
> > +	if ! [ -z "${uds_address}" ]; then
> > +		extra_args="${extra_args} -z ${uds_address}"
> > +	fi
> > +
> > +	phc2sys_log="$(mktemp)"
> > +
> > +	chrt -f 10 phc2sys -m \
> > +		-c ${if_name} \
> > +		-s CLOCK_REALTIME \
> > +		-O ${UTC_TAI_OFFSET} \
> > +		--step_threshold 0.00002 \
> > +		--first_step_threshold 0.00002 \
> > +		${extra_args} \
> > +		> "${phc2sys_log}" 2>&1 &
> > +	phc2sys_pid=$!
> > +
> > +	echo "phc2sys logs to ${phc2sys_log} and has pid ${phc2sys_pid}"
> > +
> > +	sleep 1
> > +}
> > +
> > +phc2sys_stop()
> > +{
> > +	{ kill ${phc2sys_pid} && wait ${phc2sys_pid}; } 2> /dev/null
> > +	rm "${phc2sys_log}" 2> /dev/null
> > +}
> > +
> > +ptp4l_start()
> > +{
> > +	local if_name=$1
> > +	local slave_only=$2
> > +	local uds_address=$3
> > +	local log="ptp4l_log_${if_name}"
> > +	local pid="ptp4l_pid_${if_name}"
> > +	local extra_args=""
> > +
> > +	if [ "${slave_only}" = true ]; then
> > +		extra_args="${extra_args} -s"
> > +	fi
> > +
> > +	# declare dynamic variables ptp4l_log_${if_name} and ptp4l_pid_${if_name}
> > +	# as global, so that they can be referenced later
> > +	declare -g "${log}=$(mktemp)"
> > +
> > +	chrt -f 10 ptp4l -m -2 -P \
> > +		-i ${if_name} \
> > +		--step_threshold 0.00002 \
> > +		--first_step_threshold 0.00002 \
> > +		--tx_timestamp_timeout 100 \
> > +		--uds_address="${uds_address}" \
> > +		${extra_args} \
> > +		> "${!log}" 2>&1 &
> > +	declare -g "${pid}=$!"
> > +
> > +	echo "ptp4l for interface ${if_name} logs to ${!log} and has pid ${!pid}"
> > +
> > +	sleep 1
> > +}
> > +
> > +ptp4l_stop()
> > +{
> > +	local if_name=$1
> > +	local log="ptp4l_log_${if_name}"
> > +	local pid="ptp4l_pid_${if_name}"
> > +
> > +	{ kill ${!pid} && wait ${!pid}; } 2> /dev/null
> > +	rm "${!log}" 2> /dev/null
> > +}
> > +
> > +cpufreq_max()
> > +{
> > +	local cpu=$1
> > +	local freq="cpu${cpu}_freq"
> > +	local governor="cpu${cpu}_governor"
> > +
> > +	# declare dynamic variables cpu${cpu}_freq and cpu${cpu}_governor as
> > +	# global, so they can be referenced later
> > +	declare -g "${freq}=$(cat /sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_min_freq)"
> > +	declare -g "${governor}=$(cat /sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_governor)"
> > +
> > +	cat /sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_max_freq > \
> > +		/sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_min_freq
> > +	echo -n "performance" > \
> > +		/sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_governor
> > +}
> > +
> > +cpufreq_restore()
> > +{
> > +	local cpu=$1
> > +	local freq="cpu${cpu}_freq"
> > +	local governor="cpu${cpu}_governor"
> > +
> > +	echo "${!freq}" > /sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_min_freq
> > +	echo -n "${!governor}" > \
> > +		/sys/bus/cpu/devices/cpu${cpu}/cpufreq/scaling_governor
> > +}
> > +
> > +isochron_recv_start()
> > +{
> > +	local if_name=$1
> > +	local uds=$2
> > +	local extra_args=$3
> > +
> > +	if ! [ -z "${uds}" ]; then
> > +		extra_args="--unix-domain-socket ${uds}"
> > +	fi
> > +
> > +	isochron rcv \
> > +		--interface ${if_name} \
> > +		--sched-priority 98 \
> > +		--sched-rr \
> 
> Why SCHED_RR?

Because it's not SCHED_OTHER? Why not SCHED_RR?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] selftests: forwarding: add Per-Stream Filtering and Policing test for Ocelot
  2022-04-29  9:38   ` Vladimir Oltean
@ 2022-04-29 10:15     ` Kurt Kanzenbach
  2022-04-29 11:00       ` Vladimir Oltean
  0 siblings, 1 reply; 8+ messages in thread
From: Kurt Kanzenbach @ 2022-04-29 10:15 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: netdev, Jakub Kicinski, David S. Miller, Paolo Abeni,
	Eric Dumazet, Florian Fainelli, Vivien Didelot, Andrew Lunn,
	Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Vinicius Costa Gomes, Gerhard Engleder, Y.B. Lu, Xiaoliang Yang,
	Richard Cochran, Sebastian Andrzej Siewior, Yannick Vignon,
	Rui Sousa, Jiri Pirko, Ido Schimmel

[-- Attachment #1: Type: text/plain, Size: 4072 bytes --]

On Fri Apr 29 2022, Vladimir Oltean wrote:
> Hi Kurt,
>
> Thanks for reviewing.
>
> On Fri, Apr 29, 2022 at 08:32:22AM +0200, Kurt Kanzenbach wrote:
>> Hi Vladimir,
>> 
>> On Thu Apr 28 2022, Vladimir Oltean wrote:
>> > The Felix VSC9959 switch in NXP LS1028A supports the tc-gate action
>> > which enforced time-based access control per stream. A stream as seen by
>> > this switch is identified by {MAC DA, VID}.
>> >
>> > We use the standard forwarding selftest topology with 2 host interfaces
>> > and 2 switch interfaces. The host ports must require timestamping non-IP
>> > packets and supporting tc-etf offload, for isochron to work. The
>> > isochron program monitors network sync status (ptp4l, phc2sys) and
>> > deterministically transmits packets to the switch such that the tc-gate
>> > action either (a) always accepts them based on its schedule, or
>> > (b) always drops them.
>> >
>> > I tried to keep as much of the logic that isn't specific to the NXP
>> > LS1028A in a new tsn_lib.sh, for future reuse. This covers
>> > synchronization using ptp4l and phc2sys, and isochron.
>> 
>> For running this selftest `isochron` tool is required. That's neither
>> packaged on Linux distributions or available in the kernel source. I
>> guess, it has to be built from your Github account/repository?
>
> This is slightly inconvenient, yes. But for this selftest in particular,
> a more specialized setup is required anyway, as it only runs on an NXP
> LS1028A based board. So I guess it's only the smaller of several
> inconveniences?

The thing is, you already moved common parts to a library. So, future
TSN selftests for other devices, switches, NIC(s) may come around and reuse
isochron.

>
> A few years ago when I decided to work on isochron, I searched for an
> application for detailed network latency testing and I couldn't find
> one. I don't think the situation has improved a lot since then.

It didn't :/.

> If isochron is useful for a larger audience, I can look into what I
> can do about distribution. It's license-compatible with the kernel,
> but it's a large-ish program to just toss into
> tools/testing/selftests/, plus I still commit rather frequently to it,
> and I'd probably annoy the crap out of everyone if I move its
> development to netdev@vger.kernel.org.

I agree. Nevertheless, having a standardized tool for this kind latency
testing would be nice. For instance, cyclictest is also not part of the
kernel, but packaged for all major Linux distributions.

>> > +# Tunables
>> > +UTC_TAI_OFFSET=37
>> 
>> Why do you need the UTC to TAI offset? isochron could just use CLOCK_TAI
>> as clockid for the task scheduling.
>
> isochron indeed works in CLOCK_TAI (this is done so that all timestamps
> are chronologically ordered when everything is synchronized).
>
> However, not all the input it has to work with is in CLOCK_TAI. For
> example, software PTP timestamps are collected by the kernel using
> __net_timestamp() -> ktime_get_real(), and that is in CLOCK_REALTIME
> domain. So user space converts the CLOCK_REALTIME timestamps to
> CLOCK_TAI by factoring in the UTC-to-TAI offset.
>
> I am not in love with specifying this offset via a tunable script value
> either. The isochron program has the ability to detect the kernel's TAI
> offset and run with that, but sadly, phc2sys in non-automatic mode wants
> the "-O" argument to be supplied externally. So regardless, I have to
> come up with an offset to give to phc2sys which it will apply when
> disciplining the PHC. So I figured why not just supply 37, the current
> value.

OK, makes sense. I just wondering whether there's a better solution to
specifying that 37.

>> > +	isochron rcv \
>> > +		--interface ${if_name} \
>> > +		--sched-priority 98 \
>> > +		--sched-rr \
>> 
>> Why SCHED_RR?
>
> Because it's not SCHED_OTHER? Why not SCHED_RR?

I was more thinking of SCHED_FIFO. RR performs round robin with a fixed
time slice (100ms). Is that what you want?

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] selftests: forwarding: add Per-Stream Filtering and Policing test for Ocelot
  2022-04-29 10:15     ` Kurt Kanzenbach
@ 2022-04-29 11:00       ` Vladimir Oltean
  2022-04-29 14:30         ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 8+ messages in thread
From: Vladimir Oltean @ 2022-04-29 11:00 UTC (permalink / raw)
  To: Kurt Kanzenbach
  Cc: netdev, Jakub Kicinski, David S. Miller, Paolo Abeni,
	Eric Dumazet, Florian Fainelli, Vivien Didelot, Andrew Lunn,
	Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Vinicius Costa Gomes, Gerhard Engleder, Y.B. Lu, Xiaoliang Yang,
	Richard Cochran, Sebastian Andrzej Siewior, Yannick Vignon,
	Rui Sousa, Jiri Pirko, Ido Schimmel

On Fri, Apr 29, 2022 at 12:15:39PM +0200, Kurt Kanzenbach wrote:
> On Fri Apr 29 2022, Vladimir Oltean wrote:
> > Hi Kurt,
> >
> > Thanks for reviewing.
> >
> > On Fri, Apr 29, 2022 at 08:32:22AM +0200, Kurt Kanzenbach wrote:
> >> Hi Vladimir,
> >> 
> >> On Thu Apr 28 2022, Vladimir Oltean wrote:
> >> > The Felix VSC9959 switch in NXP LS1028A supports the tc-gate action
> >> > which enforced time-based access control per stream. A stream as seen by
> >> > this switch is identified by {MAC DA, VID}.
> >> >
> >> > We use the standard forwarding selftest topology with 2 host interfaces
> >> > and 2 switch interfaces. The host ports must require timestamping non-IP
> >> > packets and supporting tc-etf offload, for isochron to work. The
> >> > isochron program monitors network sync status (ptp4l, phc2sys) and
> >> > deterministically transmits packets to the switch such that the tc-gate
> >> > action either (a) always accepts them based on its schedule, or
> >> > (b) always drops them.
> >> >
> >> > I tried to keep as much of the logic that isn't specific to the NXP
> >> > LS1028A in a new tsn_lib.sh, for future reuse. This covers
> >> > synchronization using ptp4l and phc2sys, and isochron.
> >> 
> >> For running this selftest `isochron` tool is required. That's neither
> >> packaged on Linux distributions or available in the kernel source. I
> >> guess, it has to be built from your Github account/repository?
> >
> > This is slightly inconvenient, yes. But for this selftest in particular,
> > a more specialized setup is required anyway, as it only runs on an NXP
> > LS1028A based board. So I guess it's only the smaller of several
> > inconveniences?
> 
> The thing is, you already moved common parts to a library. So, future
> TSN selftests for other devices, switches, NIC(s) may come around and reuse
> isochron.

Right, it's hard to figure out what's common when the sample size is 1 :-/
So you're saying I should do what I did with mausezahn in the main lib.sh, i.e.

REQUIRE_MZ=${REQUIRE_MZ:=yes}

if [[ "$REQUIRE_MZ" = "yes" ]]; then
	require_command $MZ
fi

?

> > A few years ago when I decided to work on isochron, I searched for an
> > application for detailed network latency testing and I couldn't find
> > one. I don't think the situation has improved a lot since then.
> 
> It didn't :/.

Part of the reason might be that not a lot of people care about TSN testing?
In my experience it's pretty damn hard to build a Linux-based network
connected system that sends/receives data with a cycle time of 100 us or
lower.

And automatically ensuring that the system is prepared to handle that
workload when running a selftest (like this) is... well, I gave up.
If you have some suggestions on more RT-related sanity checks that can
be added to tsn_lib.sh I'd be glad to add them.

> > If isochron is useful for a larger audience, I can look into what I
> > can do about distribution. It's license-compatible with the kernel,
> > but it's a large-ish program to just toss into
> > tools/testing/selftests/, plus I still commit rather frequently to it,
> > and I'd probably annoy the crap out of everyone if I move its
> > development to netdev@vger.kernel.org.
> 
> I agree. Nevertheless, having a standardized tool for this kind latency
> testing would be nice. For instance, cyclictest is also not part of the
> kernel, but packaged for all major Linux distributions.

Right, the thing is that I'm giving myself the liberty to still make
backwards-incompatible changes to isochron until it reaches v1.0 (right
now it's at v0.7 + 14 patches, so v0.8 should be coming rather soon).
I don't really want to submit unstable software for inclusion in a
distro (plus I don't know what distros would be interested in TSN
testing, see above).
And isochron itself needs to become more stable by gathering more users,
being integrated in scripts such as selftests, catering to more varied
requirements.
So it's a bit of a chicken and egg situation.

> >> > +# Tunables
> >> > +UTC_TAI_OFFSET=37
> >> 
> >> Why do you need the UTC to TAI offset? isochron could just use CLOCK_TAI
> >> as clockid for the task scheduling.
> >
> > isochron indeed works in CLOCK_TAI (this is done so that all timestamps
> > are chronologically ordered when everything is synchronized).
> >
> > However, not all the input it has to work with is in CLOCK_TAI. For
> > example, software PTP timestamps are collected by the kernel using
> > __net_timestamp() -> ktime_get_real(), and that is in CLOCK_REALTIME
> > domain. So user space converts the CLOCK_REALTIME timestamps to
> > CLOCK_TAI by factoring in the UTC-to-TAI offset.
> >
> > I am not in love with specifying this offset via a tunable script value
> > either. The isochron program has the ability to detect the kernel's TAI
> > offset and run with that, but sadly, phc2sys in non-automatic mode wants
> > the "-O" argument to be supplied externally. So regardless, I have to
> > come up with an offset to give to phc2sys which it will apply when
> > disciplining the PHC. So I figured why not just supply 37, the current
> > value.
> 
> OK, makes sense. I just wondering whether there's a better solution to
> specifying that 37.

Essentially, we could work with whatever the system is configured for,
but the system is probably not configured for anything (see
https://www.mail-archive.com/linuxptp-users@lists.sourceforge.net/msg02463.html)

The problem is that phc2sys wants a -O option. We could query the kernel
from the script itself to figure out the current TAI offset, just to
provide it to phc2sys. That offset would probably be 0 (but it could
also be 37 on a well configured system).

The thing is that isochron, when not told via command line what the UTC
offset is, proceeds to ask around what the UTC offset is, and tries to
settle on the most plausible value. It queries the kernel (sees 0), and
queries ptp4l, and that will report 37 (this is configurable via
"utc_offset" in default.cfg). So isochron says, "oh, hey, half the
system thinks the UTC offset is 37, and half the system has no idea.
Let me just update the kernel's UTC offset to 37 so that we all agree".

And that is fine too, and it makes CLOCK_TAI timers work properly, but
we've just told phc2sys to operate with a -O option of 0... If phc2sys
ran in automatic mode, I think that wouldn't have been a problem either,
because it queries ptp4l for the UTC offset as well. But it won't,
because we want to establish a fixed direction for synchronization
(CLOCK_REALTIME -> the master PHC).

> >> > +	isochron rcv \
> >> > +		--interface ${if_name} \
> >> > +		--sched-priority 98 \
> >> > +		--sched-rr \
> >> 
> >> Why SCHED_RR?
> >
> > Because it's not SCHED_OTHER? Why not SCHED_RR?
> 
> I was more thinking of SCHED_FIFO. RR performs round robin with a fixed
> time slice (100ms). Is that what you want?
> 
> Thanks,
> Kurt

I don't think I'm necessarily going to hit that condition, as isochron
will periodically go to sleep much sooner than once every 100 ms.
Nonetheless I can update this option to --sched-fifo.

I'm going to wait at least until 24 hours after v1 before resubmitting
v2 with some changes, so if there is any other feedback please let me know.

Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] selftests: forwarding: add Per-Stream Filtering and Policing test for Ocelot
  2022-04-29 11:00       ` Vladimir Oltean
@ 2022-04-29 14:30         ` Sebastian Andrzej Siewior
  2022-04-30 13:19           ` Vladimir Oltean
  0 siblings, 1 reply; 8+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-04-29 14:30 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Kurt Kanzenbach, netdev, Jakub Kicinski, David S. Miller,
	Paolo Abeni, Eric Dumazet, Florian Fainelli, Vivien Didelot,
	Andrew Lunn, Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Vinicius Costa Gomes, Gerhard Engleder, Y.B. Lu, Xiaoliang Yang,
	Richard Cochran, Yannick Vignon, Rui Sousa, Jiri Pirko,
	Ido Schimmel

On 2022-04-29 11:00:39 [+0000], Vladimir Oltean wrote:
> > I agree. Nevertheless, having a standardized tool for this kind latency
> > testing would be nice. For instance, cyclictest is also not part of the
> > kernel, but packaged for all major Linux distributions.
> 
> Right, the thing is that I'm giving myself the liberty to still make
> backwards-incompatible changes to isochron until it reaches v1.0 (right
> now it's at v0.7 + 14 patches, so v0.8 should be coming rather soon).
> I don't really want to submit unstable software for inclusion in a
> distro (plus I don't know what distros would be interested in TSN
> testing, see above).

Users of those distros, that need to test TSN, will be interested in
having it packaged rather than having it to compile first. Just make it
available, point to it in tests etc. and it should get packaged.

> And isochron itself needs to become more stable by gathering more users,
> being integrated in scripts such as selftests, catering to more varied
> requirements.
> So it's a bit of a chicken and egg situation.
If it is completely experimental then it could be added to, say,
Debian's experimental distribution so user's of unstable/sid can install
it fairly easy but it won't become part of the upcoming stable release
(the relevant freeze is currently set to 2023-02).

Sebastian

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] selftests: forwarding: add Per-Stream Filtering and Policing test for Ocelot
  2022-04-29 14:30         ` Sebastian Andrzej Siewior
@ 2022-04-30 13:19           ` Vladimir Oltean
  2022-05-02 14:26             ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 8+ messages in thread
From: Vladimir Oltean @ 2022-04-30 13:19 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Kurt Kanzenbach, netdev, Jakub Kicinski, David S. Miller,
	Paolo Abeni, Eric Dumazet, Florian Fainelli, Vivien Didelot,
	Andrew Lunn, Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Vinicius Costa Gomes, Gerhard Engleder, Y.B. Lu, Xiaoliang Yang,
	Richard Cochran, Yannick Vignon, Rui Sousa, Jiri Pirko,
	Ido Schimmel

Hi Sebastian,

On Fri, Apr 29, 2022 at 04:30:47PM +0200, Sebastian Andrzej Siewior wrote:
> On 2022-04-29 11:00:39 [+0000], Vladimir Oltean wrote:
> > > I agree. Nevertheless, having a standardized tool for this kind latency
> > > testing would be nice. For instance, cyclictest is also not part of the
> > > kernel, but packaged for all major Linux distributions.
> > 
> > Right, the thing is that I'm giving myself the liberty to still make
> > backwards-incompatible changes to isochron until it reaches v1.0 (right
> > now it's at v0.7 + 14 patches, so v0.8 should be coming rather soon).
> > I don't really want to submit unstable software for inclusion in a
> > distro (plus I don't know what distros would be interested in TSN
> > testing, see above).
> 
> Users of those distros, that need to test TSN, will be interested in
> having it packaged rather than having it to compile first. Just make it
> available, point to it in tests etc. and it should get packaged.
> 
> > And isochron itself needs to become more stable by gathering more users,
> > being integrated in scripts such as selftests, catering to more varied
> > requirements.
> > So it's a bit of a chicken and egg situation.
> If it is completely experimental then it could be added to, say,
> Debian's experimental distribution so user's of unstable/sid can install
> it fairly easy but it won't become part of the upcoming stable release
> (the relevant freeze is currently set to 2023-02).
> 
> Sebastian

If I get you right, you're saying it would be preferable to submit
isochron for inclusion in Debian Testing.

Ok, I've submitted an Intent To Package:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1010396

but if you don't mind, I'd still like to proceed with v2 right away,
since the process of getting isochron packaged by Debian is essentially
unbounded and I wouldn't like to create a dependency between packaging
and this selftest. There is already a link to the Github repo in
tsn_lib.sh, I expect people are still going to get it from there for a
while. I will also make the dependency optional via a REQUIRE_ISOCHRON
variable, as discussed with Kurt. I hope that's ok.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] selftests: forwarding: add Per-Stream Filtering and Policing test for Ocelot
  2022-04-30 13:19           ` Vladimir Oltean
@ 2022-05-02 14:26             ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 8+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-05-02 14:26 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Kurt Kanzenbach, netdev, Jakub Kicinski, David S. Miller,
	Paolo Abeni, Eric Dumazet, Florian Fainelli, Vivien Didelot,
	Andrew Lunn, Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Vinicius Costa Gomes, Gerhard Engleder, Y.B. Lu, Xiaoliang Yang,
	Richard Cochran, Yannick Vignon, Rui Sousa, Jiri Pirko,
	Ido Schimmel

On 2022-04-30 13:19:59 [+0000], Vladimir Oltean wrote:
> Hi Sebastian,
Hi Vladimir,

> If I get you right, you're saying it would be preferable to submit
> isochron for inclusion in Debian Testing.

If you intend to change command line switches or something that could
confuse users, you could start with experimental. Otherwise, yes,
testing.

> Ok, I've submitted an Intent To Package:
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1010396

Thank you. 

> but if you don't mind, I'd still like to proceed with v2 right away,
> since the process of getting isochron packaged by Debian is essentially
> unbounded and I wouldn't like to create a dependency between packaging
> and this selftest. There is already a link to the Github repo in
> tsn_lib.sh, I expect people are still going to get it from there for a
> while. I will also make the dependency optional via a REQUIRE_ISOCHRON
> variable, as discussed with Kurt. I hope that's ok.

Sure.

Sebastian

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-05-02 14:28 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-28 20:48 [PATCH net-next] selftests: forwarding: add Per-Stream Filtering and Policing test for Ocelot Vladimir Oltean
2022-04-29  6:32 ` Kurt Kanzenbach
2022-04-29  9:38   ` Vladimir Oltean
2022-04-29 10:15     ` Kurt Kanzenbach
2022-04-29 11:00       ` Vladimir Oltean
2022-04-29 14:30         ` Sebastian Andrzej Siewior
2022-04-30 13:19           ` Vladimir Oltean
2022-05-02 14:26             ` Sebastian Andrzej Siewior

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.