From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3FEE3C433E0 for ; Tue, 16 Mar 2021 04:43:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id ECC526510F for ; Tue, 16 Mar 2021 04:42:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235051AbhCPEm1 (ORCPT ); Tue, 16 Mar 2021 00:42:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37936 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235050AbhCPEmI (ORCPT ); Tue, 16 Mar 2021 00:42:08 -0400 Received: from mail-yb1-xb29.google.com (mail-yb1-xb29.google.com [IPv6:2607:f8b0:4864:20::b29]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DDB1FC06174A; Mon, 15 Mar 2021 21:42:07 -0700 (PDT) Received: by mail-yb1-xb29.google.com with SMTP id u3so35511821ybk.6; Mon, 15 Mar 2021 21:42:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=3xJbacQTkJuXZYZ5dPtFSpZjC86+5pBMHeLF8x4cXpc=; b=bkzsqZKDm05dYqMhU6C7s30SRKe+xuJoofXJKbwr9RU+FXeRQrD3LBpGLKVpm94jOz fgdbvRFczz4iGSkcFID8i9HYgs8pvWhqgB+T3yu7EAdI97cDz8lXHn5LlWK4D41BKR6v 5hfLNtTbPfqAv2SCoUlACofLcHfILXwcdLBwUgqwunFEIHQAqEO1/UKoCGSjnzKB+4Xt Q8ceX0fp+AhYjp1wsTbHZuvrWCcP35HsdlsOCg0N9L+YuQTAHD2vIdiS5//9CcuQUcv9 Sdgak1ooDXNJA4PiSX/zZLpE/nZDwcB4LNUrIe/D1fPSiFjVYG0DedaVb8hCt2qeg3Lu O9mw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=3xJbacQTkJuXZYZ5dPtFSpZjC86+5pBMHeLF8x4cXpc=; b=bHSetd9FkBK1tun/0Jq+3bl6kqIRfthMHJtNwogV7xnUMqvAEBSr9PfCzZBxpjRDj0 62GEmfZ8jWnT5pgkfWwxufgQQ7mSIM6cmTWgyn1m2TXI+atLOo2dGYYZvjYvWynYvM7d 42Fe1TN8dl0dLX3m7xmZn/Omu21/gotXdATBvUZ1Xy5FjW9cZwCql3UxsjUSwD0X6Nys u5hRwCI8OCUOPhuoEZCgh8xhRwRLzP1lZV6ohANt+afZs7XeUE9E31plBUNZHP6IAL2H ngDicocFn2H/tRynnqGqEn0H29RlhXXPEYtWDrz+gsQ6J7k9/gihYSZ9Kyd5rHWhZXaq AhpQ== X-Gm-Message-State: AOAM530U/2ZQPkjZwxtW/V4O1CGXSZt59Pa5pnPpw4oXQlz1gVAUKoxi x6wv/xZxvuwfLp5mg7yJGt+Jr7wKrdn3RQnV8NY= X-Google-Smtp-Source: ABdhPJyaocZVg7IqJQRWn/PI2/hufcMQoiK2czRXgGaH0SY+SNl9Tf1TlJTCrAwbhMzk6tszR6kzQA7NcdoBaeHBUXo= X-Received: by 2002:a25:c6cb:: with SMTP id k194mr4273769ybf.27.1615869727191; Mon, 15 Mar 2021 21:42:07 -0700 (PDT) MIME-Version: 1.0 References: <20210310220211.1454516-1-revest@chromium.org> <20210310220211.1454516-4-revest@chromium.org> In-Reply-To: From: Andrii Nakryiko Date: Mon, 15 Mar 2021 21:41:56 -0700 Message-ID: Subject: Re: [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field To: Florent Revest Cc: bpf , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Yonghong Song , KP Singh , Brendan Jackman , open list Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Mon, Mar 15, 2021 at 9:36 PM Andrii Nakryiko wrote: > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest wrote: > > > > When initializing the __param array with a one liner, if all args are > > const, the initial array value will be placed in the rodata section but > > because libbpf does not support relocation in the rodata section, any > > pointer in this array will stay NULL. > > > > This is a workaround, ideally the rodata relocation should be supported > > by libbpf but this would require a disproportionate amount of work given > > the actual usecases. (it is very unlikely that one uses a const array of > > relocated addresses) Can you please drop this paragraph? This is not a workaround, it's a completely working code that should continue working. And this is not something that libbpf doesn't support, there is no kernel interface to make it work at all. Please add Fixes: tag as well. > > > > Signed-off-by: Florent Revest > > --- > > tools/lib/bpf/bpf_tracing.h | 30 +++++++++++++++++++++++++++++- > > 1 file changed, 29 insertions(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h > > index f9ef37707888..f6a2deb3cd5b 100644 > > --- a/tools/lib/bpf/bpf_tracing.h > > +++ b/tools/lib/bpf/bpf_tracing.h > > @@ -413,6 +413,34 @@ typeof(name(0)) name(struct pt_regs *ctx) \ > > } \ > > static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) > > > > +#define ___bpf_build_param0(narg, x) > > +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x > > +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \ > > + ___bpf_build_param1(narg, args) > > +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \ > > + ___bpf_build_param2(narg, args) > > +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \ > > + ___bpf_build_param3(narg, args) > > +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \ > > + ___bpf_build_param4(narg, args) > > +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \ > > + ___bpf_build_param5(narg, args) > > +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \ > > + ___bpf_build_param6(narg, args) > > +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \ > > + ___bpf_build_param7(narg, args) > > +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \ > > + ___bpf_build_param8(narg, args) > > +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \ > > + ___bpf_build_param9(narg, args) > > +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \ > > + ___bpf_build_param10(narg, args) > > +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \ > > + ___bpf_build_param11(narg, args) > > took me some time to get why the [narg - 12] :) it makes sense, but > then I started wondering why not > > #define ___bpf_build_param12(narg, x, args...) > ___bpf_build_param11(narg, args); ___param[11] = x > > ? seems more straightforward, no? > > also please keep all of them on single line. And to make lines > shorter, let's call it ___bpf_fillX? I also don't like hard-coded > ___param, which is both inflexible and is obscure at the point of use > of this macro. So let's pass it as the first argument? > > > +#define ___bpf_build_param(args...) \ > > + unsigned long long ___param[___bpf_narg(args)]; \ > > + ___bpf_apply(___bpf_build_param, ___bpf_narg(args))(___bpf_narg(args), args) > > + > > And here I'd pass array as a parameter and let caller define it, so > macro is literally just filling the array elements, not defining the > array itself and what's the type of elements > > > /* > > * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values > > * in a structure. > > @@ -422,7 +450,7 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) > > _Pragma("GCC diagnostic push") \ > > _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ > > static const char ___fmt[] = fmt; \ > > - unsigned long long ___param[] = { args }; \ > > + ___bpf_build_param(args); \ > > _Pragma("GCC diagnostic pop") \ > > int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \ > > ___param, sizeof(___param)); \ > > here you are violating separation of variables and code, > ___bpf_build_param is defining a variable, then has code statements, > then you are declaring ___ret after the code. So please split ___ret > definition, > > > -- > > 2.30.1.766.gb4fecdf3b7-goog > >