Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
From:   Song Liu <songliubraving@meta.com>
To:     David Vernet <void@manifault.com>
CC:     bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
        "daniel@iogearbox.net" <daniel@iogearbox.net>,
        "andrii@kernel.org" <andrii@kernel.org>,
        "martin.lau@linux.dev" <martin.lau@linux.dev>,
        "song@kernel.org" <song@kernel.org>,
        "yonghong.song@linux.dev" <yonghong.song@linux.dev>,
        "john.fastabend@gmail.com" <john.fastabend@gmail.com>,
        "kpsingh@kernel.org" <kpsingh@kernel.org>,
        "sdf@google.com" <sdf@google.com>,
        "haoluo@google.com" <haoluo@google.com>,
        "jolsa@kernel.org" <jolsa@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Kernel Team <kernel-team@meta.com>,
        "himadrispandya@gmail.com" <himadrispandya@gmail.com>,
        "julia.lawall@inria.fr" <julia.lawall@inria.fr>
Subject: Re: [PATCH bpf-next 2/2] bpf/selftests: Test pinning bpf timer to a
 core
Thread-Topic: [PATCH bpf-next 2/2] bpf/selftests: Test pinning bpf timer to a
 core
Thread-Index: AQHZ9YrIU8YDncG/c0W9wNVK+zLFpLA4X+wA
Date:   Tue, 3 Oct 2023 18:15:03 +0000
Message-ID: <4EC94A6E-B3C5-4D2D-BD4B-FF7C4F149FD1@fb.com>
References: <20231002234708.331192-1-void@manifault.com>
 <20231002234708.331192-2-void@manifault.com>
In-Reply-To: <20231002234708.331192-2-void@manifault.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?uRqS614t0/ilg/T+6DdMe7qdM8Bo4xHVk8derY7z2W7ZsMfyMAhK3FBkQFpl?=
 =?us-ascii?Q?1Ui7zcO3AVm6R1aoIaNlyLvF0gy+d2v3ijLZUkknWvTRDmHgdl6vOL3iSe77?=
 =?us-ascii?Q?ESgSX2RRmW+09f/lyV+/JrLYeU+jrrtOOM4ipCWAvyQ97mEtq+qmDEac1vz1?=
 =?us-ascii?Q?8KldpBn5z+nU/uWpMbqmncuq3UwzJ+Q8UrG7T4nrjShXXlxYPTwM2ncOMui9?=
 =?us-ascii?Q?fnqIRpTZ3wWucxt+uQCS3MVSMSb7ObVzZo9OIDtnntlC4a643C8zTkmkof//?=
 =?us-ascii?Q?aJeyTgMmrUmrW3zz65EKhqTFV/ElFD60N01onP/VWU0+S55lPm2t3kNpTqcF?=
 =?us-ascii?Q?JuxdWn5/Y9yVXbudBiK7fSrIKSS0B4AyeJBNb9HzuGWVZ8Y9bteDsOB1ETd2?=
 =?us-ascii?Q?+ts4mtfHTXtAjE8RLtdnuwCzVHjprQ6tbXQiLSrTDeUsTYxLuRT2QmCWFwG3?=
 =?us-ascii?Q?FIQ3rukULN3Rk90caQQRQvgYYJpml0K3VYilDQnaIqDT3vsdonFLiCFb6Arc?=
 =?us-ascii?Q?jIm8VbHRI1C1lWhH4HO6FjQ6hj74HDMLpcheMgkBY5fSjgABfF/baskRZtnK?=
 =?us-ascii?Q?EFbXgw0O7uM9mKTjvTjsGhJA4HSS0aNRvLEZDlKXNbCcx4STEOOJYPFOtz8W?=
 =?us-ascii?Q?Z36RUofZZh4nB/Gqtr2vBxlIU3J8lBH5Xx/y2LvqAh2R6c96L0RLuifPbWQ1?=
 =?us-ascii?Q?agyCoU9UUQowtxzjymQVXv5kR0QAk2HgoSQz/TVPcdFrnvMMALnSqj8ouSt7?=
 =?us-ascii?Q?816SxPpHRZSrp8chu3/sJr6jb95uJorXjrr5Ad8MvofvGd2AeKRTS4Wb8BLs?=
 =?us-ascii?Q?8bgfCmxxxHE0k1iMG6f5nxu56accvaYKbtM8EX0lmDgXh5D0GwPdxxMkoaYy?=
 =?us-ascii?Q?hUK8BI+vPF9I5IVdsVUInq2Ef/Ta8uMcRFrzvNWmkz4GXGnzia97Xa4Vee5w?=
 =?us-ascii?Q?fixGMxMyQGxJsZJGpC8Ww4o0F4J8m5vYX78NsZOLgduEjdX3UQ0GzOd4muF1?=
 =?us-ascii?Q?gBjxKcOkRNKiwQ0lvBvFX2CHmPoMxTNd816M2985gG+K88yAJnmJswfhgu72?=
 =?us-ascii?Q?jdwvOMtCwhpNzhDguLtlc5y8b3WjSx40MolSlSRgSSgeCvla+iLcbrDlaWNF?=
 =?us-ascii?Q?2GF7W3eMLg7P5fA+E04fgXQckQONZnHjIm4qEFmC3KJz8AGidvyV9JAy7dq6?=
 =?us-ascii?Q?iEquC5gkIQL+WSkA7mnNoQwN3XqBqzvHVN6QnjXOnb9hNCnq9gzv6Zz1lo2B?=
 =?us-ascii?Q?CPSoneF3+lWPw2nSVMirkeYipHNuCRD1TDi7RB0MNINwTNVLwYQK6PQSrY1u?=
 =?us-ascii?Q?59mlmq8tUMwVUKzpd/n5CtAZJAlCwagzm91LtiWlNPDsEIoKnHqfD2OeL2uy?=
 =?us-ascii?Q?3EQmMWed87rXUHX/0CuPvnUQMBnbTncqSmJcKcQJdq6/tE+JqDCc7465GpKC?=
 =?us-ascii?Q?F1RWFKhR/xsE33NMCEo09S6NItPbsgYb/F/gL2vJPPq22gRtZhhSFNcjLLkK?=
 =?us-ascii?Q?8US+4kGJ+7FZ17Z3tRNfxMstbiTEydLEMVjBCDqjNTlp55kHiLSzoV9dx3c9?=
 =?us-ascii?Q?p5amwLIMx5h7LaFJ8OZdaPTlI10s+T1uRDYTFFWCo4cQfAXc/1On1UGnqiXa?=
 =?us-ascii?Q?3rMSMX3uBjLkWVyq9ooGHys=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <8875383DD23C924DAF1F7E210A93192C@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dd86de2f-51e7-4466-d3e3-08dbc43caa0c
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Oct 2023 18:15:03.2680
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 8UtTgANRyckHihbOTLlX8Z69yET6m8D/flmRxA27r+yQOnlSCnZwXxsTcdV7o0KdYuz9FWB9QsipzxRpUTK3Ww==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR15MB3844
X-Proofpoint-GUID: VkEbUC1F2-amcQR4S8JDiLU1yVw67IEV
X-Proofpoint-ORIG-GUID: VkEbUC1F2-amcQR4S8JDiLU1yVw67IEV
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.267,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-10-03_15,2023-10-02_01,2023-05-22_02
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Tue, 03 Oct 2023 11:15:22 -0700 (PDT)


