Message-ID: <656f66023f7bd_3dd6422942a@willemb.c.googlers.com.notmuch>
Date: Tue, 05 Dec 2023 13:03:46 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Stanislav Fomichev <sdf@...gle.com>, 
 Florian Bezdeka <florian.bezdeka@...mens.com>
Cc: "Song, Yoong Siang" <yoong.siang.song@...el.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 Jesper Dangaard Brouer <hawk@...nel.org>, 
 "David S . Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 Jonathan Corbet <corbet@....net>, 
 Bjorn Topel <bjorn@...nel.org>, 
 "Karlsson, Magnus" <magnus.karlsson@...el.com>, 
 "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>, 
 Jonathan Lemon <jonathan.lemon@...il.com>, 
 Alexei Starovoitov <ast@...nel.org>, 
 Daniel Borkmann <daniel@...earbox.net>, 
 John Fastabend <john.fastabend@...il.com>, 
 Lorenzo Bianconi <lorenzo@...nel.org>, 
 Tariq Toukan <tariqt@...dia.com>, 
 Willem de Bruijn <willemb@...gle.com>, 
 Maxime Coquelin <mcoquelin.stm32@...il.com>, 
 Andrii Nakryiko <andrii@...nel.org>, 
 Mykola Lysenko <mykolal@...com>, 
 Martin KaFai Lau <martin.lau@...ux.dev>, 
 Song Liu <song@...nel.org>, 
 Yonghong Song <yonghong.song@...ux.dev>, 
 KP Singh <kpsingh@...nel.org>, 
 Hao Luo <haoluo@...gle.com>, 
 Jiri Olsa <jolsa@...nel.org>, 
 Shuah Khan <shuah@...nel.org>, 
 Alexandre Torgue <alexandre.torgue@...s.st.com>, 
 Jose Abreu <joabreu@...opsys.com>, 
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
 "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>, 
 "bpf@...r.kernel.org" <bpf@...r.kernel.org>, 
 "xdp-hints@...-project.net" <xdp-hints@...-project.net>, 
 "linux-stm32@...md-mailman.stormreply.com" <linux-stm32@...md-mailman.stormreply.com>, 
 "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, 
 "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>
Subject: Re: [xdp-hints] Re: [PATCH bpf-next v3 2/3] net: stmmac: add Launch
 Time support to XDP ZC

Stanislav Fomichev wrote:
> On Tue, Dec 5, 2023 at 7:34 AM Florian Bezdeka
> <florian.bezdeka@...mens.com> wrote:
> >
> > On Tue, 2023-12-05 at 15:25 +0000, Song, Yoong Siang wrote:
> > > On Monday, December 4, 2023 10:55 PM, Willem de Bruijn wrote:
> > > > Jesper Dangaard Brouer wrote:
> > > > >
> > > > >
> > > > > On 12/3/23 17:51, Song Yoong Siang wrote:
> > > > > > This patch enables Launch Time (Time-Based Scheduling) support to XDP zero
> > > > > > copy via XDP Tx metadata framework.
> > > > > >
> > > > > > Signed-off-by: Song Yoong Siang<yoong.siang.song@...el.com>
> > > > > > ---
> > > > > >   drivers/net/ethernet/stmicro/stmmac/stmmac.h      |  2 ++
> > > > >
> > > > > As requested before, I think we need to see another driver implementing
> > > > > this.
> > > > >
> > > > > I propose driver igc and chip i225.
> > >
> > > Sure. I will include igc patches in next version.
> > >
> > > > >
> > > > > The interesting thing for me is to see how the LaunchTime max 1 second
> > > > > into the future[1] is handled code wise. One suggestion is to add a
> > > > > section to Documentation/networking/xsk-tx-metadata.rst per driver that
> > > > > mentions/documents these different hardware limitations.  It is natural
> > > > > that different types of hardware have limitations.  This is a close-to
> > > > > hardware-level abstraction/API, and IMHO as long as we document the
> > > > > limitations we can expose this API without too many limitations for more
> > > > > capable hardware.
> > >
> > > Sure. I will try to add hardware limitations in documentation.
> > >
> > > >
> > > > I would assume that the kfunc will fail when a value is passed that
> > > > cannot be programmed.
> > > >
> > >
> > > In current design, the xsk_tx_metadata_request() dint got return value.
> > > So user won't know if their request is fail.
> > > It is complex to inform user which request is failing.
> > > Therefore, IMHO, it is good that we let driver handle the error silently.
> > >
> >
> > If the programmed value is invalid, the packet will be "dropped" / will
> > never make it to the wire, right?

Programmable behavior is to either drop or cap to some boundary
value, such as the farthest programmable time in the future: the
horizon. In fq:

                /* Check if packet timestamp is too far in the future. */
                if (fq_packet_beyond_horizon(skb, q, now)) {
                        if (q->horizon_drop) {
                                        q->stat_horizon_drops++;
                                        return qdisc_drop(skb, sch, to_free);
                        }
                        q->stat_horizon_caps++;
                        skb->tstamp = now + q->horizon;
                }
                fq_skb_cb(skb)->time_to_send = skb->tstamp;

Drop is the more obviously correct mode.

Programming with a clock source that the driver does not support will
then be a persistent failure.

Preferably, this driver capability can be queried beforehand (rather
than only through reading error counters afterwards).

Perhaps it should not be a driver task to convert from possibly
multiple clock sources to the device native clock. Right now, we do
use per-device timecounters for this, implemented in the driver.

As for which clocks are relevant. For PTP, I suppose the device PHC,
converted to nsec. For pacing offload, TCP uses CLOCK_MONOTONIC.

> >
> > That is clearly a situation that the user should be informed about. For
> > RT systems this normally means that something is really wrong regarding
> > timing / cycle overflow. Such systems have to react on that situation.
> 
> In general, af_xdp is a bit lacking in this 'notify the user that they
> somehow messed up' area :-(
> For example, pushing a tx descriptor with a wrong addr/len in zc mode
> will not give any visible signal back (besides driver potentially
> spilling something into dmesg as it was in the mlx case).
> We can probably start with having some counters for these events?

This is because the AF_XDP completion queue descriptor format is only
a u64 address?

Could error conditions be reported on tx completion in the metadata,
using xsk_tx_metadata_complete?

Powered by blists - more mailing lists