From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mail.toke.dk (Postfix) with ESMTPS id E949BA13BCA for ; Thu, 29 Jun 2023 14:01:28 +0200 (CEST) Authentication-Results: mail.toke.dk; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=iZ5ZPFVR DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1688040087; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=uSpgJmIFFR4HJnYxcDaUk1aoo7kIHQ2jqmuv95n0lY0=; b=iZ5ZPFVRUJ7oWbC34DLM7AyTFtBp965lgWGLhP1sCpL2BjgCXCu42gO9vcn/5JU5Zzdvqb T/sH2Js4VuX5QlY8OosrvtslgwtPoW8SK5VgfGF1GSGoWO7i8SQSv46/ingQFIpRItqZkC YSUC5EUhFAwM+G7N/4eju0R4vmcp9mI= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-177-x_BPpzVmNXi-5MgI2AzyUw-1; Thu, 29 Jun 2023 08:01:25 -0400 X-MC-Unique: x_BPpzVmNXi-5MgI2AzyUw-1 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-3fa979d0c32so2756975e9.2 for ; Thu, 29 Jun 2023 05:01:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688040083; x=1690632083; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=2v0l5lRAbV2Wf694zzIazWXahqcGK+nFJgcSv3W4pq4=; b=WTjx5w8qVE4i+uICl1dGzLLwFdoSDVsOKrafQFaoLG2X1vcRh15t+Dkt/D7ls9GC/H QOPge+ba1uPutxotfTbYvx335cXTJgshGS/ewcI7iSMGmPPix6Qx5PlaDJnPCqbwHClt Z1RvUbGCOuUYAs533icpehCuxdhWE5iI0LThlpd93OUTDpIIeGFfm5bCtyUDvAKrxSzD bbYzIUfjyBA+/IixrouP4oehzkAZOH03pKN9hdckqAz1Map+ERUu97OB9ERJP+URwNFi NiNNAjXEWePuKn66ti5T7sBJC7kCdTx0fiywFaa1w3WFlzWXx3mqPwXtw5wy8IwYB0i2 a83w== X-Gm-Message-State: AC+VfDxKaSqIbj/KDZOJVXKMLf9YG94r5eSfYyi+lCbFk1jCvdvKHBUT 7ooCAFYf5EwSSo/3XXpCI+6Sn+5eisvh0G1H6YyGqs/tLNzKC6ie0Exl4l/JvN8EovWiggJMoPz gGqHXCc8Glpd4DSeMi1FI X-Received: by 2002:a05:600c:24e:b0:3fb:af95:1f4d with SMTP id 14-20020a05600c024e00b003fbaf951f4dmr4144498wmj.15.1688040083520; Thu, 29 Jun 2023 05:01:23 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5q2Nuit0XX6S8TqYjSh3Cc+IFOOEc8W/3Q00LAUE2onY6SkmCScAbdv/yOP4uu5r/BlvYQsg== X-Received: by 2002:a05:600c:24e:b0:3fb:af95:1f4d with SMTP id 14-20020a05600c024e00b003fbaf951f4dmr4144479wmj.15.1688040083044; Thu, 29 Jun 2023 05:01:23 -0700 (PDT) Received: from alrua-x1.borgediget.toke.dk ([45.145.92.2]) by smtp.gmail.com with ESMTPSA id z24-20020a05600c221800b003fa96fe2bd9sm11291727wml.22.2023.06.29.05.01.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 29 Jun 2023 05:01:22 -0700 (PDT) Received: by alrua-x1.borgediget.toke.dk (Postfix, from userid 1000) id 6EF17BC0447; Thu, 29 Jun 2023 14:01:21 +0200 (CEST) From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= To: Magnus Karlsson In-Reply-To: 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> <871qhuh5ec.fsf@toke.dk> X-Clacks-Overhead: GNU Terry Pratchett Date: Thu, 29 Jun 2023 14:01:21 +0200 Message-ID: <87r0pufpf2.fsf@toke.dk> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: 2LUQOAA3CW3QISKPMLG5IIXTXROZAPCR X-Message-ID-Hash: 2LUQOAA3CW3QISKPMLG5IIXTXROZAPCR X-MailFrom: toke@redhat.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: Stanislav Fomichev , Jesper Dangaard Brouer , 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: Magnus Karlsson writes: > On Thu, 29 Jun 2023 at 13:30, Toke H=C3=B8iland-J=C3=B8rgensen wrote: >> >> Stanislav Fomichev writes: >> >> > On Wed, Jun 28, 2023 at 1:09=E2=80=AFAM Magnus Karlsson >> > wrote: >> >> >> >> On Mon, 26 Jun 2023 at 19:06, Stanislav Fomichev wro= te: >> >> > >> >> > 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 B= j=C3=B8rn (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 b= ytes >> >> > > >>>>> 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 this 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 sett= le on all >> >> > > >>> the details (similar to the one we have for rx). >> >> > > >>> >> >> > > >>>> What is the plan for dealing with cases that doesn't populat= e same/full >> >> > > >>>> TX metadata size ? >> >> > > >>> >> >> > > >>> Do we need to support that? I was assuming that the TX layout= would be >> >> > > >>> fixed between the userspace and BPF. >> >> > > >> >> >> > > >> I hope you don't mean fixed layout, as the whole point is addi= ng >> >> > > >> flexibility and extensibility. >> >> > > > >> >> > > > I do mean a fixed layout between the userspace (af_xdp) and dev= tx program. >> >> > > > At least fixed max size of the metadata. The userspace and the = bpf >> >> > > > prog can then use this fixed space to implement some flexibilit= y >> >> > > > (btf_ids, versioned structs, bitmasks, tlv, etc). >> >> > > > If we were to make the metalen vary per packet, we'd have to si= gnal >> >> > > > its size per packet. Probably not worth it? >> >> > > >> >> > > Existing XDP metadata implementation also expand in a fixed/limit= ed >> >> > > 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 se= ems like >> >> > > >>> a nightmare to parse? >> >> > > >>> >> >> > > >> >> >> > > >> No parsing is really needed. We can simply use BTF IDs and ty= pe 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 r= ephrase >> >> > > >> and reframe the question, what is your *plan* for dealing with= different >> >> > > >> *types* of TX metadata. The different struct *types* will of-= cause have >> >> > > >> 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= headroom >> >> > > >> for metadata, but we need a plan for handling more than one ty= pe 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 flexibilit= y is >> >> > > > offloaded to the bpf program via btf_id/tlv/etc. >> >> > > > >> >> > > > Regarding leftover metadata: can we assume the userspace will t= ake >> >> > > > 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. T= hese two >> >> > > >> components can as illustrated here [1]+[2] can coordinate base= d on local >> >> > > >> BPF-prog BTF IDs. This approach works as-is today, but patchs= et >> >> > > >> selftests examples don't use this and instead have a very stat= ic >> >> > > >> approach (that people will copy-paste). >> >> > > >> >> >> > > >> An unsolved problem with TX-hook is that it can also get packe= ts from >> >> > > >> XDP_REDIRECT and even normal SKBs gets processed (right?). Ho= w does the >> >> > > >> BPF-prog know if metadata is valid and intended to be used for= e.g. >> >> > > >> requesting the timestamp? (imagine metadata size happen to mat= ch) >> >> > > > >> >> > > > My assumption was the bpf program can do ifindex/netns filterin= g. 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 communi= cate >> >> > > 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 im= plicit >> >> > > 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, me= ta_len >> >> > > is fixed even if not used (and can contain garbage), it can by ac= cident >> >> > > create hard-to-debug situations. As discussed with Magnus+Maryam >> >> > > before, we found it was practical (and faster than mem zero) to e= xtend >> >> > > 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 descript= or, >> >> > but I'm trying to understand whether we can do without it. >> >> >> >> To me, this "has_metadata" bit in the descriptor is just an >> >> optimization. If it is 0, then there is no need to go and check the >> >> metadata field and you save some performance. Regardless of this bit, >> >> you need some way to say "is_used" for each metadata entry (at least >> >> when the number of metadata entries is >1). Three options come to min= d >> >> each with their pros and cons. >> >> >> >> #1: Let each metadata entry have an invalid state. Not possible for >> >> every metadata and requires the user/kernel to go scan through every >> >> entry for every packet. >> >> >> >> #2: Have a field of bits at the start of the metadata section (closes= t >> >> to packet data) that signifies if a metadata entry is valid or not. I= f >> >> there are N metadata entries in the metadata area, then N bits in thi= s >> >> field would be used to signify if the corresponding metadata is used >> >> or not. Only requires the user/kernel to scan the valid entries plus >> >> one access for the "is_used" bits. >> >> >> >> #3: Have N bits in the AF_XDP descriptor options field instead of the >> >> N bits in the metadata area of #2. Faster but would consume many >> >> precious bits in the fixed descriptor and cap the number of metadata >> >> entries possible at around 8. E.g., 8 for Rx, 8 for Tx, 1 for the >> >> multi-buffer work, and 15 for some future use. Depends on how daring >> >> we are. >> >> >> >> The "has_metadata" bit suggestion can be combined with 1 or 2. >> >> Approach 3 is just a fine grained extension of the idea itself. >> >> >> >> IMO, the best approach unfortunately depends on the metadata itself. >> >> If it is rarely valid, you want something like the "has_metadata" bit= . >> >> If it is nearly always valid and used, approach #1 (if possible for >> >> the metadata) should be the fastest. The decision also depends on the >> >> number of metadata entries you have per packet. Sorry that I do not >> >> have a good answer. My feeling is that we need something like #1 or >> >> #2, or maybe both, then if needed we can add the "has_metadata" bit o= r >> >> bits (#3) optimization. Can we do this encoding and choice (#1, #2, o= r >> >> a combo) in the eBPF program itself? Would provide us with the >> >> flexibility, if possible. >> > >> > Here is my take on it, lmk if I'm missing something: >> > >> > af_xdp users call this new setsockopt(XDP_TX_METADATA_LEN) when they >> > plan to use metadata on tx. >> > This essentially requires allocating a fixed headroom to carry the met= adata. >> > af_xdp machinery exports this fixed len into the bpf programs somehow >> > (devtx_frame.meta_len in this series). >> > Then it's up to the userspace and bpf program to agree on the layout. >> > If not every packet is expected to carry the metadata, there might be >> > some bitmask in the metadata area to indicate that. >> > >> > Iow, the metadata isn't interpreted by the kernel. It's up to the prog >> > to interpret it and call appropriate kfunc to enable some offload. >> >> The reason for the flag on RX is mostly performance: there's a >> substantial performance hit from reading the metadata area because it's >> not cache-hot; we want to avoid that when no metadata is in use. Putting >> the flag inside the metadata area itself doesn't work for this, because >> then you incur the cache miss just to read the flag. > > Not necessarily. Let us say that the flag is 4 bytes. Increase the > start address of the packet buffer with 4 and the flags field will be > on the same cache line as the first 60 bytes of the packet data > (assuming a 64 byte cache line size and the flags field is closest to > the start of the packet data). As long as you write something in those > first 60 bytes of packet data that cache line will be brought in and > will likely be in the cache when you access the bits in the metadata > field. The trick works similarly for Rx by setting the umem headroom > accordingly. Yeah, a trick like that was what I was alluding to with the "could" in this bit: >> but I see no reason it could not also occur on TX (it'll mostly >> depend on data alignment I guess?). right below the text you quoted ;) > But you are correct in that dedicating a bit in the descriptor will > make sure it is always hot, while the trick above is dependent on the > app wanting to read or write the first cache line worth of packet > data. Exactly; which is why I think it's worth the flag bit :) -Toke