Message ID | 20240221175210.19936-2-khuey@kylehuey.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-75229-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:693c:2685:b0:108:e6aa:91d0 with SMTP id mn5csp1196743dyc; Wed, 21 Feb 2024 09:52:43 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCWEp1ti+s9PJ+4w780k9JIsliR6DkH+cv3IBLaPSYv30IGryayxLMDSZrurPzuo0yQke9+XMQ1ghVsYzQgEM4+XgXyYhw== X-Google-Smtp-Source: AGHT+IGmyfApQzmoOpWSEpOaFgRsdnmuPpmEo8foP5rDksmKYpNdtdCzl10Do7cmNHyNnu+G97QN X-Received: by 2002:a05:6a20:c890:b0:19b:a07a:344d with SMTP id hb16-20020a056a20c89000b0019ba07a344dmr18731586pzb.7.1708537963039; Wed, 21 Feb 2024 09:52:43 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1708537963; cv=pass; d=google.com; s=arc-20160816; b=IDEqpcSen6Oi/rMu4/wPM6lrNrSqCnuyLI/tZaQ7AcB4kXCTRo44NaxJtCWYx3bpWm +6Bi5tv8QX0Rg1M+B4M/pZcyhgbjaaO1wAf1ylAYiVnwJ1pNcF5tJRLDkPIzRCtN8gdj MYoZ5x970bykqdGk077jTJ7HWX4aQXpTzHJTa4FoMwwIVTTsVeBHkzTYCW1+NsAAqSMs NtmJ81rxHVqGMqnvYE1SAOLJ60ZQzFxZ/dVsUc15HDUN8Es2Op+/JM7DW6XNrbeIazkz c2s0Jnkk6igwndrjMQ4hA8zpE+ZhR1kOgYbWOvbwAiq5+2Jht4m0Chw6azjrSgW8AR5l T3pQ== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=q+DfUwTrji+JiliVaosYGzYtaY7yoOzYOgDQgUlYBFM=; fh=TmQqHmE1vlduyf9uTAeMjkdyP2bpSn6Oi89MADrlaS0=; b=PCaVJIUu3Yc3pbE5j7L1WVfAWBsxKMBWwlSrLJase3yN9nBZt+H2gQkv7jc87EGYA4 tx6Ir4DILNkqoUelkv68qxsLwBIdaz5AtNrEOk85azh93qWjnnFgtX+jnlUsi+RLCiTr gr9wg1QspvHKaYrDZTYiHzHKrEqBz5CWEvkp4SoqDOVPLh781REbiFGX/ssNqnYSwnVN mYi2w04bLWgQ/Iz8Ettn0aFU8vC2Dl7KETQ/5t4ac2WXH2f9RnixPblOtORxkOaZ8bC0 XD4tn0m++uEq9x+Vbtit2demMMZoAQ9nMOlz98fR+SNfT1GrxZoqWqyb5Bis5bcVyMLp umNA==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@kylehuey.com header.s=google header.b=bhT4zPJ1; arc=pass (i=1 spf=pass spfdomain=kylehuey.com dkim=pass dkdomain=kylehuey.com); spf=pass (google.com: domain of linux-kernel+bounces-75229-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-75229-ouuuleilei=gmail.com@vger.kernel.org" Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [139.178.88.99]) by mx.google.com with ESMTPS id h63-20020a638342000000b005d8b313de29si8460484pge.650.2024.02.21.09.52.42 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Feb 2024 09:52:43 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-75229-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99; Authentication-Results: mx.google.com; dkim=pass header.i=@kylehuey.com header.s=google header.b=bhT4zPJ1; arc=pass (i=1 spf=pass spfdomain=kylehuey.com dkim=pass dkdomain=kylehuey.com); spf=pass (google.com: domain of linux-kernel+bounces-75229-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-75229-ouuuleilei=gmail.com@vger.kernel.org" Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id D27A6285843 for <ouuuleilei@gmail.com>; Wed, 21 Feb 2024 17:52:42 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 9A46085291; Wed, 21 Feb 2024 17:52:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kylehuey.com header.i=@kylehuey.com header.b="bhT4zPJ1" Received: from mail-pl1-f177.google.com (mail-pl1-f177.google.com [209.85.214.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 020F384052 for <linux-kernel@vger.kernel.org>; Wed, 21 Feb 2024 17:52:14 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.177 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708537936; cv=none; b=VIKD0hP9jvh0Bgt6aXXRITkkIDsqEER+8yZ7tGk5QAE51njUCxa7Engly8TxCNQ4F783z6To6mE34/bAug78O15FMYWnuNRYClymmclqTi/YhfWRHRiMlnnJk638ULmrNDWQrny1iCCpkisjCo3yK8kTDkfbqUrWlDos1JjTd2I= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708537936; c=relaxed/simple; bh=tZ7ZR1nkA/EFD6K9KNCEcHnkRImuMCfdTQ5YwENITDw=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=ZrDhAkr6gBi6UUm6iVk7wQU+3zuf+VHEdBNQD14ryp8cWl86VmReNf3sCNp5NN6wtSa7ClUSh5yt/6Xc4oFj/sPWjeVgEuDzZdbV8My7eT3Bc/GfbQ0URMQ1+ziMoCjwX8sZ+lvKQmw1ZemZb2gfbU+o6qWzFTwvjNv3nEdOQKw= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=kylehuey.com; spf=pass smtp.mailfrom=kylehuey.com; dkim=pass (2048-bit key) header.d=kylehuey.com header.i=@kylehuey.com header.b=bhT4zPJ1; arc=none smtp.client-ip=209.85.214.177 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=kylehuey.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=kylehuey.com Received: by mail-pl1-f177.google.com with SMTP id d9443c01a7336-1d932f6ccfaso58227545ad.1 for <linux-kernel@vger.kernel.org>; Wed, 21 Feb 2024 09:52:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kylehuey.com; s=google; t=1708537934; x=1709142734; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=q+DfUwTrji+JiliVaosYGzYtaY7yoOzYOgDQgUlYBFM=; b=bhT4zPJ1KFUJzAlSHukomfjbC+A4tFZLm0WucBPFXI8g90T5L5ygrojSJFuKyIoqqn 6Z7AiYfFfnSgkqAnBEZf+NM4S/VJQFoF4kYGhEXXI2/vDSK+97OEjc1bJTGgS8d4m5C3 LMeMTQxLhRLS6wmfU1PHtu5ssuiZK79ry82hMRaAVUeZd2txLiIIfTpBJJCd/zbYa0bz JF03aUHrCe4RvT1gYpKB04Juj1WKE9dGXW1Wo3QwhWIJysbOwuV8hVWcEkEmNtLuWTE2 m/09lswBxDRTCX8EMWs8wm5olDvSs/eOzCAV7ZQEslXJpC3uZqaQihhDeXEN4KeJ4gk/ 0eNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708537934; x=1709142734; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=q+DfUwTrji+JiliVaosYGzYtaY7yoOzYOgDQgUlYBFM=; b=wzhH+7NCWK7gv7WRXRiFp2smGn4atbCyoY9HRNHEEzPpcsiJrsZYpzSEKGnWKHxWuD zpmyyMvVxq/xEvcpkBV7CFKwAV8/XeC8WC6W2P5SJK2R7UvXBXsehX7k8iV8c/cebefA K8FR4VgRwtBcPKo7zeTb2QXiWk+Ef41bp355RD2mLeT7Ijazn2eldwRnDofONCNlPwyh NPVh6COEQ4nUqw6KcoHBj6WHuXwTlXEZd6fe0ntjSrEXOGBO1vD0P793oA7yJXFLVkqs TGq9QjVXFZSU86/yBWHaqIKMtjipCzqEAJxv5s0ZMA4ltoQowFaIfVXlfd9SoCKI0vMV 6sig== X-Forwarded-Encrypted: i=1; AJvYcCW9I/93xdFnrPhteUe1Mmcea6Ie089SQ62AMINSb+r6NV5taNsNpQzHHpKMQ5pn/MS70LMr6aYeT0hhSI4JKyU0kXOfgIVmH+bz81Fk X-Gm-Message-State: AOJu0YxwrX+jIFWG7LphoytD/VMGexYKhPBIww9VnXn3UkoEIPwJeC0f RI/blqyOnTmG242Ea3hRVuKp3e2KLOgVxXkEUtvJAy7KmwMuujk5rWFZFUT5qg== X-Received: by 2002:a17:902:eccf:b0:1db:f513:28cf with SMTP id a15-20020a170902eccf00b001dbf51328cfmr11635532plh.23.1708537934363; Wed, 21 Feb 2024 09:52:14 -0800 (PST) Received: from zhadum.home.kylehuey.com (c-76-126-33-191.hsd1.ca.comcast.net. [76.126.33.191]) by smtp.gmail.com with ESMTPSA id 6-20020a170902e9c600b001d706e373a9sm8312596plk.292.2024.02.21.09.52.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Feb 2024 09:52:13 -0800 (PST) From: Kyle Huey <me@kylehuey.com> X-Google-Original-From: Kyle Huey <khuey@kylehuey.com> To: Kyle Huey <khuey@kylehuey.com>, Robert O'Callahan <robert@ocallahan.org>, Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, Namhyung Kim <namhyung@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com>, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>, Adrian Hunter <adrian.hunter@intel.com>, linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH 2/2] perf test: Test FASYNC with watermark wakeups. Date: Wed, 21 Feb 2024 09:52:10 -0800 Message-Id: <20240221175210.19936-2-khuey@kylehuey.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20240221175210.19936-1-khuey@kylehuey.com> References: <20240221175210.19936-1-khuey@kylehuey.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1791531903513024356 X-GMAIL-MSGID: 1791531903513024356 |
Series |
[1/2] perf/ring_buffer: Trigger FASYNC signals for watermark wakeups
|
|
Commit Message
Kyle Huey
Feb. 21, 2024, 5:52 p.m. UTC
The test uses PERF_RECORD_SWITCH records to fill the ring buffer and
trigger the watermark wakeup, which in turn should trigger an IO
signal.
Signed-off-by: Kyle Huey <khuey@kylehuey.com>
---
tools/perf/tests/Build | 1 +
tools/perf/tests/builtin-test.c | 1 +
tools/perf/tests/tests.h | 1 +
tools/perf/tests/watermark_signal.c | 123 ++++++++++++++++++++++++++++
4 files changed, 126 insertions(+)
create mode 100644 tools/perf/tests/watermark_signal.c
Comments
On Wed, Feb 21, 2024 at 9:52 AM Kyle Huey <me@kylehuey.com> wrote: > > The test uses PERF_RECORD_SWITCH records to fill the ring buffer and > trigger the watermark wakeup, which in turn should trigger an IO > signal. > > Signed-off-by: Kyle Huey <khuey@kylehuey.com> > --- > tools/perf/tests/Build | 1 + > tools/perf/tests/builtin-test.c | 1 + > tools/perf/tests/tests.h | 1 + > tools/perf/tests/watermark_signal.c | 123 ++++++++++++++++++++++++++++ > 4 files changed, 126 insertions(+) > create mode 100644 tools/perf/tests/watermark_signal.c > > diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build > index 53ba9c3e20e0..de43eb60b280 100644 > --- a/tools/perf/tests/Build > +++ b/tools/perf/tests/Build > @@ -67,6 +67,7 @@ perf-y += sigtrap.o > perf-y += event_groups.o > perf-y += symbols.o > perf-y += util.o > +perf-y += watermark_signal.o > > ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 arm arm64 powerpc)) > perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c > index 4a5973f9bb9b..715c01a2172a 100644 > --- a/tools/perf/tests/builtin-test.c > +++ b/tools/perf/tests/builtin-test.c > @@ -124,6 +124,7 @@ static struct test_suite *generic_tests[] = { > &suite__event_groups, > &suite__symbols, > &suite__util, > + &suite__watermark_signal, > NULL, > }; > > diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h > index dad3d7414142..7ef4e0d0a77b 100644 > --- a/tools/perf/tests/tests.h > +++ b/tools/perf/tests/tests.h > @@ -146,6 +146,7 @@ DECLARE_SUITE(sigtrap); > DECLARE_SUITE(event_groups); > DECLARE_SUITE(symbols); > DECLARE_SUITE(util); > +DECLARE_SUITE(watermark_signal); > > /* > * PowerPC and S390 do not support creation of instruction breakpoints using the > diff --git a/tools/perf/tests/watermark_signal.c b/tools/perf/tests/watermark_signal.c > new file mode 100644 > index 000000000000..ae4abedc4b7c > --- /dev/null > +++ b/tools/perf/tests/watermark_signal.c > @@ -0,0 +1,123 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <stddef.h> > +#include <signal.h> > +#include <stdlib.h> > +#include <string.h> > +#include <sys/ioctl.h> > +#include <sys/mman.h> > +#include <sys/wait.h> > +#include <unistd.h> > +#include <errno.h> > +#include <fcntl.h> > + > +#include "tests.h" > +#include "debug.h" > +#include "event.h" > +#include "../perf-sys.h" > +#include "cloexec.h" > +#include <internal/lib.h> // page_size > + > +static int sigio_count; > + > +static void handle_sigio(int sig __always_unused) > +{ > + ++sigio_count; > +} > + > +static void do_child(void) > +{ > + for (int i = 0; i < 20; ++i) > + sleep(1); > + > + exit(0); > +} > + > +static int test__watermark_signal(struct test_suite *test __maybe_unused, int subtest __maybe_unused) > +{ > + struct perf_event_attr attr; > + struct perf_event_mmap_page *p = NULL; > + sighandler_t previous_sigio = SIG_ERR; > + pid_t child = -1; > + int fd = -1; > + int ret = TEST_FAIL; > + > + previous_sigio = signal(SIGIO, handle_sigio); > + if (previous_sigio == SIG_ERR) { > + pr_debug("failed setting SIGIO handler\n"); > + goto cleanup; > + } > + > + memset(&attr, 0, sizeof(attr)); > + attr.size = sizeof(attr); > + attr.type = PERF_TYPE_SOFTWARE; > + attr.config = PERF_COUNT_SW_DUMMY; > + attr.sample_period = 1; > + attr.disabled = 1; > + attr.watermark = 1; > + attr.context_switch = 1; > + attr.wakeup_watermark = 1; > + > + child = fork(); > + if (child == 0) > + do_child(); > + else if (child < 0) { > + pr_debug("failed fork() %d\n", errno); > + goto cleanup; > + } > + > + fd = sys_perf_event_open(&attr, child, -1, -1, > + perf_event_open_cloexec_flag()); > + if (fd < 0) { > + pr_debug("failed opening event %llx\n", attr.config); > + goto cleanup; > + } > + > + if (fcntl(fd, F_SETFL, FASYNC)) { > + pr_debug("failed F_SETFL FASYNC %d\n", errno); > + goto cleanup; > + } Thanks for the work! The perf tool and perf test should run on older kernels ideally without failure. I'm assuming this would fail on an older kernel. Could we make the return value skip in that case? > + > + if (fcntl(fd, F_SETOWN, getpid())) { > + pr_debug("failed F_SETOWN getpid() %d\n", errno); > + goto cleanup; > + } > + > + if (fcntl(fd, F_SETSIG, SIGIO)) { > + pr_debug("failed F_SETSIG SIGIO %d\n", errno); > + goto cleanup; > + } > + > + p = mmap(NULL, 2 * page_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); > + if (p == NULL) { > + pr_debug("failed to mmap\n"); > + goto cleanup; > + } > + > + if (ioctl(fd, PERF_EVENT_IOC_ENABLE, 0)) { > + pr_debug("failed PERF_EVENT_IOC_ENABLE %d\n", errno); > + goto cleanup; > + } > + > + sleep(30); This is kind of a painful wait, could it be less? No sleeps would be best :-) Thanks, Ian > + > + ret = (sigio_count == 1) ? TEST_OK : TEST_FAIL; > + > +cleanup: > + if (p != NULL) > + munmap(p, 2 * page_size); > + > + if (fd >= 0) > + close(fd); > + > + if (child > 0) { > + kill(child, SIGTERM); > + waitpid(child, NULL, 0); > + } > + > + if (previous_sigio != SIG_ERR) > + signal(SIGIO, previous_sigio); > + > + return ret; > +} > + > +DEFINE_SUITE("Watermark signal handler", watermark_signal); > -- > 2.34.1 >
On Wed, Feb 21, 2024 at 10:36 AM Ian Rogers <irogers@google.com> wrote: > > On Wed, Feb 21, 2024 at 9:52 AM Kyle Huey <me@kylehuey.com> wrote: > > > > The test uses PERF_RECORD_SWITCH records to fill the ring buffer and > > trigger the watermark wakeup, which in turn should trigger an IO > > signal. > > > > Signed-off-by: Kyle Huey <khuey@kylehuey.com> > > --- > > tools/perf/tests/Build | 1 + > > tools/perf/tests/builtin-test.c | 1 + > > tools/perf/tests/tests.h | 1 + > > tools/perf/tests/watermark_signal.c | 123 ++++++++++++++++++++++++++++ > > 4 files changed, 126 insertions(+) > > create mode 100644 tools/perf/tests/watermark_signal.c > > > > diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build > > index 53ba9c3e20e0..de43eb60b280 100644 > > --- a/tools/perf/tests/Build > > +++ b/tools/perf/tests/Build > > @@ -67,6 +67,7 @@ perf-y += sigtrap.o > > perf-y += event_groups.o > > perf-y += symbols.o > > perf-y += util.o > > +perf-y += watermark_signal.o > > > > ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 arm arm64 powerpc)) > > perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o > > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c > > index 4a5973f9bb9b..715c01a2172a 100644 > > --- a/tools/perf/tests/builtin-test.c > > +++ b/tools/perf/tests/builtin-test.c > > @@ -124,6 +124,7 @@ static struct test_suite *generic_tests[] = { > > &suite__event_groups, > > &suite__symbols, > > &suite__util, > > + &suite__watermark_signal, > > NULL, > > }; > > > > diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h > > index dad3d7414142..7ef4e0d0a77b 100644 > > --- a/tools/perf/tests/tests.h > > +++ b/tools/perf/tests/tests.h > > @@ -146,6 +146,7 @@ DECLARE_SUITE(sigtrap); > > DECLARE_SUITE(event_groups); > > DECLARE_SUITE(symbols); > > DECLARE_SUITE(util); > > +DECLARE_SUITE(watermark_signal); > > > > /* > > * PowerPC and S390 do not support creation of instruction breakpoints using the > > diff --git a/tools/perf/tests/watermark_signal.c b/tools/perf/tests/watermark_signal.c > > new file mode 100644 > > index 000000000000..ae4abedc4b7c > > --- /dev/null > > +++ b/tools/perf/tests/watermark_signal.c > > @@ -0,0 +1,123 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +#include <stddef.h> > > +#include <signal.h> > > +#include <stdlib.h> > > +#include <string.h> > > +#include <sys/ioctl.h> > > +#include <sys/mman.h> > > +#include <sys/wait.h> > > +#include <unistd.h> > > +#include <errno.h> > > +#include <fcntl.h> > > + > > +#include "tests.h" > > +#include "debug.h" > > +#include "event.h" > > +#include "../perf-sys.h" > > +#include "cloexec.h" > > +#include <internal/lib.h> // page_size > > + > > +static int sigio_count; > > + > > +static void handle_sigio(int sig __always_unused) > > +{ > > + ++sigio_count; > > +} > > + > > +static void do_child(void) > > +{ > > + for (int i = 0; i < 20; ++i) > > + sleep(1); > > + > > + exit(0); > > +} > > + > > +static int test__watermark_signal(struct test_suite *test __maybe_unused, int subtest __maybe_unused) > > +{ > > + struct perf_event_attr attr; > > + struct perf_event_mmap_page *p = NULL; > > + sighandler_t previous_sigio = SIG_ERR; > > + pid_t child = -1; > > + int fd = -1; > > + int ret = TEST_FAIL; > > + > > + previous_sigio = signal(SIGIO, handle_sigio); > > + if (previous_sigio == SIG_ERR) { > > + pr_debug("failed setting SIGIO handler\n"); > > + goto cleanup; > > + } > > + > > + memset(&attr, 0, sizeof(attr)); > > + attr.size = sizeof(attr); > > + attr.type = PERF_TYPE_SOFTWARE; > > + attr.config = PERF_COUNT_SW_DUMMY; > > + attr.sample_period = 1; > > + attr.disabled = 1; > > + attr.watermark = 1; > > + attr.context_switch = 1; > > + attr.wakeup_watermark = 1; > > + > > + child = fork(); > > + if (child == 0) > > + do_child(); > > + else if (child < 0) { > > + pr_debug("failed fork() %d\n", errno); > > + goto cleanup; > > + } > > + > > + fd = sys_perf_event_open(&attr, child, -1, -1, > > + perf_event_open_cloexec_flag()); > > + if (fd < 0) { > > + pr_debug("failed opening event %llx\n", attr.config); > > + goto cleanup; > > + } > > + > > + if (fcntl(fd, F_SETFL, FASYNC)) { > > + pr_debug("failed F_SETFL FASYNC %d\n", errno); > > + goto cleanup; > > + } > > Thanks for the work! The perf tool and perf test should run on older > kernels ideally without failure. I'm assuming this would fail on an > older kernel. Could we make the return value skip in that case? Ah, hmm, I wasn't aware of that. This would fail on an older kernel, yes. It's not possible to distinguish between an older kernel and a kernel where this fix broke (at least not without hardcoding in an expected good kernel version, which seems undesirable), so that would mean the test would always return ok or skip, not ok or fail. Is that ok? > > + > > + if (fcntl(fd, F_SETOWN, getpid())) { > > + pr_debug("failed F_SETOWN getpid() %d\n", errno); > > + goto cleanup; > > + } > > + > > + if (fcntl(fd, F_SETSIG, SIGIO)) { > > + pr_debug("failed F_SETSIG SIGIO %d\n", errno); > > + goto cleanup; > > + } > > + > > + p = mmap(NULL, 2 * page_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); > > + if (p == NULL) { > > + pr_debug("failed to mmap\n"); > > + goto cleanup; > > + } > > + > > + if (ioctl(fd, PERF_EVENT_IOC_ENABLE, 0)) { > > + pr_debug("failed PERF_EVENT_IOC_ENABLE %d\n", errno); > > + goto cleanup; > > + } > > + > > + sleep(30); > > This is kind of a painful wait, could it be less? No sleeps would be best :-) We could synchronize with the child using SIGSTOP instead of sleeping here. Not sure about getting rid of the tiny sleeps in the child. I have observed that sched_yield() doesn't reliably trigger context switches (which isn't surprising). I'll experiment with blocking on a pipe or something. - Kyle > Thanks, > Ian > > > + > > + ret = (sigio_count == 1) ? TEST_OK : TEST_FAIL; > > + > > +cleanup: > > + if (p != NULL) > > + munmap(p, 2 * page_size); > > + > > + if (fd >= 0) > > + close(fd); > > + > > + if (child > 0) { > > + kill(child, SIGTERM); > > + waitpid(child, NULL, 0); > > + } > > + > > + if (previous_sigio != SIG_ERR) > > + signal(SIGIO, previous_sigio); > > + > > + return ret; > > +} > > + > > +DEFINE_SUITE("Watermark signal handler", watermark_signal); > > -- > > 2.34.1 > >
On Thu, Feb 22, 2024 at 9:55 AM Kyle Huey <me@kylehuey.com> wrote: > > On Wed, Feb 21, 2024 at 10:36 AM Ian Rogers <irogers@google.com> wrote: > > > > On Wed, Feb 21, 2024 at 9:52 AM Kyle Huey <me@kylehuey.com> wrote: > > > > > > The test uses PERF_RECORD_SWITCH records to fill the ring buffer and > > > trigger the watermark wakeup, which in turn should trigger an IO > > > signal. > > > > > > Signed-off-by: Kyle Huey <khuey@kylehuey.com> > > > --- > > > tools/perf/tests/Build | 1 + > > > tools/perf/tests/builtin-test.c | 1 + > > > tools/perf/tests/tests.h | 1 + > > > tools/perf/tests/watermark_signal.c | 123 ++++++++++++++++++++++++++++ > > > 4 files changed, 126 insertions(+) > > > create mode 100644 tools/perf/tests/watermark_signal.c > > > > > > diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build > > > index 53ba9c3e20e0..de43eb60b280 100644 > > > --- a/tools/perf/tests/Build > > > +++ b/tools/perf/tests/Build > > > @@ -67,6 +67,7 @@ perf-y += sigtrap.o > > > perf-y += event_groups.o > > > perf-y += symbols.o > > > perf-y += util.o > > > +perf-y += watermark_signal.o > > > > > > ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 arm arm64 powerpc)) > > > perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o > > > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c > > > index 4a5973f9bb9b..715c01a2172a 100644 > > > --- a/tools/perf/tests/builtin-test.c > > > +++ b/tools/perf/tests/builtin-test.c > > > @@ -124,6 +124,7 @@ static struct test_suite *generic_tests[] = { > > > &suite__event_groups, > > > &suite__symbols, > > > &suite__util, > > > + &suite__watermark_signal, > > > NULL, > > > }; > > > > > > diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h > > > index dad3d7414142..7ef4e0d0a77b 100644 > > > --- a/tools/perf/tests/tests.h > > > +++ b/tools/perf/tests/tests.h > > > @@ -146,6 +146,7 @@ DECLARE_SUITE(sigtrap); > > > DECLARE_SUITE(event_groups); > > > DECLARE_SUITE(symbols); > > > DECLARE_SUITE(util); > > > +DECLARE_SUITE(watermark_signal); > > > > > > /* > > > * PowerPC and S390 do not support creation of instruction breakpoints using the > > > diff --git a/tools/perf/tests/watermark_signal.c b/tools/perf/tests/watermark_signal.c > > > new file mode 100644 > > > index 000000000000..ae4abedc4b7c > > > --- /dev/null > > > +++ b/tools/perf/tests/watermark_signal.c > > > @@ -0,0 +1,123 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +#include <stddef.h> > > > +#include <signal.h> > > > +#include <stdlib.h> > > > +#include <string.h> > > > +#include <sys/ioctl.h> > > > +#include <sys/mman.h> > > > +#include <sys/wait.h> > > > +#include <unistd.h> > > > +#include <errno.h> > > > +#include <fcntl.h> > > > + > > > +#include "tests.h" > > > +#include "debug.h" > > > +#include "event.h" > > > +#include "../perf-sys.h" > > > +#include "cloexec.h" > > > +#include <internal/lib.h> // page_size > > > + > > > +static int sigio_count; > > > + > > > +static void handle_sigio(int sig __always_unused) > > > +{ > > > + ++sigio_count; > > > +} > > > + > > > +static void do_child(void) > > > +{ > > > + for (int i = 0; i < 20; ++i) > > > + sleep(1); > > > + > > > + exit(0); > > > +} > > > + > > > +static int test__watermark_signal(struct test_suite *test __maybe_unused, int subtest __maybe_unused) > > > +{ > > > + struct perf_event_attr attr; > > > + struct perf_event_mmap_page *p = NULL; > > > + sighandler_t previous_sigio = SIG_ERR; > > > + pid_t child = -1; > > > + int fd = -1; > > > + int ret = TEST_FAIL; > > > + > > > + previous_sigio = signal(SIGIO, handle_sigio); > > > + if (previous_sigio == SIG_ERR) { > > > + pr_debug("failed setting SIGIO handler\n"); > > > + goto cleanup; > > > + } > > > + > > > + memset(&attr, 0, sizeof(attr)); > > > + attr.size = sizeof(attr); > > > + attr.type = PERF_TYPE_SOFTWARE; > > > + attr.config = PERF_COUNT_SW_DUMMY; > > > + attr.sample_period = 1; > > > + attr.disabled = 1; > > > + attr.watermark = 1; > > > + attr.context_switch = 1; > > > + attr.wakeup_watermark = 1; > > > + > > > + child = fork(); > > > + if (child == 0) > > > + do_child(); > > > + else if (child < 0) { > > > + pr_debug("failed fork() %d\n", errno); > > > + goto cleanup; > > > + } > > > + > > > + fd = sys_perf_event_open(&attr, child, -1, -1, > > > + perf_event_open_cloexec_flag()); > > > + if (fd < 0) { > > > + pr_debug("failed opening event %llx\n", attr.config); > > > + goto cleanup; > > > + } > > > + > > > + if (fcntl(fd, F_SETFL, FASYNC)) { > > > + pr_debug("failed F_SETFL FASYNC %d\n", errno); > > > + goto cleanup; > > > + } > > > > Thanks for the work! The perf tool and perf test should run on older > > kernels ideally without failure. I'm assuming this would fail on an > > older kernel. Could we make the return value skip in that case? > > Ah, hmm, I wasn't aware of that. This would fail on an older kernel, > yes. It's not possible to distinguish between an older kernel and a > kernel where this fix broke (at least not without hardcoding in an > expected good kernel version, which seems undesirable), so that would > mean the test would always return ok or skip, not ok or fail. Is that > ok? Not great :-) Is there any hint from the errno? I was hoping for EOPNOTSUPP but I'm guessing it will be EINVAL, in which case probably something like: /* Older kernels lack support. */ ret = (errno == EINVAL) ? TEST_SKIP : TEST_FAIL; > > > + > > > + if (fcntl(fd, F_SETOWN, getpid())) { > > > + pr_debug("failed F_SETOWN getpid() %d\n", errno); > > > + goto cleanup; > > > + } > > > + > > > + if (fcntl(fd, F_SETSIG, SIGIO)) { > > > + pr_debug("failed F_SETSIG SIGIO %d\n", errno); > > > + goto cleanup; > > > + } > > > + > > > + p = mmap(NULL, 2 * page_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); > > > + if (p == NULL) { > > > + pr_debug("failed to mmap\n"); > > > + goto cleanup; > > > + } > > > + > > > + if (ioctl(fd, PERF_EVENT_IOC_ENABLE, 0)) { > > > + pr_debug("failed PERF_EVENT_IOC_ENABLE %d\n", errno); > > > + goto cleanup; > > > + } > > > + > > > + sleep(30); > > > > This is kind of a painful wait, could it be less? No sleeps would be best :-) > > We could synchronize with the child using SIGSTOP instead of sleeping > here. Not sure about getting rid of the tiny sleeps in the child. I > have observed that sched_yield() doesn't reliably trigger context > switches (which isn't surprising). I'll experiment with blocking on a > pipe or something. Great, thanks! Ian > - Kyle > > > Thanks, > > Ian > > > > > + > > > + ret = (sigio_count == 1) ? TEST_OK : TEST_FAIL; > > > + > > > +cleanup: > > > + if (p != NULL) > > > + munmap(p, 2 * page_size); > > > + > > > + if (fd >= 0) > > > + close(fd); > > > + > > > + if (child > 0) { > > > + kill(child, SIGTERM); > > > + waitpid(child, NULL, 0); > > > + } > > > + > > > + if (previous_sigio != SIG_ERR) > > > + signal(SIGIO, previous_sigio); > > > + > > > + return ret; > > > +} > > > + > > > +DEFINE_SUITE("Watermark signal handler", watermark_signal); > > > -- > > > 2.34.1 > > >
On Thu, Feb 22, 2024 at 10:33 AM Ian Rogers <irogers@google.com> wrote: > > On Thu, Feb 22, 2024 at 9:55 AM Kyle Huey <me@kylehuey.com> wrote: > > > > On Wed, Feb 21, 2024 at 10:36 AM Ian Rogers <irogers@google.com> wrote: > > > > > > On Wed, Feb 21, 2024 at 9:52 AM Kyle Huey <me@kylehuey.com> wrote: > > > > > > > > The test uses PERF_RECORD_SWITCH records to fill the ring buffer and > > > > trigger the watermark wakeup, which in turn should trigger an IO > > > > signal. > > > > > > > > Signed-off-by: Kyle Huey <khuey@kylehuey.com> > > > > --- > > > > tools/perf/tests/Build | 1 + > > > > tools/perf/tests/builtin-test.c | 1 + > > > > tools/perf/tests/tests.h | 1 + > > > > tools/perf/tests/watermark_signal.c | 123 ++++++++++++++++++++++++++++ > > > > 4 files changed, 126 insertions(+) > > > > create mode 100644 tools/perf/tests/watermark_signal.c > > > > > > > > diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build > > > > index 53ba9c3e20e0..de43eb60b280 100644 > > > > --- a/tools/perf/tests/Build > > > > +++ b/tools/perf/tests/Build > > > > @@ -67,6 +67,7 @@ perf-y += sigtrap.o > > > > perf-y += event_groups.o > > > > perf-y += symbols.o > > > > perf-y += util.o > > > > +perf-y += watermark_signal.o > > > > > > > > ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 arm arm64 powerpc)) > > > > perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o > > > > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c > > > > index 4a5973f9bb9b..715c01a2172a 100644 > > > > --- a/tools/perf/tests/builtin-test.c > > > > +++ b/tools/perf/tests/builtin-test.c > > > > @@ -124,6 +124,7 @@ static struct test_suite *generic_tests[] = { > > > > &suite__event_groups, > > > > &suite__symbols, > > > > &suite__util, > > > > + &suite__watermark_signal, > > > > NULL, > > > > }; > > > > > > > > diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h > > > > index dad3d7414142..7ef4e0d0a77b 100644 > > > > --- a/tools/perf/tests/tests.h > > > > +++ b/tools/perf/tests/tests.h > > > > @@ -146,6 +146,7 @@ DECLARE_SUITE(sigtrap); > > > > DECLARE_SUITE(event_groups); > > > > DECLARE_SUITE(symbols); > > > > DECLARE_SUITE(util); > > > > +DECLARE_SUITE(watermark_signal); > > > > > > > > /* > > > > * PowerPC and S390 do not support creation of instruction breakpoints using the > > > > diff --git a/tools/perf/tests/watermark_signal.c b/tools/perf/tests/watermark_signal.c > > > > new file mode 100644 > > > > index 000000000000..ae4abedc4b7c > > > > --- /dev/null > > > > +++ b/tools/perf/tests/watermark_signal.c > > > > @@ -0,0 +1,123 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +#include <stddef.h> > > > > +#include <signal.h> > > > > +#include <stdlib.h> > > > > +#include <string.h> > > > > +#include <sys/ioctl.h> > > > > +#include <sys/mman.h> > > > > +#include <sys/wait.h> > > > > +#include <unistd.h> > > > > +#include <errno.h> > > > > +#include <fcntl.h> > > > > + > > > > +#include "tests.h" > > > > +#include "debug.h" > > > > +#include "event.h" > > > > +#include "../perf-sys.h" > > > > +#include "cloexec.h" > > > > +#include <internal/lib.h> // page_size > > > > + > > > > +static int sigio_count; > > > > + > > > > +static void handle_sigio(int sig __always_unused) > > > > +{ > > > > + ++sigio_count; > > > > +} > > > > + > > > > +static void do_child(void) > > > > +{ > > > > + for (int i = 0; i < 20; ++i) > > > > + sleep(1); > > > > + > > > > + exit(0); > > > > +} > > > > + > > > > +static int test__watermark_signal(struct test_suite *test __maybe_unused, int subtest __maybe_unused) > > > > +{ > > > > + struct perf_event_attr attr; > > > > + struct perf_event_mmap_page *p = NULL; > > > > + sighandler_t previous_sigio = SIG_ERR; > > > > + pid_t child = -1; > > > > + int fd = -1; > > > > + int ret = TEST_FAIL; > > > > + > > > > + previous_sigio = signal(SIGIO, handle_sigio); > > > > + if (previous_sigio == SIG_ERR) { > > > > + pr_debug("failed setting SIGIO handler\n"); > > > > + goto cleanup; > > > > + } > > > > + > > > > + memset(&attr, 0, sizeof(attr)); > > > > + attr.size = sizeof(attr); > > > > + attr.type = PERF_TYPE_SOFTWARE; > > > > + attr.config = PERF_COUNT_SW_DUMMY; > > > > + attr.sample_period = 1; > > > > + attr.disabled = 1; > > > > + attr.watermark = 1; > > > > + attr.context_switch = 1; > > > > + attr.wakeup_watermark = 1; > > > > + > > > > + child = fork(); > > > > + if (child == 0) > > > > + do_child(); > > > > + else if (child < 0) { > > > > + pr_debug("failed fork() %d\n", errno); > > > > + goto cleanup; > > > > + } > > > > + > > > > + fd = sys_perf_event_open(&attr, child, -1, -1, > > > > + perf_event_open_cloexec_flag()); > > > > + if (fd < 0) { > > > > + pr_debug("failed opening event %llx\n", attr.config); > > > > + goto cleanup; > > > > + } > > > > + > > > > + if (fcntl(fd, F_SETFL, FASYNC)) { > > > > + pr_debug("failed F_SETFL FASYNC %d\n", errno); > > > > + goto cleanup; > > > > + } > > > > > > Thanks for the work! The perf tool and perf test should run on older > > > kernels ideally without failure. I'm assuming this would fail on an > > > older kernel. Could we make the return value skip in that case? > > > > Ah, hmm, I wasn't aware of that. This would fail on an older kernel, > > yes. It's not possible to distinguish between an older kernel and a > > kernel where this fix broke (at least not without hardcoding in an > > expected good kernel version, which seems undesirable), so that would > > mean the test would always return ok or skip, not ok or fail. Is that > > ok? > > Not great :-) Is there any hint from the errno? I was hoping for > EOPNOTSUPP but I'm guessing it will be EINVAL, in which case probably > something like: > /* Older kernels lack support. */ > ret = (errno == EINVAL) ? TEST_SKIP : TEST_FAIL; Ah, I see. The issue I am trying to fix is not that setting FASYNC is rejected with an error code, it's that it doesn't actually send the IO signal when the watermark is reached. - Kyle > > > > + > > > > + if (fcntl(fd, F_SETOWN, getpid())) { > > > > + pr_debug("failed F_SETOWN getpid() %d\n", errno); > > > > + goto cleanup; > > > > + } > > > > + > > > > + if (fcntl(fd, F_SETSIG, SIGIO)) { > > > > + pr_debug("failed F_SETSIG SIGIO %d\n", errno); > > > > + goto cleanup; > > > > + } > > > > + > > > > + p = mmap(NULL, 2 * page_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); > > > > + if (p == NULL) { > > > > + pr_debug("failed to mmap\n"); > > > > + goto cleanup; > > > > + } > > > > + > > > > + if (ioctl(fd, PERF_EVENT_IOC_ENABLE, 0)) { > > > > + pr_debug("failed PERF_EVENT_IOC_ENABLE %d\n", errno); > > > > + goto cleanup; > > > > + } > > > > + > > > > + sleep(30); > > > > > > This is kind of a painful wait, could it be less? No sleeps would be best :-) > > > > We could synchronize with the child using SIGSTOP instead of sleeping > > here. Not sure about getting rid of the tiny sleeps in the child. I > > have observed that sched_yield() doesn't reliably trigger context > > switches (which isn't surprising). I'll experiment with blocking on a > > pipe or something. > > Great, thanks! > Ian > > > - Kyle > > > > > Thanks, > > > Ian > > > > > > > + > > > > + ret = (sigio_count == 1) ? TEST_OK : TEST_FAIL; > > > > + > > > > +cleanup: > > > > + if (p != NULL) > > > > + munmap(p, 2 * page_size); > > > > + > > > > + if (fd >= 0) > > > > + close(fd); > > > > + > > > > + if (child > 0) { > > > > + kill(child, SIGTERM); > > > > + waitpid(child, NULL, 0); > > > > + } > > > > + > > > > + if (previous_sigio != SIG_ERR) > > > > + signal(SIGIO, previous_sigio); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +DEFINE_SUITE("Watermark signal handler", watermark_signal); > > > > -- > > > > 2.34.1 > > > >
On Thu, Feb 22, 2024 at 10:41 AM Kyle Huey <me@kylehuey.com> wrote: > > On Thu, Feb 22, 2024 at 10:33 AM Ian Rogers <irogers@google.com> wrote: > > > > On Thu, Feb 22, 2024 at 9:55 AM Kyle Huey <me@kylehuey.com> wrote: > > > > > > On Wed, Feb 21, 2024 at 10:36 AM Ian Rogers <irogers@google.com> wrote: > > > > > > > > On Wed, Feb 21, 2024 at 9:52 AM Kyle Huey <me@kylehuey.com> wrote: > > > > > > > > > > The test uses PERF_RECORD_SWITCH records to fill the ring buffer and > > > > > trigger the watermark wakeup, which in turn should trigger an IO > > > > > signal. > > > > > > > > > > Signed-off-by: Kyle Huey <khuey@kylehuey.com> > > > > > --- > > > > > tools/perf/tests/Build | 1 + > > > > > tools/perf/tests/builtin-test.c | 1 + > > > > > tools/perf/tests/tests.h | 1 + > > > > > tools/perf/tests/watermark_signal.c | 123 ++++++++++++++++++++++++++++ > > > > > 4 files changed, 126 insertions(+) > > > > > create mode 100644 tools/perf/tests/watermark_signal.c > > > > > > > > > > diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build > > > > > index 53ba9c3e20e0..de43eb60b280 100644 > > > > > --- a/tools/perf/tests/Build > > > > > +++ b/tools/perf/tests/Build > > > > > @@ -67,6 +67,7 @@ perf-y += sigtrap.o > > > > > perf-y += event_groups.o > > > > > perf-y += symbols.o > > > > > perf-y += util.o > > > > > +perf-y += watermark_signal.o > > > > > > > > > > ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 arm arm64 powerpc)) > > > > > perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o > > > > > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c > > > > > index 4a5973f9bb9b..715c01a2172a 100644 > > > > > --- a/tools/perf/tests/builtin-test.c > > > > > +++ b/tools/perf/tests/builtin-test.c > > > > > @@ -124,6 +124,7 @@ static struct test_suite *generic_tests[] = { > > > > > &suite__event_groups, > > > > > &suite__symbols, > > > > > &suite__util, > > > > > + &suite__watermark_signal, > > > > > NULL, > > > > > }; > > > > > > > > > > diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h > > > > > index dad3d7414142..7ef4e0d0a77b 100644 > > > > > --- a/tools/perf/tests/tests.h > > > > > +++ b/tools/perf/tests/tests.h > > > > > @@ -146,6 +146,7 @@ DECLARE_SUITE(sigtrap); > > > > > DECLARE_SUITE(event_groups); > > > > > DECLARE_SUITE(symbols); > > > > > DECLARE_SUITE(util); > > > > > +DECLARE_SUITE(watermark_signal); > > > > > > > > > > /* > > > > > * PowerPC and S390 do not support creation of instruction breakpoints using the > > > > > diff --git a/tools/perf/tests/watermark_signal.c b/tools/perf/tests/watermark_signal.c > > > > > new file mode 100644 > > > > > index 000000000000..ae4abedc4b7c > > > > > --- /dev/null > > > > > +++ b/tools/perf/tests/watermark_signal.c > > > > > @@ -0,0 +1,123 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > +#include <stddef.h> > > > > > +#include <signal.h> > > > > > +#include <stdlib.h> > > > > > +#include <string.h> > > > > > +#include <sys/ioctl.h> > > > > > +#include <sys/mman.h> > > > > > +#include <sys/wait.h> > > > > > +#include <unistd.h> > > > > > +#include <errno.h> > > > > > +#include <fcntl.h> > > > > > + > > > > > +#include "tests.h" > > > > > +#include "debug.h" > > > > > +#include "event.h" > > > > > +#include "../perf-sys.h" > > > > > +#include "cloexec.h" > > > > > +#include <internal/lib.h> // page_size > > > > > + > > > > > +static int sigio_count; > > > > > + > > > > > +static void handle_sigio(int sig __always_unused) > > > > > +{ > > > > > + ++sigio_count; > > > > > +} > > > > > + > > > > > +static void do_child(void) > > > > > +{ > > > > > + for (int i = 0; i < 20; ++i) > > > > > + sleep(1); > > > > > + > > > > > + exit(0); > > > > > +} > > > > > + > > > > > +static int test__watermark_signal(struct test_suite *test __maybe_unused, int subtest __maybe_unused) > > > > > +{ > > > > > + struct perf_event_attr attr; > > > > > + struct perf_event_mmap_page *p = NULL; > > > > > + sighandler_t previous_sigio = SIG_ERR; > > > > > + pid_t child = -1; > > > > > + int fd = -1; > > > > > + int ret = TEST_FAIL; > > > > > + > > > > > + previous_sigio = signal(SIGIO, handle_sigio); > > > > > + if (previous_sigio == SIG_ERR) { > > > > > + pr_debug("failed setting SIGIO handler\n"); > > > > > + goto cleanup; > > > > > + } > > > > > + > > > > > + memset(&attr, 0, sizeof(attr)); > > > > > + attr.size = sizeof(attr); > > > > > + attr.type = PERF_TYPE_SOFTWARE; > > > > > + attr.config = PERF_COUNT_SW_DUMMY; > > > > > + attr.sample_period = 1; > > > > > + attr.disabled = 1; > > > > > + attr.watermark = 1; > > > > > + attr.context_switch = 1; > > > > > + attr.wakeup_watermark = 1; > > > > > + > > > > > + child = fork(); > > > > > + if (child == 0) > > > > > + do_child(); > > > > > + else if (child < 0) { > > > > > + pr_debug("failed fork() %d\n", errno); > > > > > + goto cleanup; > > > > > + } > > > > > + > > > > > + fd = sys_perf_event_open(&attr, child, -1, -1, > > > > > + perf_event_open_cloexec_flag()); > > > > > + if (fd < 0) { > > > > > + pr_debug("failed opening event %llx\n", attr.config); > > > > > + goto cleanup; > > > > > + } > > > > > + > > > > > + if (fcntl(fd, F_SETFL, FASYNC)) { > > > > > + pr_debug("failed F_SETFL FASYNC %d\n", errno); > > > > > + goto cleanup; > > > > > + } > > > > > > > > Thanks for the work! The perf tool and perf test should run on older > > > > kernels ideally without failure. I'm assuming this would fail on an > > > > older kernel. Could we make the return value skip in that case? > > > > > > Ah, hmm, I wasn't aware of that. This would fail on an older kernel, > > > yes. It's not possible to distinguish between an older kernel and a > > > kernel where this fix broke (at least not without hardcoding in an > > > expected good kernel version, which seems undesirable), so that would > > > mean the test would always return ok or skip, not ok or fail. Is that > > > ok? > > > > Not great :-) Is there any hint from the errno? I was hoping for > > EOPNOTSUPP but I'm guessing it will be EINVAL, in which case probably > > something like: > > /* Older kernels lack support. */ > > ret = (errno == EINVAL) ? TEST_SKIP : TEST_FAIL; > > Ah, I see. The issue I am trying to fix is not that setting FASYNC is > rejected with an error code, it's that it doesn't actually send the IO > signal when the watermark is reached. That's unfortunate. Perhaps similar to errno you could use uname, return skip for failures on older kernels and fail on newer kernels. Thanks, Ian > - Kyle > > > > > > + > > > > > + if (fcntl(fd, F_SETOWN, getpid())) { > > > > > + pr_debug("failed F_SETOWN getpid() %d\n", errno); > > > > > + goto cleanup; > > > > > + } > > > > > + > > > > > + if (fcntl(fd, F_SETSIG, SIGIO)) { > > > > > + pr_debug("failed F_SETSIG SIGIO %d\n", errno); > > > > > + goto cleanup; > > > > > + } > > > > > + > > > > > + p = mmap(NULL, 2 * page_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); > > > > > + if (p == NULL) { > > > > > + pr_debug("failed to mmap\n"); > > > > > + goto cleanup; > > > > > + } > > > > > + > > > > > + if (ioctl(fd, PERF_EVENT_IOC_ENABLE, 0)) { > > > > > + pr_debug("failed PERF_EVENT_IOC_ENABLE %d\n", errno); > > > > > + goto cleanup; > > > > > + } > > > > > + > > > > > + sleep(30); > > > > > > > > This is kind of a painful wait, could it be less? No sleeps would be best :-) > > > > > > We could synchronize with the child using SIGSTOP instead of sleeping > > > here. Not sure about getting rid of the tiny sleeps in the child. I > > > have observed that sched_yield() doesn't reliably trigger context > > > switches (which isn't surprising). I'll experiment with blocking on a > > > pipe or something. > > > > Great, thanks! > > Ian > > > > > - Kyle > > > > > > > Thanks, > > > > Ian > > > > > > > > > + > > > > > + ret = (sigio_count == 1) ? TEST_OK : TEST_FAIL; > > > > > + > > > > > +cleanup: > > > > > + if (p != NULL) > > > > > + munmap(p, 2 * page_size); > > > > > + > > > > > + if (fd >= 0) > > > > > + close(fd); > > > > > + > > > > > + if (child > 0) { > > > > > + kill(child, SIGTERM); > > > > > + waitpid(child, NULL, 0); > > > > > + } > > > > > + > > > > > + if (previous_sigio != SIG_ERR) > > > > > + signal(SIGIO, previous_sigio); > > > > > + > > > > > + return ret; > > > > > +} > > > > > + > > > > > +DEFINE_SUITE("Watermark signal handler", watermark_signal); > > > > > -- > > > > > 2.34.1 > > > > >
On Thu, Feb 22, 2024 at 09:55:36AM -0800, Kyle Huey wrote: > On Wed, Feb 21, 2024 at 10:36 AM Ian Rogers <irogers@google.com> wrote: > > On Wed, Feb 21, 2024 at 9:52 AM Kyle Huey <me@kylehuey.com> wrote: > > > + if (fcntl(fd, F_SETFL, FASYNC)) { > > > + pr_debug("failed F_SETFL FASYNC %d\n", errno); > > > + goto cleanup; > > > + } > > Thanks for the work! The perf tool and perf test should run on older > > kernels ideally without failure. I'm assuming this would fail on an > > older kernel. Could we make the return value skip in that case? > Ah, hmm, I wasn't aware of that. This would fail on an older kernel, > yes. It's not possible to distinguish between an older kernel and a > kernel where this fix broke (at least not without hardcoding in an > expected good kernel version, which seems undesirable), so that would Take a look at: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/tools/perf/tests/sigtrap.c?id=650e0bde43f35bb675e87e30f679a57cfa22e0e5 To see how introspecting using BTF can be used to figure out if internal data structures have what is needed, or if functions with some specific arguments are present, etc, for sigtrap we have, in the patch above: - TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, NUM_THREADS * ctx.iterate_on); + expected_sigtraps = NUM_THREADS * ctx.iterate_on; + + if (ctx.signal_count < expected_sigtraps && kernel_with_sleepable_spinlocks()) { + pr_debug("Expected %d sigtraps, got %d, running on a kernel with sleepable spinlocks.\n", + expected_sigtraps, ctx.signal_count); + pr_debug("See https://lore.kernel.org/all/e368f2c848d77fbc8d259f44e2055fe469c219cf.camel@gmx.de/\n"); + return TEST_SKIP; + } else + TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, expected_sigtraps); And then: +static bool kernel_with_sleepable_spinlocks(void) +{ + const struct btf_member *member; + const struct btf_type *type; + const char *type_name; + int id; + + if (!btf__available()) + return false; + + id = btf__find_by_name_kind(btf, "spinlock", BTF_KIND_STRUCT); + if (id < 0) + return false; + + // Only RT has a "lock" member for "struct spinlock" + member = __btf_type__find_member_by_name(id, "lock"); + if (member == NULL) + return false; + + // But check its type as well + type = btf__type_by_id(btf, member->type); + if (!type || !btf_is_struct(type)) + return false; + + type_name = btf__name_by_offset(btf, type->name_off); + return type_name && !strcmp(type_name, "rt_mutex_base"); +} > mean the test would always return ok or skip, not ok or fail. Is that > ok? It should return: Ok if the kernel has what is needed and the test passes Skip if the test fails and the kernel doesn't have what is needed FAIL if the test fails and the kernel HAS what is needed. 'perf test sigtrap' also checks for the presence of the feature it requires: static bool attr_has_sigtrap(void) { int id; if (!btf__available()) { /* should be an old kernel */ return false; } id = btf__find_by_name_kind(btf, "perf_event_attr", BTF_KIND_STRUCT); if (id < 0) return false; return __btf_type__find_member_by_name(id, "sigtrap") != NULL; } fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag()); if (fd < 0) { if (attr_has_sigtrap()) { pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf))); } else { pr_debug("perf_event_attr doesn't have sigtrap\n"); ret = TEST_SKIP; } goto out_restore_sigaction; } - Arnaldo
On Thu, Feb 22, 2024 at 11:54 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > On Thu, Feb 22, 2024 at 09:55:36AM -0800, Kyle Huey wrote: > > On Wed, Feb 21, 2024 at 10:36 AM Ian Rogers <irogers@google.com> wrote: > > > On Wed, Feb 21, 2024 at 9:52 AM Kyle Huey <me@kylehuey.com> wrote: > > > > + if (fcntl(fd, F_SETFL, FASYNC)) { > > > > + pr_debug("failed F_SETFL FASYNC %d\n", errno); > > > > + goto cleanup; > > > > + } > > > > Thanks for the work! The perf tool and perf test should run on older > > > kernels ideally without failure. I'm assuming this would fail on an > > > older kernel. Could we make the return value skip in that case? > > > Ah, hmm, I wasn't aware of that. This would fail on an older kernel, > > yes. It's not possible to distinguish between an older kernel and a > > kernel where this fix broke (at least not without hardcoding in an > > expected good kernel version, which seems undesirable), so that would > > Take a look at: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/tools/perf/tests/sigtrap.c?id=650e0bde43f35bb675e87e30f679a57cfa22e0e5 > > To see how introspecting using BTF can be used to figure out if internal > data structures have what is needed, or if functions with some specific > arguments are present, etc, for sigtrap we have, in the patch above: > > - TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, NUM_THREADS * ctx.iterate_on); > + expected_sigtraps = NUM_THREADS * ctx.iterate_on; > + > + if (ctx.signal_count < expected_sigtraps && kernel_with_sleepable_spinlocks()) { > + pr_debug("Expected %d sigtraps, got %d, running on a kernel with sleepable spinlocks.\n", > + expected_sigtraps, ctx.signal_count); > + pr_debug("See https://lore.kernel.org/all/e368f2c848d77fbc8d259f44e2055fe469c219cf.camel@gmx.de/\n"); > + return TEST_SKIP; > + } else > + TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, expected_sigtraps); > > And then: > > +static bool kernel_with_sleepable_spinlocks(void) > +{ > + const struct btf_member *member; > + const struct btf_type *type; > + const char *type_name; > + int id; > + > + if (!btf__available()) > + return false; > + > + id = btf__find_by_name_kind(btf, "spinlock", BTF_KIND_STRUCT); > + if (id < 0) > + return false; > + > + // Only RT has a "lock" member for "struct spinlock" > + member = __btf_type__find_member_by_name(id, "lock"); > + if (member == NULL) > + return false; > + > + // But check its type as well > + type = btf__type_by_id(btf, member->type); > + if (!type || !btf_is_struct(type)) > + return false; > + > + type_name = btf__name_by_offset(btf, type->name_off); > + return type_name && !strcmp(type_name, "rt_mutex_base"); > +} > > > mean the test would always return ok or skip, not ok or fail. Is that > > ok? > > It should return: > > Ok if the kernel has what is needed and the test passes > Skip if the test fails and the kernel doesn't have what is needed > FAIL if the test fails and the kernel HAS what is needed. > > 'perf test sigtrap' also checks for the presence of the feature it > requires: > > static bool attr_has_sigtrap(void) > { > int id; > > if (!btf__available()) { > /* should be an old kernel */ > return false; > } > > id = btf__find_by_name_kind(btf, "perf_event_attr", BTF_KIND_STRUCT); > if (id < 0) > return false; > > return __btf_type__find_member_by_name(id, "sigtrap") != NULL; > } > > fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag()); > if (fd < 0) { > if (attr_has_sigtrap()) { > pr_debug("FAILED sys_perf_event_open(): %s\n", > str_error_r(errno, sbuf, sizeof(sbuf))); > } else { > pr_debug("perf_event_attr doesn't have sigtrap\n"); > ret = TEST_SKIP; > } > goto out_restore_sigaction; > } > > - Arnaldo I think perhaps I'm barking up the wrong tree here. This seems like a ton of work just to write a regression test. Maybe I should be doing this in tools/testing/selftests instead? - Kyle
On Fri, Feb 23, 2024 at 09:35:29AM -0800, Kyle Huey wrote: > On Thu, Feb 22, 2024 at 11:54 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag()); > > if (fd < 0) { > > if (attr_has_sigtrap()) { > > pr_debug("FAILED sys_perf_event_open(): %s\n", > > str_error_r(errno, sbuf, sizeof(sbuf))); > > } else { > > pr_debug("perf_event_attr doesn't have sigtrap\n"); > > ret = TEST_SKIP; > > } > > goto out_restore_sigaction; > I think perhaps I'm barking up the wrong tree here. This seems like a > ton of work just to write a regression test. Maybe I should be doing > this in tools/testing/selftests instead? Well, if it tests a perf feature, then yeah, that would be better placed in 'perf test'. Maybe you can just assume this will run just on modern kernels where this feature is expected to be present and then add some pr_debug() stating that maybe this feature is not present in the kernel. For the vast majority of cases this will be enough, so that is what I encourage you to do now. Its possible that distros, like Red Hat, end up backporting first the perf tools with this test and not the feature it is being tested, if that happens, then it will eventually come to my attention and I will be able to do the BTF treatment, as in that case even a test based on the kernel version would be insufficient. - Arnaldo
On Fri, Feb 23, 2024 at 9:35 AM Kyle Huey <me@kylehuey.com> wrote: > > On Thu, Feb 22, 2024 at 11:54 AM Arnaldo Carvalho de Melo > <acme@kernel.org> wrote: > > > > On Thu, Feb 22, 2024 at 09:55:36AM -0800, Kyle Huey wrote: > > > On Wed, Feb 21, 2024 at 10:36 AM Ian Rogers <irogers@google.com> wrote: > > > > On Wed, Feb 21, 2024 at 9:52 AM Kyle Huey <me@kylehuey.com> wrote: > > > > > + if (fcntl(fd, F_SETFL, FASYNC)) { > > > > > + pr_debug("failed F_SETFL FASYNC %d\n", errno); > > > > > + goto cleanup; > > > > > + } > > > > > > Thanks for the work! The perf tool and perf test should run on older > > > > kernels ideally without failure. I'm assuming this would fail on an > > > > older kernel. Could we make the return value skip in that case? > > > > > Ah, hmm, I wasn't aware of that. This would fail on an older kernel, > > > yes. It's not possible to distinguish between an older kernel and a > > > kernel where this fix broke (at least not without hardcoding in an > > > expected good kernel version, which seems undesirable), so that would > > > > Take a look at: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/tools/perf/tests/sigtrap.c?id=650e0bde43f35bb675e87e30f679a57cfa22e0e5 > > > > To see how introspecting using BTF can be used to figure out if internal > > data structures have what is needed, or if functions with some specific > > arguments are present, etc, for sigtrap we have, in the patch above: > > > > - TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, NUM_THREADS * ctx.iterate_on); > > + expected_sigtraps = NUM_THREADS * ctx.iterate_on; > > + > > + if (ctx.signal_count < expected_sigtraps && kernel_with_sleepable_spinlocks()) { > > + pr_debug("Expected %d sigtraps, got %d, running on a kernel with sleepable spinlocks.\n", > > + expected_sigtraps, ctx.signal_count); > > + pr_debug("See https://lore.kernel.org/all/e368f2c848d77fbc8d259f44e2055fe469c219cf.camel@gmx.de/\n"); > > + return TEST_SKIP; > > + } else > > + TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, expected_sigtraps); > > > > And then: > > > > +static bool kernel_with_sleepable_spinlocks(void) > > +{ > > + const struct btf_member *member; > > + const struct btf_type *type; > > + const char *type_name; > > + int id; > > + > > + if (!btf__available()) > > + return false; > > + > > + id = btf__find_by_name_kind(btf, "spinlock", BTF_KIND_STRUCT); > > + if (id < 0) > > + return false; > > + > > + // Only RT has a "lock" member for "struct spinlock" > > + member = __btf_type__find_member_by_name(id, "lock"); > > + if (member == NULL) > > + return false; > > + > > + // But check its type as well > > + type = btf__type_by_id(btf, member->type); > > + if (!type || !btf_is_struct(type)) > > + return false; > > + > > + type_name = btf__name_by_offset(btf, type->name_off); > > + return type_name && !strcmp(type_name, "rt_mutex_base"); > > +} > > > > > mean the test would always return ok or skip, not ok or fail. Is that > > > ok? > > > > It should return: > > > > Ok if the kernel has what is needed and the test passes > > Skip if the test fails and the kernel doesn't have what is needed > > FAIL if the test fails and the kernel HAS what is needed. > > > > 'perf test sigtrap' also checks for the presence of the feature it > > requires: > > > > static bool attr_has_sigtrap(void) > > { > > int id; > > > > if (!btf__available()) { > > /* should be an old kernel */ > > return false; > > } > > > > id = btf__find_by_name_kind(btf, "perf_event_attr", BTF_KIND_STRUCT); > > if (id < 0) > > return false; > > > > return __btf_type__find_member_by_name(id, "sigtrap") != NULL; > > } > > > > fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag()); > > if (fd < 0) { > > if (attr_has_sigtrap()) { > > pr_debug("FAILED sys_perf_event_open(): %s\n", > > str_error_r(errno, sbuf, sizeof(sbuf))); > > } else { > > pr_debug("perf_event_attr doesn't have sigtrap\n"); > > ret = TEST_SKIP; > > } > > goto out_restore_sigaction; > > } > > > > - Arnaldo > > I think perhaps I'm barking up the wrong tree here. This seems like a > ton of work just to write a regression test. Maybe I should be doing > this in tools/testing/selftests instead? The problem is detecting support for the feature in the kernel. The BTF approach isn't that bad, a couple of finds, but I think in this case there isn't anything to be found to indicate the feature is present. I like the perf test as perf tests are a form of documentation. Perhaps just using TEST_SKIP here (rather than TEST_FAIL) is best and the skip_reason can be a presumed lack of kernel support. Thanks, Ian > - Kyle
On Fri, Feb 23, 2024 at 10:01:31AM -0800, Ian Rogers wrote: > On Fri, Feb 23, 2024 at 9:35 AM Kyle Huey <me@kylehuey.com> wrote: > > I think perhaps I'm barking up the wrong tree here. This seems like a > > ton of work just to write a regression test. Maybe I should be doing > > this in tools/testing/selftests instead? > The problem is detecting support for the feature in the kernel. The > BTF approach isn't that bad, a couple of finds, but I think in this > case there isn't anything to be found to indicate the feature is > present. I like the perf test as perf tests are a form of > documentation. Perhaps just using TEST_SKIP here (rather than > TEST_FAIL) is best and the skip_reason can be a presumed lack of > kernel support. But going forward the general expectation is that it should pass as the feature _is_ present, isn't it? - Arnaldo
(I work with Kyle.) IMHO this is more of a bug fix than a feature. `man perf_event_open` expects this to work already: "watermark: If set, have an overflow notification happen when we cross the wakeup_watermark boundary" and later "Alternatively, the overflow events can be captured via a signal handler, by enabling I/O signaling". Bug fixes need regression tests. Such tests should fail on any kernel where the bug is present. It seems strange to expect each such test to detect whether the bug "should be fixed" in the kernel it's running on and skip when that's not the case. I haven't seen any other project try to do this. Instead (as in kernel selftests) the tests, the code under test, and any metadata about which tests are expected to pass are all in the repository together and updated together. It makes sense that tests for the code in tools/perf should not fail on older kernels, given that the code in tools/perf is expected to work on older kernels. But tests for bug fixes in the kernel itself should be expected to fail on older kernels and therefore should live somewhere else, IMHO. Rob
Hello, On Fri, Feb 23, 2024 at 1:44 PM Robert O'Callahan <robert@ocallahanorg> wrote: > > (I work with Kyle.) > > IMHO this is more of a bug fix than a feature. `man perf_event_open` > expects this to work already: "watermark: If set, have an overflow > notification happen when we cross the wakeup_watermark boundary" and > later "Alternatively, the overflow events can be captured via a signal > handler, by enabling I/O signaling". > > Bug fixes need regression tests. Such tests should fail on any kernel > where the bug is present. It seems strange to expect each such test to > detect whether the bug "should be fixed" in the kernel it's running on > and skip when that's not the case. I haven't seen any other project > try to do this. Instead (as in kernel selftests) the tests, the code > under test, and any metadata about which tests are expected to pass > are all in the repository together and updated together. > > It makes sense that tests for the code in tools/perf should not fail > on older kernels, given that the code in tools/perf is expected to > work on older kernels. But tests for bug fixes in the kernel itself > should be expected to fail on older kernels and therefore should live > somewhere else, IMHO. I think it makes sense to put the test in the selftests and to be deployed with the kernel. AFAICS it doesn't have anything specific to the perf tools and tests the general kernel behavior. Thanks, Namhyung
On Sat, Feb 24, 2024 at 10:43:38AM +1300, Robert O'Callahan wrote: > (I work with Kyle.) > > IMHO this is more of a bug fix than a feature. `man perf_event_open` > expects this to work already: "watermark: If set, have an overflow > notification happen when we cross the wakeup_watermark boundary" and > later "Alternatively, the overflow events can be captured via a signal > handler, by enabling I/O signaling". > > Bug fixes need regression tests. Such tests should fail on any kernel > where the bug is present. It seems strange to expect each such test to > detect whether the bug "should be fixed" in the kernel it's running on > and skip when that's not the case. I haven't seen any other project > try to do this. Instead (as in kernel selftests) the tests, the code > under test, and any metadata about which tests are expected to pass > are all in the repository together and updated together. > > It makes sense that tests for the code in tools/perf should not fail > on older kernels, given that the code in tools/perf is expected to > work on older kernels. But tests for bug fixes in the kernel itself > should be expected to fail on older kernels and therefore should live > somewhere else, IMHO. That is fine, I was mostly trying to help in having the 'perf test' patch posted to address the review comments from Ian. And I'm biased because I work on a company that does lots of backports and then test perf functionality using 'perf test'. And also because the kernel now has this BTF information that can be used for introspection. Being able to run the installed 'perf' tool and check if some fix is in place and its regression test is passing looked like an improvement when compared to the selftests method. If in the end people decide to do this in selftests, that is ok with me. - Arnaldo
diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build index 53ba9c3e20e0..de43eb60b280 100644 --- a/tools/perf/tests/Build +++ b/tools/perf/tests/Build @@ -67,6 +67,7 @@ perf-y += sigtrap.o perf-y += event_groups.o perf-y += symbols.o perf-y += util.o +perf-y += watermark_signal.o ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 arm arm64 powerpc)) perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c index 4a5973f9bb9b..715c01a2172a 100644 --- a/tools/perf/tests/builtin-test.c +++ b/tools/perf/tests/builtin-test.c @@ -124,6 +124,7 @@ static struct test_suite *generic_tests[] = { &suite__event_groups, &suite__symbols, &suite__util, + &suite__watermark_signal, NULL, }; diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h index dad3d7414142..7ef4e0d0a77b 100644 --- a/tools/perf/tests/tests.h +++ b/tools/perf/tests/tests.h @@ -146,6 +146,7 @@ DECLARE_SUITE(sigtrap); DECLARE_SUITE(event_groups); DECLARE_SUITE(symbols); DECLARE_SUITE(util); +DECLARE_SUITE(watermark_signal); /* * PowerPC and S390 do not support creation of instruction breakpoints using the diff --git a/tools/perf/tests/watermark_signal.c b/tools/perf/tests/watermark_signal.c new file mode 100644 index 000000000000..ae4abedc4b7c --- /dev/null +++ b/tools/perf/tests/watermark_signal.c @@ -0,0 +1,123 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <stddef.h> +#include <signal.h> +#include <stdlib.h> +#include <string.h> +#include <sys/ioctl.h> +#include <sys/mman.h> +#include <sys/wait.h> +#include <unistd.h> +#include <errno.h> +#include <fcntl.h> + +#include "tests.h" +#include "debug.h" +#include "event.h" +#include "../perf-sys.h" +#include "cloexec.h" +#include <internal/lib.h> // page_size + +static int sigio_count; + +static void handle_sigio(int sig __always_unused) +{ + ++sigio_count; +} + +static void do_child(void) +{ + for (int i = 0; i < 20; ++i) + sleep(1); + + exit(0); +} + +static int test__watermark_signal(struct test_suite *test __maybe_unused, int subtest __maybe_unused) +{ + struct perf_event_attr attr; + struct perf_event_mmap_page *p = NULL; + sighandler_t previous_sigio = SIG_ERR; + pid_t child = -1; + int fd = -1; + int ret = TEST_FAIL; + + previous_sigio = signal(SIGIO, handle_sigio); + if (previous_sigio == SIG_ERR) { + pr_debug("failed setting SIGIO handler\n"); + goto cleanup; + } + + memset(&attr, 0, sizeof(attr)); + attr.size = sizeof(attr); + attr.type = PERF_TYPE_SOFTWARE; + attr.config = PERF_COUNT_SW_DUMMY; + attr.sample_period = 1; + attr.disabled = 1; + attr.watermark = 1; + attr.context_switch = 1; + attr.wakeup_watermark = 1; + + child = fork(); + if (child == 0) + do_child(); + else if (child < 0) { + pr_debug("failed fork() %d\n", errno); + goto cleanup; + } + + fd = sys_perf_event_open(&attr, child, -1, -1, + perf_event_open_cloexec_flag()); + if (fd < 0) { + pr_debug("failed opening event %llx\n", attr.config); + goto cleanup; + } + + if (fcntl(fd, F_SETFL, FASYNC)) { + pr_debug("failed F_SETFL FASYNC %d\n", errno); + goto cleanup; + } + + if (fcntl(fd, F_SETOWN, getpid())) { + pr_debug("failed F_SETOWN getpid() %d\n", errno); + goto cleanup; + } + + if (fcntl(fd, F_SETSIG, SIGIO)) { + pr_debug("failed F_SETSIG SIGIO %d\n", errno); + goto cleanup; + } + + p = mmap(NULL, 2 * page_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); + if (p == NULL) { + pr_debug("failed to mmap\n"); + goto cleanup; + } + + if (ioctl(fd, PERF_EVENT_IOC_ENABLE, 0)) { + pr_debug("failed PERF_EVENT_IOC_ENABLE %d\n", errno); + goto cleanup; + } + + sleep(30); + + ret = (sigio_count == 1) ? TEST_OK : TEST_FAIL; + +cleanup: + if (p != NULL) + munmap(p, 2 * page_size); + + if (fd >= 0) + close(fd); + + if (child > 0) { + kill(child, SIGTERM); + waitpid(child, NULL, 0); + } + + if (previous_sigio != SIG_ERR) + signal(SIGIO, previous_sigio); + + return ret; +} + +DEFINE_SUITE("Watermark signal handler", watermark_signal);