From: Stanislav Fomichev <sdf@google.com>
To: Jesper Dangaard Brouer <hawk@kernel.org>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org,
haoluo@google.com, jolsa@kernel.org, kuba@kernel.org,
toke@kernel.org, willemb@google.com, dsahern@kernel.org,
magnus.karlsson@intel.com, bjorn@kernel.org,
maciej.fijalkowski@intel.com, yoong.siang.song@intel.com,
netdev@vger.kernel.org, xdp-hints@xdp-project.net
Subject: [xdp-hints] Re: [PATCH bpf-next v5 02/13] xsk: Add TX timestamp and TX checksum offload support
Date: Mon, 20 Nov 2023 12:45:00 -0800 [thread overview]
Message-ID: <ZVvFTFD1k-aRc3rY@google.com> (raw)
In-Reply-To: <a0dc04da-eb36-4824-b774-fd16f3f65875@kernel.org>
On 11/16, Jesper Dangaard Brouer wrote:
>
>
> On 11/15/23 14:37, Stanislav Fomichev wrote:
> > On 11/15, Jesper Dangaard Brouer wrote:
> > >
> > >
> > > On 11/13/23 18:02, Stanislav Fomichev wrote:
> > > > On 11/13, Jesper Dangaard Brouer wrote:
> > > > >
> > > > >
> > > > > On 11/13/23 15:10, Stanislav Fomichev wrote:
> > > > > > On Mon, Nov 13, 2023 at 5:16 AM Jesper Dangaard Brouer <hawk@kernel.org> wrote:
> > > > > > >
> > > > > > >
> > > > > > > On 11/2/23 23:58, Stanislav Fomichev wrote:
> > > > > > > > diff --git a/include/uapi/linux/if_xdp.h b/include/uapi/linux/if_xdp.h
> > > > > > > > index 2ecf79282c26..b0ee7ad19b51 100644
> > > > > > > > --- a/include/uapi/linux/if_xdp.h
> > > > > > > > +++ b/include/uapi/linux/if_xdp.h
> > > > > > > > @@ -106,6 +106,41 @@ struct xdp_options {
> > > > > > > > #define XSK_UNALIGNED_BUF_ADDR_MASK \
> > > > > > > > ((1ULL << XSK_UNALIGNED_BUF_OFFSET_SHIFT) - 1)
> > > > > > > >
> > > > > > > > +/* Request transmit timestamp. Upon completion, put it into tx_timestamp
> > > > > > > > + * field of struct xsk_tx_metadata.
> > > > > > > > + */
> > > > > > > > +#define XDP_TXMD_FLAGS_TIMESTAMP (1 << 0)
> > > > > > > > +
> > > > > > > > +/* Request transmit checksum offload. Checksum start position and offset
> > > > > > > > + * are communicated via csum_start and csum_offset fields of struct
> > > > > > > > + * xsk_tx_metadata.
> > > > > > > > + */
> > > > > > > > +#define XDP_TXMD_FLAGS_CHECKSUM (1 << 1)
> > > > > > > > +
> > > > > > > > +/* AF_XDP offloads request. 'request' union member is consumed by the driver
> > > > > > > > + * when the packet is being transmitted. 'completion' union member is
> > > > > > > > + * filled by the driver when the transmit completion arrives.
> > > > > > > > + */
> > > > > > > > +struct xsk_tx_metadata {
> > > > > > > > + union {
> > > > > > > > + struct {
> > > > > > > > + __u32 flags;
> > > > > > > > +
> > > > > > > > + /* XDP_TXMD_FLAGS_CHECKSUM */
> > > > > > > > +
> > > > > > > > + /* Offset from desc->addr where checksumming should start. */
> > > > > > > > + __u16 csum_start;
> > > > > > > > + /* Offset from csum_start where checksum should be stored. */
> > > > > > > > + __u16 csum_offset;
> > > > > > > > + } request;
> > > > > > > > +
> > > > > > > > + struct {
> > > > > > > > + /* XDP_TXMD_FLAGS_TIMESTAMP */
> > > > > > > > + __u64 tx_timestamp;
> > > > > > > > + } completion;
> > > > > > > > + };
> > > > > > > > +};
> > > > > > >
> > > > > > > This looks wrong to me. It looks like member @flags is not avail at
> > > > > > > completion time. At completion time, I assume we also want to know if
> > > > > > > someone requested to get the timestamp for this packet (else we could
> > > > > > > read garbage).
> > > > > >
> > > > > > I've moved the parts that are preserved across tx and tx completion
> > > > > > into xsk_tx_metadata_compl.
> > > > > > This is to address Magnus/Maciej feedback where userspace might race
> > > > > > with the kernel.
> > > > > > See: https://lore.kernel.org/bpf/ZNoJenzKXW5QSR3E@boxer/
> > > > > >
> > > > >
> > > > > Does this mean that every driver have to extend their TX-desc ring with
> > > > > sizeof(struct xsk_tx_metadata_compl)?
> > > > > Won't this affect the performance of this V5?
> > > >
> > > > Yes, but it doesn't have to be a descriptor. Might be some internal
> > > > driver completion queue (as in the case of mlx5). And definitely does
> > > > affect performance :-( (see all the static branches to disable it)
> > > > > $ pahole -C xsk_tx_metadata_compl
> > > > > ./drivers/net/ethernet/stmicro/stmmac/stmmac.ko
> > > > > struct xsk_tx_metadata_compl {
> > > > > __u64 * tx_timestamp; /* 0 8 */
> > > > >
> > > > > /* size: 8, cachelines: 1, members: 1 */
> > > > > /* last cacheline: 8 bytes */
> > > > > };
> > > > >
> > > > > Guess, I must be misunderstanding, as I was expecting to see the @flags
> > > > > member being preserved across, as I get the race there.
> > > > >
> > > > > Looking at stmmac driver, it does look like this xsk_tx_metadata_compl
> > > > > is part of the TX-ring for completion (tx_skbuff_dma) and the
> > > > > tx_timestamp data is getting stored here. How is userspace AF_XDP
> > > > > application getting access to the tx_timestamp data?
> > > > > I though this was suppose to get stored in metadata data area (umem)?
> > > > >
> > > > > Also looking at the code, the kernel would not have a "crash" race on
> > > > > the flags member (if we preserve in struct), because the code checks the
> > > > > driver HW-TS config-state + TX-descriptor for the availability of a
> > > > > HW-TS in the descriptor.
> > > >
> > > > xsk_tx_metadata_compl stores a pointer to the completion timestamp
> > > > in the umem, so everything still arrives via the metadata area.
> > > >
> > > > We want to make sure the flags are not changing across tx and tx completion.
> > > > Instead of saving the flags, we just use that xsk_tx_metadata_compl to
> > > > signal to the completion that "I know that I've requested the tx
> > > > completion timestamp, please put it at this address in umem".
> > > >
> > > > I store the pointer instead of flags to avoid doing pointer math again
> > > > at completion. But it's an implementation detail and somewhat abstracted
> > > > from the drivers (besides the fact that it's probably has to fit in 8
> > > > bytes).
> > >
> > > I see it now (what I missed). At TX time you are storing a pointer where
> > > to (later) write the TS at completion time. It just seems overkill to
> > > store 8 byte (pointer) to signal (via NULL) if the HWTS was requested.
> > > Space in the drivers TX-ring is performance critical, and I think driver
> > > developers would prefer to find a bit to indicate HWTS requested.
> > >
> > > If HWTS was *NOT* requested, then the metadata area will not be updated
> > > (right, correct?). Then memory area is basically garbage that survived.
> > > How does the AF_XDP application know this packet contains a HWTS or not?
> > >
> > > From an UAPI PoV wouldn't it be easier to use (and extend) via keeping
> > > the @flags member (in struct xsk_tx_metadata), but (as you already do)
> > > not let kernel checks depend on it (to avoid the races).
> >
> > I was assuming the userspace can keep this signal out of band or use
> > the same idea as suggested with padding struct xsk_tx_metadata to keep
> > some data around. But I see your point, it might be convenient to
> > keep the original flags around during completion on the uapi side.
> >
> > I think I can just move flags from the request union member to the outer
> > struct. So the struct xsk_tx_metadata would look like:
> >
> > struct xsk_tx_metadata {
> > __u32 flags; /* maybe can even make this u64? */
> >
>
> Yes to u64 for two reasons (1) this becomes UAPI and
> (2) better alignment for tx_timestamp.
> But I'm open to keeping it u32.
>
> > union {
> > __u16 csum_start;
> > __u16 csum_offset;
> > } request;
> >
> > union {
> > __u64 tx_timestamp;
> > } completion;
> >
> > __u32 padding; /* to drop this padding */
> > };
> >
> > But I'd also keep the existing xsk_tx_metadata_compl to carry the
> > pointer+signal around. As I mentioned previously, it's completely
> > opaque to the driver and we can change the internals in the future.
> >
>
> Sure, it is an implementation detail and my objections are mostly that I
> don't find it as a pretty code approach that can be hard to follow.
> Maybe driver developer will object and change this later if it cost too
> much to increase the element size in their TX-ring queues.
To make sure I understand, your preference is to save the flags, right?
A potential problem with that approach might be that we'd also have to
carry the pointer to the original umem chunk (blowing the overhead by extra
8 bytes) or pulling it of the tx completion descriptors in the drivers (extra
complexity). Pulling it out of the tx completion also might be
problematic because because we store iova/dma addresses in the
descriptors?
next prev parent reply other threads:[~2023-11-20 20:45 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-02 22:58 [xdp-hints] [PATCH bpf-next v5 00/13] xsk: TX metadata Stanislav Fomichev
2023-11-02 22:58 ` [xdp-hints] [PATCH bpf-next v5 01/13] xsk: Support tx_metadata_len Stanislav Fomichev
2023-11-02 22:58 ` [xdp-hints] [PATCH bpf-next v5 02/13] xsk: Add TX timestamp and TX checksum offload support Stanislav Fomichev
2023-11-13 13:16 ` [xdp-hints] " Jesper Dangaard Brouer
2023-11-13 14:10 ` Stanislav Fomichev
2023-11-13 16:29 ` Jesper Dangaard Brouer
2023-11-13 17:02 ` Stanislav Fomichev
2023-11-15 10:05 ` Jesper Dangaard Brouer
2023-11-15 13:37 ` Stanislav Fomichev
2023-11-16 14:30 ` Jesper Dangaard Brouer
2023-11-20 20:45 ` Stanislav Fomichev [this message]
2023-11-02 22:58 ` [xdp-hints] [PATCH bpf-next v5 03/13] tools: ynl: Print xsk-features from the sample Stanislav Fomichev
2023-11-02 22:58 ` [xdp-hints] [PATCH bpf-next v5 04/13] net/mlx5e: Implement AF_XDP TX timestamp and checksum offload Stanislav Fomichev
2023-11-02 22:58 ` [xdp-hints] [PATCH bpf-next v5 05/13] net: stmmac: Add Tx HWTS support to XDP ZC Stanislav Fomichev
2023-11-02 22:58 ` [xdp-hints] [PATCH bpf-next v5 06/13] xsk: Document tx_metadata_len layout Stanislav Fomichev
2023-11-05 12:45 ` [xdp-hints] " Simon Horman
2023-11-06 16:47 ` Stanislav Fomichev
2023-11-02 22:58 ` [xdp-hints] [PATCH bpf-next v5 07/13] xsk: Validate xsk_tx_metadata flags Stanislav Fomichev
2023-11-02 22:58 ` [xdp-hints] [PATCH bpf-next v5 08/13] xsk: Add option to calculate TX checksum in SW Stanislav Fomichev
2023-11-02 22:58 ` [xdp-hints] [PATCH bpf-next v5 09/13] selftests/xsk: Support tx_metadata_len Stanislav Fomichev
2023-11-02 22:58 ` [xdp-hints] [PATCH bpf-next v5 10/13] selftests/bpf: Add csum helpers Stanislav Fomichev
2023-11-02 22:58 ` [xdp-hints] [PATCH bpf-next v5 11/13] selftests/bpf: Add TX side to xdp_metadata Stanislav Fomichev
2023-11-02 22:58 ` [xdp-hints] [PATCH bpf-next v5 12/13] selftests/bpf: Convert xdp_hw_metadata to XDP_USE_NEED_WAKEUP Stanislav Fomichev
2023-11-02 22:58 ` [xdp-hints] [PATCH bpf-next v5 13/13] selftests/bpf: Add TX side to xdp_hw_metadata Stanislav Fomichev
2023-11-06 23:31 ` [xdp-hints] Re: [PATCH bpf-next v5 00/13] xsk: TX metadata Jakub Kicinski
2023-11-09 18:03 ` Alexei Starovoitov
2023-11-09 18:07 ` Stanislav Fomichev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://lists.xdp-project.net/postorius/lists/xdp-hints.xdp-project.net/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZVvFTFD1k-aRc3rY@google.com \
--to=sdf@google.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bjorn@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=dsahern@kernel.org \
--cc=haoluo@google.com \
--cc=hawk@kernel.org \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=kuba@kernel.org \
--cc=maciej.fijalkowski@intel.com \
--cc=magnus.karlsson@intel.com \
--cc=martin.lau@linux.dev \
--cc=netdev@vger.kernel.org \
--cc=song@kernel.org \
--cc=toke@kernel.org \
--cc=willemb@google.com \
--cc=xdp-hints@xdp-project.net \
--cc=yhs@fb.com \
--cc=yoong.siang.song@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox