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=-11.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, 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 919FDC433B4 for ; Tue, 27 Apr 2021 18:00:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 67AE8613B2 for ; Tue, 27 Apr 2021 18:00:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236742AbhD0SBE (ORCPT ); Tue, 27 Apr 2021 14:01:04 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:23515 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235974AbhD0SBE (ORCPT ); Tue, 27 Apr 2021 14:01:04 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1619546420; 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: in-reply-to:in-reply-to:references:references; bh=CKrgALHVEgN6fGE15CxFWB5GcyFBxoUmnjcuKmaLHE0=; b=J51UlXfaeWQcuZB5u/16wUCY/GyqVOYbUSwh3191HhTIuU1psEbaPmwXymWLhk7xH7PuaF IJOwYat/FOWLiAnDhjHMj+x51ol4HCB06nhf4qhKhGZIjhEIm5SqSBYMtSCdEn40QG6/EL 58JyFjUFpRD9QnwQJlbEB5hTANpFo0U= Received: from mail-ej1-f70.google.com (mail-ej1-f70.google.com [209.85.218.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-294-sEHiG_6EO-WYfPqXcdhVeQ-1; Tue, 27 Apr 2021 14:00:18 -0400 X-MC-Unique: sEHiG_6EO-WYfPqXcdhVeQ-1 Received: by mail-ej1-f70.google.com with SMTP id gb17-20020a1709079611b029038c058a504cso2409814ejc.7 for ; Tue, 27 Apr 2021 11:00:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date :message-id:mime-version; bh=CKrgALHVEgN6fGE15CxFWB5GcyFBxoUmnjcuKmaLHE0=; b=fHuKzoWYIvRq1I7xbTIQAtZQ+gRe6getqBj7w5QqrfI2JjxJGCCXGxXVaPC+d/3f0N +j8T6l2rbnztoxOU5r4gHbNqBXmlZ0jzlC6zVbTsSyukYj6OO0P49wIwE7KcHEWzz2mx iPh4aJoyzy+q51MLEgL3x+ZX04pkq9RxDGq+TLBJjcSa1YZWLlMoub4y/QH58isvnVYZ L+cywyeuo2DPPsk5TF3dAo+FuxIgI521PiAGZA7K3mBqCaj0QLfKiFG458h60ssYXpyA 1XqQ/Nfco7K8igwl1EBgOb479HFtugY7J6DO3co5ktl/WQJW4TC1LcT3jCVBMVFO6EDK MhXw== X-Gm-Message-State: AOAM531+fxukbnQpTpDU9/eUMCT8ivCvCH19NQjIMThqGvAm/gceDNK4 DNu6p2QkFAlX8w/2GWyVilsvU7zzeCvKduO7TXQtspQjCcyHERBS+Vf+Hfljem7PR/ZvbQL7O9p f+B/+wMCT8wOW X-Received: by 2002:a17:906:fa18:: with SMTP id lo24mr24423748ejb.125.1619546416467; Tue, 27 Apr 2021 11:00:16 -0700 (PDT) X-Google-Smtp-Source: ABdhPJw5XjcnUui/mbb5+o9DehwY79r16Nweo03ctwk0YzOtas7cEKznjeo1TSrS6P8rOWZX60FoYQ== X-Received: by 2002:a17:906:fa18:: with SMTP id lo24mr24423716ejb.125.1619546416039; Tue, 27 Apr 2021 11:00:16 -0700 (PDT) Received: from alrua-x1.borgediget.toke.dk ([2a0c:4d80:42:443::2]) by smtp.gmail.com with ESMTPSA id t15sm2806595edr.55.2021.04.27.11.00.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 27 Apr 2021 11:00:15 -0700 (PDT) Received: by alrua-x1.borgediget.toke.dk (Postfix, from userid 1000) id B372B180615; Tue, 27 Apr 2021 20:00:14 +0200 (CEST) From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= To: Daniel Borkmann , Kumar Kartikeya Dwivedi , bpf@vger.kernel.org Cc: Alexei Starovoitov , Andrii Nakryiko , Martin KaFai Lau , Song Liu , Yonghong Song , John Fastabend , KP Singh , "David S. Miller" , Jakub Kicinski , Jesper Dangaard Brouer , netdev@vger.kernel.org Subject: Re: [PATCH bpf-next v4 2/3] libbpf: add low level TC-BPF API In-Reply-To: <5811eb10-bc93-0b81-2ee4-10490388f238@iogearbox.net> References: <20210423150600.498490-1-memxor@gmail.com> <20210423150600.498490-3-memxor@gmail.com> <5811eb10-bc93-0b81-2ee4-10490388f238@iogearbox.net> X-Clacks-Overhead: GNU Terry Pratchett Date: Tue, 27 Apr 2021 20:00:14 +0200 Message-ID: <87lf93a9qp.fsf@toke.dk> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org Daniel Borkmann writes: > On 4/23/21 5:05 PM, Kumar Kartikeya Dwivedi wrote: > [...] >> tools/lib/bpf/libbpf.h | 92 ++++++++ >> tools/lib/bpf/libbpf.map | 5 + >> tools/lib/bpf/netlink.c | 478 ++++++++++++++++++++++++++++++++++++++- >> 3 files changed, 574 insertions(+), 1 deletion(-) >> >> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h >> index bec4e6a6e31d..1c717c07b66e 100644 >> --- a/tools/lib/bpf/libbpf.h >> +++ b/tools/lib/bpf/libbpf.h >> @@ -775,6 +775,98 @@ LIBBPF_API int bpf_linker__add_file(struct bpf_linker *linker, const char *filen >> LIBBPF_API int bpf_linker__finalize(struct bpf_linker *linker); >> LIBBPF_API void bpf_linker__free(struct bpf_linker *linker); >> >> +enum bpf_tc_attach_point { >> + BPF_TC_INGRESS, >> + BPF_TC_EGRESS, >> + BPF_TC_CUSTOM_PARENT, >> + _BPF_TC_PARENT_MAX, > > I don't think we need to expose _BPF_TC_PARENT_MAX as part of the API, I would drop > the latter. > >> +}; >> + >> +/* The opts structure is also used to return the created filters attributes >> + * (e.g. in case the user left them unset). Some of the options that were left >> + * out default to a reasonable value, documented below. >> + * >> + * protocol - ETH_P_ALL >> + * chain index - 0 >> + * class_id - 0 (can be set by bpf program using skb->tc_classid) >> + * bpf_flags - TCA_BPF_FLAG_ACT_DIRECT (direct action mode) >> + * bpf_flags_gen - 0 >> + * >> + * The user must fulfill documented requirements for each function. > > Not sure if this is overly relevant as part of the bpf_tc_opts in here. For the > 2nd part, I would probably just mention that libbpf internally attaches the bpf > programs with direct action mode. The hw offload may be future todo, and the other > bits are little used anyway; mentioning them here, what value does it have to > libbpf users? I'd rather just drop the 2nd part and/or simplify this paragraph > just stating that the progs are attached in direct action mode. The idea was that this would be for the benefit of people familiar with TC concepts. Maybe simplify it to "Programs are attached in direct action mode with a protocol value of 'all', and all other parameters that the 'tc' binary supports will be set to 0"? >> + */ >> +struct bpf_tc_opts { >> + size_t sz; >> + __u32 handle; >> + __u32 parent; >> + __u16 priority; >> + __u32 prog_id; >> + bool replace; >> + size_t :0; >> +}; >> + >> +#define bpf_tc_opts__last_field replace >> + >> +struct bpf_tc_ctx; >> + >> +struct bpf_tc_ctx_opts { >> + size_t sz; >> +}; >> + >> +#define bpf_tc_ctx_opts__last_field sz >> + >> +/* Requirements */ >> +/* >> + * @ifindex: Must be > 0. >> + * @parent: Must be one of the enum constants < _BPF_TC_PARENT_MAX >> + * @opts: Can be NULL, currently no options are supported. >> + */ > > Up to Andrii, but we don't have such API doc in general inside libbpf.h, I > would drop it for the time being to be consistent with the rest (same for > others below). > >> +LIBBPF_API struct bpf_tc_ctx *bpf_tc_ctx_init(__u32 ifindex, > > nit: in user space s/__u32 ifindex/int ifindex/ > >> + enum bpf_tc_attach_point parent, >> + struct bpf_tc_ctx_opts *opts); > > Should we enforce opts being NULL or non-NULL here, or drop the arg from here > for now altogether? (And if later versions of the functions show up this could > be mapped to the right one?) Hmm, extending later is easier if there's already an opts parameter. But it could be declared 'void *' and enforced as always 0 for now? >> +/* >> + * @ctx: Can be NULL, if not, must point to a valid object. >> + * If the qdisc was attached during ctx_init, it will be deleted if no >> + * filters are attached to it. >> + * When ctx == NULL, this is a no-op. >> + */ >> +LIBBPF_API int bpf_tc_ctx_destroy(struct bpf_tc_ctx *ctx); >> +/* >> + * @ctx: Cannot be NULL. >> + * @fd: Must be >= 0. >> + * @opts: Cannot be NULL, prog_id must be unset, all other fields can be >> + * optionally set. All fields except replace will be set as per created >> + * filter's attributes. parent must only be set when attach_point of ctx is >> + * BPF_TC_CUSTOM_PARENT, otherwise parent must be unset. >> + * >> + * Fills the following fields in opts: >> + * handle >> + * parent >> + * priority >> + * prog_id >> + */ >> +LIBBPF_API int bpf_tc_attach(struct bpf_tc_ctx *ctx, int fd, >> + struct bpf_tc_opts *opts); >> +/* >> + * @ctx: Cannot be NULL. >> + * @opts: Cannot be NULL, replace and prog_id must be unset, all other fields >> + * must be set. >> + */ >> +LIBBPF_API int bpf_tc_detach(struct bpf_tc_ctx *ctx, >> + const struct bpf_tc_opts *opts); > > One thing that I find a bit odd from this API is that BPF_TC_INGRESS / BPF_TC_EGRESS > needs to be set each time via bpf_tc_ctx_init(). So whenever a specific program would > be attached to both we need to 're-init' in between just to change from hook a to b, > whereas when you have BPF_TC_CUSTOM_PARENT, you could just use a different opts->parent > without going this detour (unless the clsact wasn't loaded there in the first place). > > Could we add a BPF_TC_UNSPEC to enum bpf_tc_attach_point, which the user would pass to > bpf_tc_ctx_init(), so that opts.direction = BPF_TC_INGRESS with subsequent bpf_tc_attach() > can be called, and same opts.direction = BPF_TC_EGRESS with bpf_tc_attach() for different > fd. The only thing we cared about in bpf_tc_ctx_init() resp. the ctx was that qdisc was > ready. This sounds workable - no objections from me :) -Toke