Message-ID: <20220429110038.6jv76qeyjjxborez@skbuf>
Date:   Fri, 29 Apr 2022 11:00:39 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Kurt Kanzenbach <kurt@...utronix.de>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        Gerhard Engleder <gerhard@...leder-embedded.com>,
        "Y.B. Lu" <yangbo.lu@....com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        Richard Cochran <richardcochran@...il.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Yannick Vignon <yannick.vignon@....com>,
        Rui Sousa <rui.sousa@....com>, Jiri Pirko <jiri@...dia.com>,
        Ido Schimmel <idosch@...dia.com>
Subject: Re: [PATCH net-next] selftests: forwarding: add Per-Stream Filtering
 and Policing test for Ocelot

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@...r.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!

Powered by blists - more mailing lists