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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6966EC19F2D for ; Wed, 3 Aug 2022 20:30:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238730AbiHCUaH (ORCPT ); Wed, 3 Aug 2022 16:30:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40428 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238714AbiHCUaE (ORCPT ); Wed, 3 Aug 2022 16:30:04 -0400 Received: from mail-qt1-x833.google.com (mail-qt1-x833.google.com [IPv6:2607:f8b0:4864:20::833]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 522055B79C for ; Wed, 3 Aug 2022 13:30:03 -0700 (PDT) Received: by mail-qt1-x833.google.com with SMTP id u12so13556824qtk.0 for ; Wed, 03 Aug 2022 13:30:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Dsz8IHMohhYZ2cZL5pWvVqmp+AxFsLxkDLJLSy2o2N4=; b=qhvgEU9aNbRaLz1LZz5Di83doxE96N37k+dzlmaEWg5j2FL+um8PmgRkdOPCt9li8w d6I9EofHmCIIm5a5qQKRxvYyHNjc/oSKXVrzBEYTWXJxPsC9SdRj1z3GYLR95V0+NkFw lgyrS8WvaiBPjACPGlGDABCkAa+ybGB/6Uv/er7mt+cMRI1NTghVCOUPKzd2udTCZ6Kc +Ti6RiS7W6hdxcjcgYGDJ/Ujgc9ms5B7xl43PRhb/E298i9nxm26sTuM2DhKkPlQ9DEg Q2NcVHoYtWH31WyzoWtwIiBrvvxg4ctBJQuILoR82lLcUyoK2mZZvbPldxUKnIwE1sE7 EVeg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Dsz8IHMohhYZ2cZL5pWvVqmp+AxFsLxkDLJLSy2o2N4=; b=fKJeCMbnEuX4HdW+hPzCjW0L/jFKD0TLPhGpcoa0qg1TvivSNCz2Qp+yNZvAVNj6za lsWfVLJa0LH6pBm7zYLMjfDGwmlZV3SmzvUXm1BGmuo1VDV6FGb1wVDbpruTgIvJXG52 dq3hG0bBAs7oMP76de5RZFVABJ5FaiWTlxiESZRko1u+oWuAgI3wvP6ERAYui8/4bYF/ RNGfJr5RngOemtdLDMnzvX79YlnTjiGdVRu4W5MtTUzN6y3ZEuRq422mo77uU3ar9Oi0 KorvyivHacO7JCYxg+kWTCbqZLpeEacxj5OJZSEXzix6Ul2KwArz68zgp9if1Ymo8oW6 2STA== X-Gm-Message-State: AJIora9kZMZb1AyKdkn4XHj8rI7DM+TArckWdvxn1bisiepIzKJctjoM HmXyhyB/pSRznuag+DachCKWA09SfSsoXgRZeYVQdTOD4UIYYsyy X-Google-Smtp-Source: AGRyM1ti9ieZjz4Bqd1rv5WpcT/IAZQJCQDgsCwKz5/2E/kmqsJiz+J94HVKuJdGxSnCmLHnF8Cz6vGIuC+uc33d2sw= X-Received: by 2002:a05:622a:8e:b0:31f:371f:e6a1 with SMTP id o14-20020a05622a008e00b0031f371fe6a1mr24197038qtw.565.1659558602311; Wed, 03 Aug 2022 13:30:02 -0700 (PDT) MIME-Version: 1.0 References: <20220722174829.3422466-1-yosryahmed@google.com> <20220722174829.3422466-5-yosryahmed@google.com> In-Reply-To: From: Hao Luo Date: Wed, 3 Aug 2022 13:29:51 -0700 Message-ID: Subject: Re: [PATCH bpf-next v5 4/8] bpf: Introduce cgroup iter To: Andrii Nakryiko Cc: Yosry Ahmed , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Martin KaFai Lau , Song Liu , Yonghong Song , Tejun Heo , Zefan Li , Johannes Weiner , Shuah Khan , Michal Hocko , KP Singh , Benjamin Tissoires , John Fastabend , =?UTF-8?Q?Michal_Koutn=C3=BD?= , Roman Gushchin , David Rientjes , Stanislav Fomichev , Greg Thelen , Shakeel Butt , linux-kernel@vger.kernel.org, netdev@vger.kernel.org, bpf@vger.kernel.org, cgroups@vger.kernel.org, Kui-Feng Lee Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Aug 2, 2022 at 3:50 PM Andrii Nakryiko wrote: > > On Tue, Aug 2, 2022 at 3:27 PM Hao Luo wrote: > > > > Hi Andrii, > > > > On Mon, Aug 1, 2022 at 8:43 PM Andrii Nakryiko > > wrote: > > > > > > On Fri, Jul 22, 2022 at 10:48 AM Yosry Ahmed wrote: [...] > > > > > > > > +enum bpf_iter_cgroup_traversal_order { > > > > + BPF_ITER_CGROUP_PRE = 0, /* pre-order traversal */ > > > > + BPF_ITER_CGROUP_POST, /* post-order traversal */ > > > > + BPF_ITER_CGROUP_PARENT_UP, /* traversal of ancestors up to the root */ > > > > > > I've just put up my arguments why it's a good idea to also support a > > > "trivial" mode of only traversing specified cgroup and no descendants > > > or parents. Please see [0]. > > > > cc Kui-Feng in this thread. > > > > Yeah, I think it's a good idea. It's useful when we only want to show > > a single object, which can be common. Going further, I think we may > > want to restructure bpf_iter to optimize for this case. > > > > > I think the same applies here, especially > > > considering that it seems like a good idea to support > > > task/task_vma/task_files iteration within a cgroup. > > > > I have reservations on these use cases. I don't see immediate use of > > iterating vma or files within a cgroup. Tasks within a cgroup? Maybe. > > :) > > > > iter/task was what I had in mind in the first place. But I can also > imagine tools utilizing iter/task_files for each process within a > cgroup, so given iter/{task, task_file, task_vma} share the same UAPI > and internals, I don't see why we'd restrict this to only iter/task. No problem. I was hoping we don't over-design the interface. IMHO keep it simple stupid. :) > [...] > > > > [1] https://lwn.net/Articles/902405/ > > > > > > > > Some more naming nits. I find BPF_ITER_CGROUP_PRE and > > > BPF_ITER_CGROUP_POST a bit confusing. Even internally in kernel we > > > have css_next_descendant_pre/css_next_descendant_post, so why not > > > reflect the fact that we are going to iterate descendants: > > > BPF_ITER_CGROUP_DESCENDANTS_{PRE,POST}. And now that we use > > > "descendants" terminology, PARENT_UP should be ANCESTORS. ANCESTORS_UP > > > probably is fine, but seems a bit redundant (unless we consider a > > > somewhat weird ANCESTORS_DOWN, where we find the furthest parent and > > > then descend through preceding parents until we reach specified > > > cgroup; seems a bit exotic). > > > > > > > BPF_ITER_CGROUP_DESCENDANTS_PRE is too verbose. If there is a > > possibility of merging rbtree and supporting walk order of rbtree > > iter, maybe the name here could be general, like > > BPF_ITER_DESCENDANTS_PRE, which seems better. > > it's not like you'll be typing this hundreds of type, so verboseness > doesn't seem to be too problematic, but sure, BPF_ITER_DESCENDANTS_PRE > is fine with me > > > > > > [0] https://lore.kernel.org/bpf/f92e20e9961963e20766e290ee6668edd4bacf06.camel@fb.com/T/#m5ce50632aa550dd87a99241efb168cbcde1ee98f > > > > > > > +}; > > > > + > > > > union bpf_iter_link_info { > > > > struct { > > > > __u32 map_fd; > > > > } map; > > > > + > > > > + /* cgroup_iter walks either the live descendants of a cgroup subtree, or the > > > > + * ancestors of a given cgroup. > > > > + */ > > > > + struct { > > > > + /* Cgroup file descriptor. This is root of the subtree if walking > > > > + * descendants; it's the starting cgroup if walking the ancestors. > > > > + * If it is left 0, the traversal starts from the default cgroup v2 > > > > + * root. For walking v1 hierarchy, one should always explicitly > > > > + * specify the cgroup_fd. > > > > + */ > > > > + __u32 cgroup_fd; > > > > > > Now, similar to what I argued in regard of pidfd vs pid, I think the > > > same applied to cgroup_fd vs cgroup_id. Why can't we support both? > > > cgroup_fd has some benefits, but cgroup_id is nice due to simplicity > > > and not having to open/close/keep extra FDs (which can add up if we > > > want to periodically query something about a large set of cgroups). > > > Please see my arguments from [0] above. > > > > > > Thoughts? > > > > > > > We can support both, it's a good idea IMO. But what exactly is the > > interface going to look like? Can you be more specific about that? > > Below is something I tried based on your description. > > > > @@ -91,6 +91,18 @@ union bpf_iter_link_info { > > struct { > > __u32 map_fd; > > } map; > > + struct { > > + /* PRE/POST/UP/SELF */ > > + __u32 order; > > + struct { > > + __u32 cgroup_fd; > > + __u64 cgroup_id; > > + } cgroup; > > + struct { > > + __u32 pid_fd; > > + __u64 pid; > > + } task; > > + }; > > }; > > > > So I wouldn't combine task and cgroup definition together, let's keep > them independent. > > then for cgroup we can do something like: > > struct { > __u32 order; > __u32 cgroup_fd; /* cgroup_fd ^ cgroup_id, exactly one can be non-zero */ > __u32 cgroup_id; > } cgroup > > Similar idea with task, but it's a bit more complicated because there > we have target that can be pid, pidfd, or cgroup (cgroup_fd and > cgroup_id). I haven't put much thought into the best representation, > though. > The cgroup part sounds good to me. For the full picture, how about this? I'm just trying a prototype, hoping that it can help people to get a clear picture. union bpf_iter_link_info { struct { __u32 map_fd; } map; struct { __u32 order; /* PRE/POST/UP/SELF */ __u32 cgroup_fd; __u64 cgroup_id; } cgroup; struct { __u32 pid; __u32 pid_fd; __u64 cgroup_id; __u32 cgroup_fd; __u32 mode; /* SELF or others */ } task; }; > > > > + __u32 traversal_order; > > > > + } cgroup; > > > > }; > > > > > > > > /* BPF syscall commands, see bpf(2) man-page for more details. */ > > > > > > [...]