From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-x104a.google.com (mail-pj1-x104a.google.com [IPv6:2607:f8b0:4864:20::104a]) by mail.toke.dk (Postfix) with ESMTPS id 1A386A3E70C for ; Mon, 20 Nov 2023 21:45:04 +0100 (CET) Authentication-Results: mail.toke.dk; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20230601 header.b=OI53vY3I Received: by mail-pj1-x104a.google.com with SMTP id 98e67ed59e1d1-27ff8065e61so6445553a91.1 for ; Mon, 20 Nov 2023 12:45:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1700513102; x=1701117902; darn=xdp-project.net; h=content-transfer-encoding:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:from:to:cc:subject:date:message-id :reply-to; bh=Q0+j8fQL1sJz92+chsBB43nsB2MtdrD3vnFCGs59vhU=; b=OI53vY3I1adqGWnLxs6kSxXBlAHvavFsXDNM+I9q7sHObzSy6rICzmVDjWSJ6x1GBB goc/IOb5ZGt9bNM2hr/iKe7Qo0vbTgmK8RQSQohU1JTIImGC3eYIaqrWxmatV6p68xXC 8XcXml9AvTOv5e28WEsVy3up6AB2cDtNwlMrBwe7d/OkiP+rIcHUo5EXPJGj9+dOvIO2 E0VrULvH1hCWdZv6zqK1aIGpGVCzavN9EHSbNk3goPHJDylHYTUahhm+Vj38MdA3e6kk iy8sUWhGTHzMXiSx1+gBjXsKGMMNFH6wYjvUtKbiMWZv2prLCz13rAEwqW8In7x2rj43 Zlfg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700513102; x=1701117902; h=content-transfer-encoding:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=Q0+j8fQL1sJz92+chsBB43nsB2MtdrD3vnFCGs59vhU=; b=udtkOeoK+50AhLsWyFRKzIS0o5mZify07UloZDy9S4T5ywd86JDYEwxEqK71MqwmkJ T70551hVc0zvNaLGZpWK8ESyJCHmI17bZE+qM6KAn8JPFhsse5w4Sv2HfECpfAcrvY9j Klc+mxfxOVQuKda0CmKN0sA2ftzQa5Z++piijK6UOETuOSen+7MkXy2OJzqFI5Y1gC7Z joc2PCRUXDJjVKn376U0bcgVQvNk2ygx571Rldi6tFcFzBEPePoJnxAvFqbyUy6Q+7s5 R1A0bgBB9fEd2NubcxVrcXnFkOAbcmUnAnimpwlx/0whKea50dIdoAmrEHLr+0/t0FRS pXMQ== X-Gm-Message-State: AOJu0Yx5tjLBCG0tkA41n3DIuhfHwpmEBrl7sat2wC/LZ70eHuYGe/DH 3nI3gUUvm5v5Bb7j8cG6XiYKtEI= X-Google-Smtp-Source: AGHT+IFf16w/HlmsFcKaSkyBb7ftE+eaSs3vM0zSXLvq2PgaNAM02ekw9Tn4BBTkGGulNt//3vU2tnQ= X-Received: from sdf.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5935]) (user=sdf job=sendgmr) by 2002:a17:90a:6586:b0:283:9a71:457a with SMTP id k6-20020a17090a658600b002839a71457amr2258267pjj.6.1700513102471; Mon, 20 Nov 2023 12:45:02 -0800 (PST) Date: Mon, 20 Nov 2023 12:45:00 -0800 In-Reply-To: Mime-Version: 1.0 References: <20231102225837.1141915-1-sdf@google.com> <20231102225837.1141915-3-sdf@google.com> <2ed17b27-f211-4f58-95b5-5a71914264f3@kernel.org> Message-ID: From: Stanislav Fomichev To: Jesper Dangaard Brouer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: V7QXLOQOBRW5MUDEZAJO5AZYUKWNZKIH X-Message-ID-Hash: V7QXLOQOBRW5MUDEZAJO5AZYUKWNZKIH X-MailFrom: 3TsVbZQMKCXkpacdlldib.Zljuam-efkqpuam-molgbZq.kbq@flex--sdf.bounces.google.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header 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 X-Mailman-Version: 3.3.8 Precedence: list Subject: [xdp-hints] Re: [PATCH bpf-next v5 02/13] xsk: Add TX timestamp and TX checksum offload support List-Id: XDP hardware hints design discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 11/16, Jesper Dangaard Brouer wrote: >=20 >=20 > On 11/15/23 14:37, Stanislav Fomichev wrote: > > On 11/15, Jesper Dangaard Brouer wrote: > > >=20 > > >=20 > > > On 11/13/23 18:02, Stanislav Fomichev wrote: > > > > On 11/13, Jesper Dangaard Brouer wrote: > > > > >=20 > > > > >=20 > > > > > On 11/13/23 15:10, Stanislav Fomichev wrote: > > > > > > On Mon, Nov 13, 2023 at 5:16=E2=80=AFAM Jesper Dangaard Brouer = wrote: > > > > > > >=20 > > > > > > >=20 > > > > > > > On 11/2/23 23:58, Stanislav Fomichev wrote: > > > > > > > > diff --git a/include/uapi/linux/if_xdp.h b/include/uapi/lin= ux/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) > > > > > > > >=20 > > > > > > > > +/* Request transmit timestamp. Upon completion, put it int= o tx_timestamp > > > > > > > > + * field of struct xsk_tx_metadata. > > > > > > > > + */ > > > > > > > > +#define XDP_TXMD_FLAGS_TIMESTAMP (1 << 0) > > > > > > > > + > > > > > > > > +/* Request transmit checksum offload. Checksum start posit= ion 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 cons= umed by the driver > > > > > > > > + * when the packet is being transmitted. 'completion' unio= n member is > > > > > > > > + * filled by the driver when the transmit completion arriv= es. > > > > > > > > + */ > > > > > > > > +struct xsk_tx_metadata { > > > > > > > > + union { > > > > > > > > + struct { > > > > > > > > + __u32 flags; > > > > > > > > + > > > > > > > > + /* XDP_TXMD_FLAGS_CHECKSUM */ > > > > > > > > + > > > > > > > > + /* Offset from desc->addr where check= summing should start. */ > > > > > > > > + __u16 csum_start; > > > > > > > > + /* Offset from csum_start where check= sum should be stored. */ > > > > > > > > + __u16 csum_offset; > > > > > > > > + } request; > > > > > > > > + > > > > > > > > + struct { > > > > > > > > + /* XDP_TXMD_FLAGS_TIMESTAMP */ > > > > > > > > + __u64 tx_timestamp; > > > > > > > > + } completion; > > > > > > > > + }; > > > > > > > > +}; > > > > > > >=20 > > > > > > > This looks wrong to me. It looks like member @flags is not av= ail at > > > > > > > completion time. At completion time, I assume we also want t= o know if > > > > > > > someone requested to get the timestamp for this packet (else = we could > > > > > > > read garbage). > > > > > >=20 > > > > > > I've moved the parts that are preserved across tx and tx comple= tion > > > > > > 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/ > > > > > >=20 > > > > >=20 > > > > > Does this mean that every driver have to extend their TX-desc rin= g with > > > > > sizeof(struct xsk_tx_metadata_compl)? > > > > > Won't this affect the performance of this V5? > > > >=20 > > > > 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 do= es > > > > 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 */ > > > > >=20 > > > > > /* size: 8, cachelines: 1, members: 1 */ > > > > > /* last cacheline: 8 bytes */ > > > > > }; > > > > >=20 > > > > > Guess, I must be misunderstanding, as I was expecting to see the = @flags > > > > > member being preserved across, as I get the race there. > > > > >=20 > > > > > 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_XD= P > > > > > application getting access to the tx_timestamp data? > > > > > I though this was suppose to get stored in metadata data area (um= em)? > > > > >=20 > > > > > Also looking at the code, the kernel would not have a "crash" rac= e on > > > > > the flags member (if we preserve in struct), because the code che= cks the > > > > > driver HW-TS config-state + TX-descriptor for the availability of= a > > > > > HW-TS in the descriptor. > > > >=20 > > > > xsk_tx_metadata_compl stores a pointer to the completion timestamp > > > > in the umem, so everything still arrives via the metadata area. > > > >=20 > > > > We want to make sure the flags are not changing across tx and tx co= mpletion. > > > > 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". > > > >=20 > > > > I store the pointer instead of flags to avoid doing pointer math ag= ain > > > > at completion. But it's an implementation detail and somewhat abstr= acted > > > > from the drivers (besides the fact that it's probably has to fit in= 8 > > > > bytes). > > >=20 > > > I see it now (what I missed). At TX time you are storing a pointer wh= ere > > > to (later) write the TS at completion time. It just seems overkill t= o > > > 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 dri= ver > > > developers would prefer to find a bit to indicate HWTS requested. > > >=20 > > > If HWTS was *NOT* requested, then the metadata area will not be updat= ed > > > (right, correct?). Then memory area is basically garbage that survive= d. > > > How does the AF_XDP application know this packet contains a HWTS or n= ot? > > >=20 > > > From an UAPI PoV wouldn't it be easier to use (and extend) via keepi= ng > > > the @flags member (in struct xsk_tx_metadata), but (as you already do= ) > > > not let kernel checks depend on it (to avoid the races). > >=20 > > 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. > >=20 > > I think I can just move flags from the request union member to the oute= r > > struct. So the struct xsk_tx_metadata would look like: > >=20 > > struct xsk_tx_metadata { > > __u32 flags; /* maybe can even make this u64? */ > >=20 >=20 > 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. >=20 > > union { > > __u16 csum_start; > > __u16 csum_offset; > > } request; > >=20 > > union { > > __u64 tx_timestamp; > > } completion; > >=20 > > __u32 padding; /* to drop this padding */ > > }; > >=20 > > 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. > >=20 >=20 > 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 (ext= ra complexity). Pulling it out of the tx completion also might be problematic because because we store iova/dma addresses in the descriptors?