From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-x62c.google.com (mail-pl1-x62c.google.com [IPv6:2607:f8b0:4864:20::62c]) by mail.toke.dk (Postfix) with ESMTPS id 343999CD3BB for ; Fri, 9 Dec 2022 00:47:09 +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=20210112 header.b=nXOiiX+V Received: by mail-pl1-x62c.google.com with SMTP id p24so3123626plw.1 for ; Thu, 08 Dec 2022 15:47:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; 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=qsiZ9s0yp8OUDlVyAwoSvSCb5GsD3AVTDpcvuxldQfs=; b=nXOiiX+VU152FyXbI9HbjdFrabUKjr2ExBALOa4KD1qpYheo3s//ZXaiKEj2spZ7oB lDyFoINq111EyQnfb1ZOg9mX4je9hOTWtixoDjmCXnD//x6Byj5YmhwTOuYzmYSV03q2 0GmIN8X/Mt7wB8A4cCNZCibWQ93HqiWHuUN+nyUNASQW/GAawMOEARYd7QnDDkgIvkZh Zl7qi82uPc0rzMpHvHEtx9YSzi5x1WN0M/rdzCDO1tyAmSyN1+1flttrLi6Uv7W/dIt1 uxs4SeoMgcFgq9w3U9a4O+i6Q/XtZHNe/u4gum9pb8dT9TZ/ygpKNGIEZpA3pJy+uwrm OtQQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=qsiZ9s0yp8OUDlVyAwoSvSCb5GsD3AVTDpcvuxldQfs=; b=1nYY0d+XOLxaNuYA6tV3WOh3Q0Fn3E7LfxA7aES0X4EV83qsQrkfycxJAUS3ZD8eIP 9TDl85H+4q0orRzQ7U2HEc6/FUEeDoDGY/AzFUCFw+CmhrljGSjaZt1CU2swCfqnaglm oqqrx7NyfxpObuC7Ap/+rzTLF1/jXAXa2ViINXgqkHMKNM2HvV0i/B4ac2iCEN/1OoFe MUxUbD9OyLn4GOllBJmmar582OonMpA0uL+qwlC3JRevAIfLmPTwIqMRsDSQKSQ5tYJ5 n+A0Yq1lNzTuCXR8+gFL4Hjz4fOOlmClQldxxIrAzM/CwF5NNzECTxGiGYALc10uHX0M q9HQ== X-Gm-Message-State: ANoB5pnGyATkS9rYqbmsk8XR6cIIxI91xUH4jN8c47OSPW5tABI1YaVF rKWxFMzNcEmdX8SPOZAM7Oxxuxq9KasRyvZ+HfSmUg== X-Google-Smtp-Source: AA0mqf53HTVaefPFy0V6y2L9HDcjm12+3TUzV47rrKRIHbnH5Tk9Q+Y9tu1d7g1+iMNJKIsJtvsc9Q2rd4XXLT+X+c0= X-Received: by 2002:a17:90a:6382:b0:219:fbc:a088 with SMTP id f2-20020a17090a638200b002190fbca088mr65066280pjj.162.1670543227082; Thu, 08 Dec 2022 15:47:07 -0800 (PST) MIME-Version: 1.0 References: <20221206024554.3826186-1-sdf@google.com> <20221206024554.3826186-4-sdf@google.com> <878rjhldv0.fsf@toke.dk> In-Reply-To: <878rjhldv0.fsf@toke.dk> From: Stanislav Fomichev Date: Thu, 8 Dec 2022 15:46:55 -0800 Message-ID: To: =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: KW3WSHOK5IJVMIXPH34FVLEZX4FDW6K6 X-Message-ID-Hash: KW3WSHOK5IJVMIXPH34FVLEZX4FDW6K6 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: 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, David Ahern , Jakub Kicinski , Willem de Bruijn , Jesper Dangaard Brouer , Anatoly Burakov , Alexander Lobakin , Magnus Karlsson , Maryam Tahhan , xdp-hints@xdp-project.net, netdev@vger.kernel.org X-Mailman-Version: 3.3.7 Precedence: list Subject: [xdp-hints] Re: [PATCH bpf-next v3 03/12] bpf: XDP metadata RX kfuncs List-Id: XDP hardware hints design discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, Dec 8, 2022 at 2:39 PM Toke H=C3=B8iland-J=C3=B8rgensen wrote: > > Stanislav Fomichev writes: > > > There is an ndo handler per kfunc, the verifier replaces a call to the > > generic kfunc with a call to the per-device one. > > > > For XDP, we define a new kfunc set (xdp_metadata_kfunc_ids) which > > implements all possible metatada kfuncs. Not all devices have to > > implement them. If kfunc is not supported by the target device, > > the default implementation is called instead. > > So one unfortunate consequence of this "fallback to the default > implementation" is that it's really easy to get a step wrong and end up > with something that doesn't work. Specifically, if you load an XDP > program that calls the metadata kfuncs, but don't set the ifindex and > flag on load, the kfunc resolution will work just fine, but you'll end > up calling the default kfunc implementations (and get no data). I ran > into this multiple times just now when playing around with it and > implementing the freplace support. > > So I really think it would be a better user experience if we completely > block (with a nice error message!) the calling of the metadata kfuncs if > the program is not device-bound... Oh, right, that's a good point. Having defaults for dev-bound only makes total sense. > Another UX thing I ran into is that libbpf will bail out if it can't > find the kfunc in the kernel vmlinux, even if the code calling the > function is behind an always-false if statement (which would be > eliminated as dead code from the verifier). This makes it a bit hard to > conditionally use them. Should libbpf just allow the load without > performing the relocation (and let the verifier worry about it), or > should we have a bpf_core_kfunc_exists() macro to use for checking? > Maybe both? I'm not sure how libbpf can allow the load without performing the relocation; maybe I'm missing something. IIUC, libbpf uses the kfunc name (from the relocation?) and replaces it with the kfunc id, right? Having bpf_core_kfunc_exists would help, but this probably needs compiler work first to preserve some of the kfunc traces in vmlinux.h? So yeah, I don't have any good ideas/suggestions here on how to make it all magically work :-( > > Upon loading, if BPF_F_XDP_HAS_METADATA is passed via prog_flags, > > we treat prog_index as target device for kfunc resolution. > > [...] > > > - if (!bpf_prog_map_compatible(map, prog)) { > > - bpf_prog_put(prog); > > - return ERR_PTR(-EINVAL); > > - } > > + /* When tail-calling from a non-dev-bound program to a dev-bound = one, > > + * XDP metadata helpers should be disabled. Until it's implemente= d, > > + * prohibit adding dev-bound programs to tail-call maps. > > + */ > > + if (bpf_prog_is_dev_bound(prog->aux)) > > + goto err; > > + > > + if (!bpf_prog_map_compatible(map, prog)) > > + goto err; > > I think it's better to move the new check into bpf_prog_map_compatible() > itself; that way it'll cover cpumaps and devmaps as well :) Will do, thanks! > -Toke >