From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-x1034.google.com (mail-pj1-x1034.google.com [IPv6:2607:f8b0:4864:20::1034]) by mail.toke.dk (Postfix) with ESMTPS id CD6A2A12F21 for ; Mon, 26 Jun 2023 19:00:19 +0200 (CEST) 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=20221208 header.b=y9qrdV86 Received: by mail-pj1-x1034.google.com with SMTP id 98e67ed59e1d1-262ec7b261bso1113691a91.3 for ; Mon, 26 Jun 2023 10:00:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1687798818; x=1690390818; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=g0BTBT7E6AaVVXNbcifwyc9C7bHK5aLZp/eqXvlgyto=; b=y9qrdV86HObNHfc6NXeTlUXblyJwZLb5qD/T4fWIzAbWfw/98yLH2SohxhTkE6wPoG 3t55+8zTETSL4j3ewGZTpsBI/H1r6zCeI/KWiT8c/ZJnNJHP9jTtm7Xq+PFo8ABJJ9RD AOgSh+RrQnjZyA6pm73gzlc8ZbryLMselpZnRTbTorBKCTt07l412CwBieQEArPROuxX suVzE8KMu/0ckudHQeZ7/xBdF2VrmWKzIplO/UdiljKmWVqTPTGTSb9QpdwLlm/vCDAR 6HjC0vzI0OkxcgFzUf5l4iaSbzyOZoTXMn2xvFvmgukzr33xYlWRpmTsDYRhNHoXGoyr tyFA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687798818; x=1690390818; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=g0BTBT7E6AaVVXNbcifwyc9C7bHK5aLZp/eqXvlgyto=; b=hFY9lBZAR2UGVKktx5XWhAk1XinjML0bJIFGCadz51y/I3Bfwbwx1gBgz5BeHyqbvX PfLVnF5r9c3oWNbdjl/4ip5sKpj82fe7ad/latC2wQ8pDHMdI48MDiaiWt99ycuv18vx gAHIEWSj9464ty0VkYYE1TZ/+dDosp+tJwCWLKRCR54L5vCwvplZYaAe0I/KUPaKrXOG zsmhOBKMq2W4+n9+WiMUnPkzwOZy7LLESxHEyVgcfpq/ctW6W1U1W5rxi2/kk9MJ9SEg c5etC6YY16w5zlR1+pGcuEU5tP6erGFjP3eDsZ/y+nWbUhlEAoe1EsC5syZIVik8K9lv UN7w== X-Gm-Message-State: AC+VfDyMCHJecO3SQ2wFKKhfByWllqvdHlWEVDYGn5oBNvnmTmTAeKuV WyKT96M/03ZNHAAZ5ttIHok1K05Ad9lnpnRmy3hkvA== X-Google-Smtp-Source: ACHHUZ753qel1EEhBEcizxknrTyReQhxpX4uUM1P1RL2JrBOQUSnKSjQ38n1ael6V26xd+7LZDsW1+3oQO5htyT0+20= X-Received: by 2002:a17:90a:4611:b0:262:f99b:a530 with SMTP id w17-20020a17090a461100b00262f99ba530mr2502736pjg.34.1687798817494; Mon, 26 Jun 2023 10:00:17 -0700 (PDT) MIME-Version: 1.0 References: <20230621170244.1283336-1-sdf@google.com> <20230621170244.1283336-4-sdf@google.com> <57b9fc14-c02e-f0e5-148d-a549ebab6cf6@brouer.com> <435d1630-c3f4-97fb-b6fe-9795d1f0bf33@redhat.com> In-Reply-To: <435d1630-c3f4-97fb-b6fe-9795d1f0bf33@redhat.com> From: Stanislav Fomichev Date: Mon, 26 Jun 2023 10:00:05 -0700 Message-ID: To: Jesper Dangaard Brouer Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: TOG4K2Q6GUHU5XDRDIUMZBZR7PFACCZZ X-Message-ID-Hash: TOG4K2Q6GUHU5XDRDIUMZBZR7PFACCZZ X-MailFrom: sdf@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: brouer@redhat.com, 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, =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= , "Karlsson, Magnus" , "xdp-hints@xdp-project.net" X-Mailman-Version: 3.3.8 Precedence: list Subject: [xdp-hints] Re: [RFC bpf-next v2 03/11] xsk: Support XDP_TX_METADATA_LEN List-Id: XDP hardware hints design discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Sat, Jun 24, 2023 at 2:02=E2=80=AFAM Jesper Dangaard Brouer wrote: > > > > On 23/06/2023 19.41, Stanislav Fomichev wrote: > > On Fri, Jun 23, 2023 at 3:24=E2=80=AFAM Jesper Dangaard Brouer > > wrote: > >> > >> > >> > >> On 22/06/2023 19.55, Stanislav Fomichev wrote: > >>> On Thu, Jun 22, 2023 at 2:11=E2=80=AFAM Jesper D. Brouer wrote: > >>>> > >>>> > >>>> This needs to be reviewed by AF_XDP maintainers Magnus and Bj=C3=B8r= n (Cc) > >>>> > >>>> On 21/06/2023 19.02, Stanislav Fomichev wrote: > >>>>> For zerocopy mode, tx_desc->addr can point to the arbitrary offset > >>>>> and carry some TX metadata in the headroom. For copy mode, there > >>>>> is no way currently to populate skb metadata. > >>>>> > >>>>> Introduce new XDP_TX_METADATA_LEN that indicates how many bytes > >>>>> to treat as metadata. Metadata bytes come prior to tx_desc address > >>>>> (same as in RX case). > >>>> > >>>> From looking at the code, this introduces a socket option for thi= s TX > >>>> metadata length (tx_metadata_len). > >>>> This implies the same fixed TX metadata size is used for all packets= . > >>>> Maybe describe this in patch desc. > >>> > >>> I was planning to do a proper documentation page once we settle on al= l > >>> the details (similar to the one we have for rx). > >>> > >>>> What is the plan for dealing with cases that doesn't populate same/f= ull > >>>> TX metadata size ? > >>> > >>> Do we need to support that? I was assuming that the TX layout would b= e > >>> fixed between the userspace and BPF. > >> > >> I hope you don't mean fixed layout, as the whole point is adding > >> flexibility and extensibility. > > > > I do mean a fixed layout between the userspace (af_xdp) and devtx progr= am. > > At least fixed max size of the metadata. The userspace and the bpf > > prog can then use this fixed space to implement some flexibility > > (btf_ids, versioned structs, bitmasks, tlv, etc). > > If we were to make the metalen vary per packet, we'd have to signal > > its size per packet. Probably not worth it? > > Existing XDP metadata implementation also expand in a fixed/limited > sized memory area, but communicate size per packet in this area (also > for validation purposes). BUT for AF_XDP we don't have room for another > pointer or size in the AF_XDP descriptor (see struct xdp_desc). > > > > > >>> If every packet would have a different metadata length, it seems like > >>> a nightmare to parse? > >>> > >> > >> No parsing is really needed. We can simply use BTF IDs and type cast = in > >> BPF-prog. Both BPF-prog and userspace have access to the local BTF ids= , > >> see [1] and [2]. > >> > >> It seems we are talking slightly past each-other(?). Let me rephrase > >> and reframe the question, what is your *plan* for dealing with differe= nt > >> *types* of TX metadata. The different struct *types* will of-cause ha= ve > >> different sizes, but that is okay as long as they fit into the maximum > >> size set by this new socket option XDP_TX_METADATA_LEN. > >> Thus, in principle I'm fine with XSK having configured a fixed headroo= m > >> for metadata, but we need a plan for handling more than one type and > >> perhaps a xsk desc indicator/flag for knowing TX metadata isn't random > >> data ("leftover" since last time this mem was used). > > > > Yeah, I think the above correctly catches my expectation here. Some > > headroom is reserved via XDP_TX_METADATA_LEN and the flexibility is > > offloaded to the bpf program via btf_id/tlv/etc. > > > > Regarding leftover metadata: can we assume the userspace will take > > care of setting it up? > > > >> With this kfunc approach, then things in-principle, becomes a contract > >> between the "local" TX-hook BPF-prog and AF_XDP userspace. These two > >> components can as illustrated here [1]+[2] can coordinate based on loc= al > >> BPF-prog BTF IDs. This approach works as-is today, but patchset > >> selftests examples don't use this and instead have a very static > >> approach (that people will copy-paste). > >> > >> An unsolved problem with TX-hook is that it can also get packets from > >> XDP_REDIRECT and even normal SKBs gets processed (right?). How does t= he > >> BPF-prog know if metadata is valid and intended to be used for e.g. > >> requesting the timestamp? (imagine metadata size happen to match) > > > > My assumption was the bpf program can do ifindex/netns filtering. Plus > > maybe check that the meta_len is the one that's expected. > > Will that be enough to handle XDP_REDIRECT? > > I don't think so, using the meta_len (+ ifindex/netns) to communicate > activation of TX hardware hints is too weak and not enough. This is an > implicit API for BPF-programmers to understand and can lead to implicit > activation. > > Think about what will happen for your AF_XDP send use-case. For > performance reasons AF_XDP don't zero out frame memory. Thus, meta_len > is fixed even if not used (and can contain garbage), it can by accident > create hard-to-debug situations. As discussed with Magnus+Maryam > before, we found it was practical (and faster than mem zero) to extend > AF_XDP descriptor (see struct xdp_desc) with some flags to > indicate/communicate this frame comes with TX metadata hints. What is that "if not used" situation? Can the metadata itself have is_used bit? The userspace has to initialize at least that bit. We can definitely add that extra "has_metadata" bit to the descriptor, but I'm trying to understand whether we can do without it. > >> > >> BPF-prog API bpf_core_type_id_local: > >> - [1] > >> https://github.com/xdp-project/bpf-examples/blob/master/AF_XDP-interac= tion/af_xdp_kern.c#L80 > >> > >> Userspace API btf__find_by_name_kind: > >> - [2] > >> https://github.com/xdp-project/bpf-examples/blob/master/AF_XDP-interac= tion/lib_xsk_extend.c#L185 > >> > > >