> On Oct 2, 2023, at 4:47 PM, David Vernet <void@manifault.com> wrote:
> 
> Now that we support pinning a BPF timer to the current core, we should
> test it with some selftests. This patch adds two new testcases to the
> timer suite, which verifies that a BPF timer both with and without
> BPF_F_TIMER_ABS, can be pinned to the calling core with
> BPF_F_TIMER_CPU_PIN.
> 
> Signed-off-by: David Vernet <void@manifault.com>

Acked-by: Song Liu <song@kernel.org>

With one nit/question below. 

> ---
> .../testing/selftests/bpf/prog_tests/timer.c  |  4 +
> tools/testing/selftests/bpf/progs/timer.c     | 75 +++++++++++++++++++
> 2 files changed, 79 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/timer.c b/tools/testing/selftests/bpf/prog_tests/timer.c
> index 290c21dbe65a..d8bc838445ec 100644
> --- a/tools/testing/selftests/bpf/prog_tests/timer.c
> +++ b/tools/testing/selftests/bpf/prog_tests/timer.c
> @@ -14,6 +14,7 @@ static int timer(struct timer *timer_skel)
> 
> ASSERT_EQ(timer_skel->data->callback_check, 52, "callback_check1");
> ASSERT_EQ(timer_skel->data->callback2_check, 52, "callback2_check1");
> + ASSERT_EQ(timer_skel->bss->pinned_callback_check, 0, "pinned_callback_check1");
> 
> prog_fd = bpf_program__fd(timer_skel->progs.test1);
> err = bpf_prog_test_run_opts(prog_fd, &topts);
> @@ -32,6 +33,9 @@ static int timer(struct timer *timer_skel)
> /* check that timer_cb3() was executed twice */
> ASSERT_EQ(timer_skel->bss->abs_data, 12, "abs_data");
> 
> + /* check that timer_cb_pinned() was executed twice */
> + ASSERT_EQ(timer_skel->bss->pinned_callback_check, 2, "pinned_callback_check");
> +
> /* check that there were no errors in timer execution */
> ASSERT_EQ(timer_skel->bss->err, 0, "err");
> 
> diff --git a/tools/testing/selftests/bpf/progs/timer.c b/tools/testing/selftests/bpf/progs/timer.c
> index 9a16d95213e1..0112b9c038b4 100644
> --- a/tools/testing/selftests/bpf/progs/timer.c
> +++ b/tools/testing/selftests/bpf/progs/timer.c
> @@ -53,12 +53,28 @@ struct {
> __type(value, struct elem);
> } abs_timer SEC(".maps");
> 
> +struct {
> + __uint(type, BPF_MAP_TYPE_ARRAY);
> + __uint(max_entries, 1);
> + __type(key, int);
> + __type(value, struct elem);
> +} soft_timer_pinned SEC(".maps");
> +
> +struct {
> + __uint(type, BPF_MAP_TYPE_ARRAY);
> + __uint(max_entries, 1);
> + __type(key, int);
> + __type(value, struct elem);
> +} abs_timer_pinned SEC(".maps");

nit: I think we can also do something like the following, but I am not 
sure whether this style is not recommended. 

diff --git i/tools/testing/selftests/bpf/progs/timer.c w/tools/testing/selftests/bpf/progs/timer.c
index 9a16d95213e1..638eeebcd6c9 100644
--- i/tools/testing/selftests/bpf/progs/timer.c
+++ w/tools/testing/selftests/bpf/progs/timer.c
@@ -51,7 +51,7 @@ struct {
        __uint(max_entries, 1);
        __type(key, int);
        __type(value, struct elem);
-} abs_timer SEC(".maps");
+} abs_timer SEC(".maps"), soft_timer_pinned SEC(".maps"), abs_timer_pinned SEC(".maps");

 __u64 bss_data;
 __u64 abs_data;


[...]