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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 15147C52D7B for ; Tue, 13 Aug 2024 14:56:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=O/d6A/qhlCYrDmQa5ZxJBHKaNKQHaV6za1HEEAuYgug=; b=1rjv3dFKkxezz4SGgiJoLXZeGS 4paFPR2f/O51sJrKNOLNhngRC1m4t5IMc7Ct5/ojszMMToPTzhhFl2+x2p8MXYhh57C/jMcoxgZwq LcaHwvUj79iNrwXqvPL1Rppx+aJEy+ja8li4d1tk/s+pJS33W7xGdxicYcsXb4dRKIpRuASj0flsr alnPXxzPgFKvABd3KBV8aCI3iq71QDXEzeTws7Z1xoey3e6XyXso4n42aJkAs4/j4HZvyt4/mYD2E 7D9S5He/CCC/s/Hh/7jrSOmAPHHNBRPf71vkIAbYK6lQt94s/j1ISPcJgFHwqfndpF+YV8B+zDZ44 vw7JIzMA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sdsx7-000000046gt-3j7R; Tue, 13 Aug 2024 14:56:49 +0000 Received: from mail-wr1-x42f.google.com ([2a00:1450:4864:20::42f]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sdswU-000000046XA-1awm for linux-arm-kernel@lists.infradead.org; Tue, 13 Aug 2024 14:56:12 +0000 Received: by mail-wr1-x42f.google.com with SMTP id ffacd0b85a97d-3684eb5be64so3129143f8f.3 for ; Tue, 13 Aug 2024 07:56:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1723560968; x=1724165768; darn=lists.infradead.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=O/d6A/qhlCYrDmQa5ZxJBHKaNKQHaV6za1HEEAuYgug=; b=P3XZ7xQSa612E9CqheYvVg+lrpkA6/hkvdTbLBtd2Dh2XCxaN4tGXPT+5guo4lw/9U FnrgNIlAqrR5L+dCk0AIdM2N+Lg3KHo6+AQU5VDtzQ/a6N9G3vVfjN9AxAuDFcS0N9bu SqNTpFHKktsVxKdj5RwSgDw73CDX+ZZJsqY1rjDVyWzRQxLu5XJc2VTsM7dMwHIcp8+f MVs6iYlvJYCAyitGNKZTGURryk5PtAzTVuL33XKHgLzSxPdTCViy2X8b2ZmMm2FOMJjr YRIzX7IoPERGh/hi5x3PEobPiMBve8YEZUYRnnWOMH5fDI39rUBB+UteZtfdkUJm1hHa BkHQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723560968; x=1724165768; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=O/d6A/qhlCYrDmQa5ZxJBHKaNKQHaV6za1HEEAuYgug=; b=hD6/AK5xcxQ9QgQxft0MboqEfAf5GQl5g1Uu1Bo4hzwg9iXEPRC6M8+r0wCg7ob6sp Sqt+D8ubKRqiZRKMjmjtSOzXSM0fAe3thgq9ow7gGPKclCH5EXp7P2GeIWTLWSHWkwZM 1wfvW4mZSm/3ARFkzZYHZgK1lP+YOD2QwgZ+BAXJo9zPVj60VtjoBpiWXAxGekHXx/KT N4bRdTfoPE/3eWgoZPz2mUKKayJlGLm6PxX/R/L3VoyjUtH6wTKujHKTjWv1HacMU6EY 5yuBa6nz2wwcBb4HZ7ZsZerkia/CEfjBNK7DfDwPaLHEL9EE6a1lVTUVgdt4EmKFOkhy OtNg== X-Forwarded-Encrypted: i=1; AJvYcCV2Q0ahojJQeCQl0M1d9UupyzA6rGUiJEPvrvKyswPUs9rSWHFqfRjJ7YP3sSz1ImB4+XemxfYkUlpHqEx6Ws2Iy81ZV5J8Hf9H3IYVQmCEh+3b43c= X-Gm-Message-State: AOJu0YxsINc0UaZf2v+OvdykVBVG9ie70RzaH4rEWMuh9biPebxvC3IQ B7J2DTu7/zV7Nre5kR21B3Er9OhAlqjoGFoNgLn6LXXnF9gAlTDihEAKVZ2Nlts= X-Google-Smtp-Source: AGHT+IH7RYPn9n3FNQLlyWO1pof58QJznrPiQ53W+LJb5iUy2dFNCJFeItywrmlmentV2tdDZTVzxQ== X-Received: by 2002:adf:e04a:0:b0:367:998a:87b3 with SMTP id ffacd0b85a97d-3716ccfe7f7mr2817949f8f.28.1723560968081; Tue, 13 Aug 2024 07:56:08 -0700 (PDT) Received: from [192.168.1.3] ([89.47.253.130]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-36e4c937b23sm10542160f8f.35.2024.08.13.07.56.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 13 Aug 2024 07:56:07 -0700 (PDT) Message-ID: <9c0ab8c6-fdb1-49b2-a030-b74e779b6e48@linaro.org> Date: Tue, 13 Aug 2024 15:56:06 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/7] perf stat: Initialize instead of overwriting clock event To: Ian Rogers Cc: linux-perf-users@vger.kernel.org, John Garry , Will Deacon , Mike Leach , Leo Yan , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Namhyung Kim , Mark Rutland , Alexander Shishkin , Jiri Olsa , Adrian Hunter , "Liang, Kan" , Yang Jihong , Ze Gao , Dominique Martinet , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org References: <20240813132323.98728-1-james.clark@linaro.org> <20240813132323.98728-2-james.clark@linaro.org> <76ee9150-36fa-4dfc-ba9f-8a10df580c92@linaro.org> Content-Language: en-US From: James Clark In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240813_075610_454350_6AF9AFC0 X-CRM114-Status: GOOD ( 25.47 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 13/08/2024 3:43 pm, Ian Rogers wrote: > On Tue, Aug 13, 2024 at 7:38 AM James Clark wrote: >> >> >> >> On 13/08/2024 3:28 pm, Ian Rogers wrote: >>> On Tue, Aug 13, 2024 at 6:24 AM James Clark wrote: >>>> >>>> This overwrite relies on the clock event remaining at index 0 and is >>>> quite a way down from where the array is initialized, making it easy to >>>> miss. Just initialize it with the correct clock event to begin with. >>>> >>>> Signed-off-by: James Clark >>>> --- >>>> tools/perf/builtin-stat.c | 7 +++---- >>>> 1 file changed, 3 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c >>>> index 1f92445f7480..a65f58f8783f 100644 >>>> --- a/tools/perf/builtin-stat.c >>>> +++ b/tools/perf/builtin-stat.c >>>> @@ -1817,7 +1817,9 @@ static int add_default_attributes(void) >>>> { >>>> struct perf_event_attr default_attrs0[] = { >>>> >>>> - { .type = PERF_TYPE_SOFTWARE, .config = PERF_COUNT_SW_TASK_CLOCK }, >>>> + { .type = PERF_TYPE_SOFTWARE, .config = target__has_cpu(&target) ? >>>> + PERF_COUNT_SW_CPU_CLOCK : >>>> + PERF_COUNT_SW_TASK_CLOCK }, >>> >>> Hand crafting perf_event_attr when we have an event name to >>> perf_event_atttr parser doesn't make sense. Doing things this way >>> means we need to duplicate logic between event parsing and these >>> default configurations. The default configurations are also using >>> legacy events which of course are broken on Apple ARM M? (albeit for >>> hardware events, here it is software). Event and metric parsing has to >>> worry about things like grouping topdown events. All-in-all let's have >>> one way to do things, event parsing, otherwise this code is going to >>> end up reinventing all the workarounds the event parsing has to have. >>> Lots of struct perf_event_attr also contribute to binary size. >>> >>> If you are worried about a cycles event being opened on arm_dsu PMUs, >>> there is this patch: >>> https://lore.kernel.org/lkml/20240525152927.665498-1-irogers@google.com/ >>> >>> Thanks, >>> Ian >> >> Hi Ian, >> >> Is this comment related to this patch specifically or is it more of a >> general comment? >> >> This patch doesn't really make any actual changes other than move one >> line of code from one place to another. > > James, this code is removed here: > https://lore.kernel.org/lkml/20240510053705.2462258-4-irogers@google.com/ > > Thanks, > Ian Oh I see yeah. We can still work on that change, merging this one doesn't necessarily have to block that one, it just makes this one a bit redundant when the other one gets done. If I remember correctly in one of the last related discussions we thought that opening the cycles event as a sampling event should be a softer warning? Actually it seems like the DSU cycles event is a non-issue specifically for perf stat because it will open successfully anyway? It was just in perf record where it was the issue?