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=-5.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no 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 373D5C433ED for ; Wed, 14 Apr 2021 22:51:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0CC446100C for ; Wed, 14 Apr 2021 22:51:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233740AbhDNWwH (ORCPT ); Wed, 14 Apr 2021 18:52:07 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:58639 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233098AbhDNWwG (ORCPT ); Wed, 14 Apr 2021 18:52:06 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1618440702; 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=dtB6Wp5AVxplW2Z84nTCGyemptbqZUOzp8eUITEWE1E=; b=cWLIng26C0iVjI00iK1+cXLsOGlmA9hfIwk9c+yJ+WV+ybBOzyqYmyyz859DaGfH81zBnv zryk3p7lT1JVsewcYZ8r8YgsvSpDvktA+MPTHMPpLMjnItUbxUt+O2QQYAWsm417Eod9Qv Im3xqK9+i+Qbby0PocXbdqxRnWpPBEo= Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-64-Ho5QoZtAOpqfzGIo15MkJg-1; Wed, 14 Apr 2021 18:51:41 -0400 X-MC-Unique: Ho5QoZtAOpqfzGIo15MkJg-1 Received: by mail-ej1-f71.google.com with SMTP id f15-20020a170906738fb029037c94426fffso312524ejl.22 for ; Wed, 14 Apr 2021 15:51:40 -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:content-transfer-encoding; bh=dtB6Wp5AVxplW2Z84nTCGyemptbqZUOzp8eUITEWE1E=; b=fFP5N6F13eFiTKCXwwv65OF4ItMwxNAATXVh7MtPIYKRisHgzMofs8f69fZJaWu2fD mNGXThsO6uGjj3ecVu084GkPs0+prjHrfhSM3f4VsSiwPAGyrn6skJDm8CeIaeLblilM Mr6wNs10ixmyXU43MSWr39o63sKxU/d3WlsrkDiLlzCf/3qmMlU7IdYmcFWVHl7SMxeV 2ztxLviLLUUwgG5uFYj7sb4K4yWJeFNtcwmH/6+eYJKReBoWPspSZ2+Zd0I1Hg1mDHOZ Od6iKEogZ8XZiUUuK9cmb/tC9iA2Sk5chxL0iQEDUoJXjovp8LP0l2Cuh4Qso8KM+SDY F1SQ== X-Gm-Message-State: AOAM531/uGEUuaT+4+Hi2okPcXgWYO2TCQY+VjpWxZCkeirVRWQW6QQP Ik57Vxd7BCWRMTbQc3HDp7lWiFaOjYuPDYEFudUX80jpC9UJhT2kWJ6E3zZRFx/OgdKsTR0vdm3 1+4HiEhT4EJAj X-Received: by 2002:a05:6402:270c:: with SMTP id y12mr597501edd.284.1618440699758; Wed, 14 Apr 2021 15:51:39 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz/Ha0YabHIRbKQeIodCU+bcTN5C7lPFVSR2jBVjzkZyW/0QqQvzy7nn33VIUTkoBF1km1Q8g== X-Received: by 2002:a05:6402:270c:: with SMTP id y12mr597457edd.284.1618440699361; Wed, 14 Apr 2021 15:51:39 -0700 (PDT) Received: from alrua-x1.borgediget.toke.dk ([2a0c:4d80:42:443::2]) by smtp.gmail.com with ESMTPSA id k9sm126025eje.102.2021.04.14.15.51.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Apr 2021 15:51:38 -0700 (PDT) Received: by alrua-x1.borgediget.toke.dk (Postfix, from userid 1000) id 087191806B3; Thu, 15 Apr 2021 00:51:38 +0200 (CEST) From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= To: Andrii Nakryiko Cc: Alexei Starovoitov , Kumar Kartikeya Dwivedi , Daniel Borkmann , bpf , Jesper Dangaard Brouer , Alexei Starovoitov , Andrii Nakryiko , Martin KaFai Lau , Song Liu , Yonghong Song , John Fastabend , KP Singh , Shuah Khan , "David S. Miller" , Jakub Kicinski , Jesper Dangaard Brouer , Peter Zijlstra , open list , Networking , "open list:KERNEL SELFTEST FRAMEWORK" Subject: Re: [PATCH bpf-next 3/5] libbpf: add low level TC-BPF API In-Reply-To: References: <20210325120020.236504-4-memxor@gmail.com> <20210328080648.oorx2no2j6zslejk@apollo> <48b99ccc-8ef6-4ba9-00f9-d7e71ae4fb5d@iogearbox.net> <20210331094400.ldznoctli6fljz64@apollo> <5d59b5ee-a21e-1860-e2e5-d03f89306fd8@iogearbox.net> <20210402152743.dbadpgcmrgjt4eca@apollo> <20210402190806.nhcgappm3iocvd3d@apollo> <20210403174721.vg4wle327wvossgl@ast-mbp> <87blar4ti7.fsf@toke.dk> <874kg9m8t1.fsf@toke.dk> X-Clacks-Overhead: GNU Terry Pratchett Date: Thu, 15 Apr 2021 00:51:37 +0200 Message-ID: <87wnt4jx8m.fsf@toke.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org Andrii Nakryiko writes: > On Wed, Apr 14, 2021 at 3:58 AM Toke H=C3=B8iland-J=C3=B8rgensen wrote: >> >> Andrii Nakryiko writes: >> >> > On Tue, Apr 6, 2021 at 3:06 AM Toke H=C3=B8iland-J=C3=B8rgensen wrote: >> >> >> >> Andrii Nakryiko writes: >> >> >> >> > On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov >> >> > wrote: >> >> >> >> >> >> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi = wrote: >> >> >> > On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: >> >> >> > > On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi wrote: >> >> >> > > > [...] >> >> >> > > >> >> >> > > All of these things are messy because of tc legacy. bpf tried = to follow tc style >> >> >> > > with cls and act distinction and it didn't quite work. cls with >> >> >> > > direct-action is the only >> >> >> > > thing that became mainstream while tc style attach wasn't real= ly addressed. >> >> >> > > There were several incidents where tc had tens of thousands of= progs attached >> >> >> > > because of this attach/query/index weirdness described above. >> >> >> > > I think the only way to address this properly is to introduce = bpf_link style of >> >> >> > > attaching to tc. Such bpf_link would support ingress/egress on= ly. >> >> >> > > direction-action will be implied. There won't be any index and= query >> >> >> > > will be obvious. >> >> >> > >> >> >> > Note that we already have bpf_link support working (without supp= ort for pinning >> >> >> > ofcourse) in a limited way. The ifindex, protocol, parent_id, pr= iority, handle, >> >> >> > chain_index tuple uniquely identifies a filter, so we stash this= in the bpf_link >> >> >> > and are able to operate on the exact filter during release. >> >> >> >> >> >> Except they're not unique. The library can stash them, but somethi= ng else >> >> >> doing detach via iproute2 or their own netlink calls will detach t= he prog. >> >> >> This other app can attach to the same spot a different prog and now >> >> >> bpf_link__destroy will be detaching somebody else prog. >> >> >> >> >> >> > > So I would like to propose to take this patch set a step furth= er from >> >> >> > > what Daniel said: >> >> >> > > int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): >> >> >> > > and make this proposed api to return FD. >> >> >> > > To detach from tc ingress/egress just close(fd). >> >> >> > >> >> >> > You mean adding an fd-based TC API to the kernel? >> >> >> >> >> >> yes. >> >> > >> >> > I'm totally for bpf_link-based TC attachment. >> >> > >> >> > But I think *also* having "legacy" netlink-based APIs will allow >> >> > applications to handle older kernels in a much nicer way without ex= tra >> >> > dependency on iproute2. We have a similar situation with kprobe, wh= ere >> >> > currently libbpf only supports "modern" fd-based attachment, but us= ers >> >> > periodically ask questions and struggle to figure out issues on old= er >> >> > kernels that don't support new APIs. >> >> >> >> +1; I am OK with adding a new bpf_link-based way to attach TC program= s, >> >> but we still need to support the netlink API in libbpf. >> >> >> >> > So I think we'd have to support legacy TC APIs, but I agree with >> >> > Alexei and Daniel that we should keep it to the simplest and most >> >> > straightforward API of supporting direction-action attachments and >> >> > setting up qdisc transparently (if I'm getting all the terminology >> >> > right, after reading Quentin's blog post). That coincidentally shou= ld >> >> > probably match how bpf_link-based TC API will look like, so all that >> >> > can be abstracted behind a single bpf_link__attach_tc() API as well, >> >> > right? That's the plan for dealing with kprobe right now, btw. Libb= pf >> >> > will detect the best available API and transparently fall back (may= be >> >> > with some warning for awareness, due to inherent downsides of legacy >> >> > APIs: no auto-cleanup being the most prominent one). >> >> >> >> Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the >> >> high-level API auto-detect. That way users can also still use the >> >> netlink attach function if they don't want the fd-based auto-close >> >> behaviour of bpf_link. >> > >> > So I thought a bit more about this, and it feels like the right move >> > would be to expose only higher-level TC BPF API behind bpf_link. It >> > will keep the API complexity and amount of APIs that libbpf will have >> > to support to the minimum, and will keep the API itself simple: >> > direct-attach with the minimum amount of input arguments. By not >> > exposing low-level APIs we also table the whole bpf_tc_cls_attach_id >> > design discussion, as we now can keep as much info as needed inside >> > bpf_link_tc (which will embed bpf_link internally as well) to support >> > detachment and possibly some additional querying, if needed. >> >> But then there would be no way for the caller to explicitly select a >> mechanism? I.e., if I write a BPF program using this mechanism targeting >> a 5.12 kernel, I'll get netlink attachment, which can stick around when >> I do bpf_link__disconnect(). But then if the kernel gets upgraded to >> support bpf_link for TC programs I'll suddenly transparently get >> bpf_link and the attachments will go away unless I pin them. This >> seems... less than ideal? > > That's what we are doing with bpf_program__attach_kprobe(), though. > And so far I've only seen people (privately) saying how good it would > be to have bpf_link-based TC APIs, doesn't seem like anyone with a > realistic use case prefers the current APIs. So I suspect it's not > going to be a problem in practice. But at least I'd start there and > see how people are using it and if they need anything else. *sigh* - I really wish you would stop arbitrarily declaring your own use cases "realistic" and mine (implied) "unrealistic". Makes it really hard to have a productive discussion... >> If we expose the low-level API I can elect to just use this if I know I >> want netlink behaviour, but if bpf_program__attach_tc() is the only API >> available it would at least need a flag to enforce one mode or the other >> (I can see someone wanting to enforce kernel bpf_link semantics as well, >> so a flag for either mode seems reasonable?). > > Sophisticated enough users can also do feature detection to know if > it's going to work or not. Sure, but that won't help if there's no API to pick the attach mode they want. > There are many ways to skin this cat. I'd prioritize bpf_link-based TC > APIs to be added with legacy TC API as a fallback. I'm fine with adding that; I just want the functions implementing the TC API to also be exported so users can use those if they prefer... -Toke