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 23DD8C6FA86 for ; Mon, 19 Sep 2022 15:26:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229909AbiISP0A (ORCPT ); Mon, 19 Sep 2022 11:26:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55258 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229663AbiISPZd (ORCPT ); Mon, 19 Sep 2022 11:25:33 -0400 Received: from mail-wr1-x436.google.com (mail-wr1-x436.google.com [IPv6:2a00:1450:4864:20::436]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 88AE238692 for ; Mon, 19 Sep 2022 08:25:16 -0700 (PDT) Received: by mail-wr1-x436.google.com with SMTP id n10so17496822wrw.12 for ; Mon, 19 Sep 2022 08:25:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=n45E9SMZgH+mX1PWjYiZMymKuV6YbCkP9GERockComU=; b=dNQHSDyQ6K4k42wEqk+38fCzWID6a69lv0oPocPWguYAS6aIJMpef0hvGXvvNYWV4i mQdb48nMtIZvfL025qkNpYsITe82S6tqb3WAFEQSJy5cX3e9b5yb77psLPh8AYySCgoH CFCjoK0Mr7CtakzujfzCNpibES7Y00oOc4hH6KMCeNkomKqsc0pJVWJ/bHy6mgoE8B1L U/y/nnC9ZY3wOLmLFZVkhEtniaDFUKyeJUvJA3mhmmAItupbH83QsXSeGR8oXQqyaK9K ZlJYvBEGrc6gqchld+92Fv9EtuJY4UrKlyF6ZMLMcUb8NlOFwjWVUpPRESekg6kbm7ku LPJw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=n45E9SMZgH+mX1PWjYiZMymKuV6YbCkP9GERockComU=; b=FME7SgBN0LixGm370P9JJqF4XkqrTtdkZKnF5myzrx1bBh8wP8fEMWsEIHKTzsnCyb SsmDLcsvjJjKnYhbTYcwMddNpWtVGRj20ZbKHM4cQbZk4yfUuvvKZLKxjTlWsIdyHXPq XkNBxvzHpkJvntte+chzZu1nkFLv8MNFgEqe/fpErX6EebFiukY4Yv3c8bWnfOri1yg/ vqj1mqI1MggNM+7v04n+lRyZzALe5jglSsRWz6Y7RE5ziBT286r3bdbC5gcJIx7sEZ+i TbveCi6dF3NKeFzThVMx88OXRN3LfLWhg9gwafOHG+Fczce0TziGkkObl38Fv1IemC5y i7mQ== X-Gm-Message-State: ACrzQf3hDNJZ2maKoiYcuQy5oPASmEUfMmRpaO2gkojkEmzbvh29pRTq 5MsfyHyV8R7WxRIVBcvCghsXQBOYCu13n/HW9g3scw== X-Google-Smtp-Source: AMsMyM7uoYYkjuWNMuec+kN6ZoFWa20saTu272aPCAqIPbKRj31DaNi5SJEl/KC7+Mzk7mgFGaGwxJyqvxJLpNyQK7c= X-Received: by 2002:a5d:64e8:0:b0:22a:bbb0:fa with SMTP id g8-20020a5d64e8000000b0022abbb000famr11506842wri.372.1663601114664; Mon, 19 Sep 2022 08:25:14 -0700 (PDT) MIME-Version: 1.0 References: <20220826230639.1249436-1-yosryahmed@google.com> In-Reply-To: From: Yosry Ahmed Date: Mon, 19 Sep 2022 08:24:38 -0700 Message-ID: Subject: Re: [PATCH] selftests/bpf: simplify cgroup_hierarchical_stats selftest To: Andrii Nakryiko Cc: Hao Luo , Andrii Nakryiko , Alexei Starovoitov , Daniel Borkmann , Martin KaFai Lau , Mykola Lysenko , Song Liu , Yonghong Song , John Fastabend , KP Singh , Stanislav Fomichev , Jiri Olsa , bpf , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Sep 9, 2022 at 5:50 PM Andrii Nakryiko wrote: > > On Tue, Sep 6, 2022 at 2:35 PM Yosry Ahmed wrote: > > > > On Mon, Aug 29, 2022 at 6:50 PM Yosry Ahmed wrote: > > > > > > On Mon, Aug 29, 2022 at 6:42 PM Hao Luo wrote: > > > > > > > > On Mon, Aug 29, 2022 at 6:07 PM Yosry Ahmed wrote: > > > > > > > > > > On Mon, Aug 29, 2022 at 3:15 PM Yosry Ahmed wrote: > > > > > > > > > > > > On Mon, Aug 29, 2022 at 1:08 PM Hao Luo wrote: > > > > > > > > > > > > > > On Fri, Aug 26, 2022 at 4:06 PM Yosry Ahmed wrote: > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > -SEC("tp_btf/mm_vmscan_memcg_reclaim_begin") > > > > > > > > -int BPF_PROG(vmscan_start, int order, gfp_t gfp_flags) > > > > > > > > +SEC("fentry/cgroup_attach_task") > > > > > > > > > > > > > > Can we select an attachpoint that is more stable? It seems > > > > > > > 'cgroup_attach_task' is an internal helper function in cgroup, and its > > > > > > > signature can change. I'd prefer using those commonly used tracepoints > > > > > > > and EXPORT'ed functions. IMHO their interfaces are more stable. > > > > > > > > > > > > > > > > > > > Will try to find a more stable attach point. Thanks! > > > > > > > > > > Hey Hao, > > > > > > > > > > I couldn't find any suitable stable attach points under kernel/cgroup. > > > > > Most tracepoints are created using TRACE_CGROUP_PATH which only > > > > > invokes the tracepoint if the trace event is enabled, which I assume > > > > > is not something we can rely on. Otherwise, there is only > > > > > > > > Can we explicitly enable the cgroup_attach_task event, just for this > > > > test? If it's not easy, I am fine with using fentry. > > > > > > I see a couple of tests that read from /sys/kernel/debug/tracing, but > > > they are mostly reading event ids, I don't see any tests enabling or > > > disabling a tracing event, so I am not sure if that's an accepted > > > pattern. Also I am not sure if we can rely on tracefs being in that > > > path. Andrii, is this considered acceptable? > > > > > > > Anyone with thoughts here? Is it acceptable to explicitly enable a > > trace event in a BPF selftest to attach to a tracepoint that is only > > invoked if the trace event is enabled (e.g. cgroup_attach_task) ? > > Otherwise the test program would attach to the fentry of an internal > > function, which is more vulnerable to being changed and breaking the > > test (until someone updates the test with the new signature). > > > > IMO it's fine to use fentry. If something changes about signature, > we'll detect it soon enough and adjust selftests. > > Messing with global tracefs in selftests is less desirable. It will > also potentially force tests to be sequential. > Undestood. Thanks Andrii. Will send v2 with other comments from KP and Hao. > > > > > > > > > trace_cgroup_setup_root() and trace_cgroup_destroy_root() which are > > > > > irrelevant here. A lot of EXPORT'ed functions are not called in the > > > > > kernel, or cannot be invoked from userspace (the test) in a > > > > > straightforward way. Even if they did, future changes to such code > > > > > paths can also change in the future, so I don't think there is really > > > > > a way to guarantee that future changes don't break the test. > > > > > > > > > > Let me know what you think. > > > > >