Re: [PATCH net-next] selftests: forwarding: add Per-Stream Filtering and Policing test for Ocelot

From: Kurt Kanzenbach <kurt@linutronix.de>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Jakub Kicinski <kuba@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Paolo Abeni <pabeni@redhat.com>,
	Eric Dumazet <edumazet@google.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Claudiu Manoil <claudiu.manoil@nxp.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	"UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
	Vinicius Costa Gomes <vinicius.gomes@intel.com>,
	Gerhard Engleder <gerhard@engleder-embedded.com>,
	"Y.B. Lu" <yangbo.lu@nxp.com>,
	Xiaoliang Yang <xiaoliang.yang_1@nxp.com>,
	Richard Cochran <richardcochran@gmail.com>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Yannick Vignon <yannick.vignon@nxp.com>,
	Rui Sousa <rui.sousa@nxp.com>, Jiri Pirko <jiri@nvidia.com>,
	Ido Schimmel <idosch@nvidia.com>
Subject: Re: [PATCH net-next] selftests: forwarding: add Per-Stream Filtering and Policing test for Ocelot
Date: Fri, 29 Apr 2022 12:15:39 +0200	[thread overview]
Message-ID: <87h76ci4ac.fsf@kurt> (raw)
In-Reply-To: <20220429093845.tyzwcwppsgbjbw2s@skbuf>

[-- 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 --]