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 21A9FC433EF for ; Thu, 7 Apr 2022 17:47:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=drYv1y9XDRUx3rK6ESt4NDfzTU6pjAMR3uvRjmJp+nE=; b=3xDUi/mNzhkvXT S0oToiJLvnvgbgsv3ehQnVzZxgvl1YAlF22pKfrgTMfUwwbrOP+EVPhZsn4ZD3x4VT/Bz283pge+A FRYYf1BGZoYhOshaA6WfmClrUWXk47uW1FVDebrZaPbaSeTpUp5qTbNVp2l1A76DJYRJis1AMxvP+ cvnyCwz1XpuEXe8g6oo1ChPoOwaazSeIqsnc8rUcATnYOh+jPBMm7Ds4c9OLyQu+scHp4tCIkCqBh 0HczwL2uIK+uSq17Cvqof78xGAfkLMKhBcwSJklq5/gju0H1Wcnu/pxxVG6hPtyWdID/Cmm15gUMI F+2TSQcZxDnw0EXythiA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ncWDO-00DRlm-1F; Thu, 07 Apr 2022 17:46:38 +0000 Received: from mail-pg1-x52c.google.com ([2607:f8b0:4864:20::52c]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ncWDK-00DRiE-CO for linux-arm-kernel@lists.infradead.org; Thu, 07 Apr 2022 17:46:36 +0000 Received: by mail-pg1-x52c.google.com with SMTP id z128so5579963pgz.2 for ; Thu, 07 Apr 2022 10:46:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=glYV/l98Ao73KMk9v/wcQMQsuIKB62MFE0MbECEB0bU=; b=HqOjqTdpUg8BZniBO/NW6yOtfh1s/xQNadFPrvtrh4cZEyz0PcnUnZHLcuc2NOLgZy 65NawgWlhpko3+AdB6n4v64b2tK8ye4gOhdvV1dD0OFj3XGP19HTxJ1JgyLWm6P6k8+n hNpcW/A8zRCpIWDHw4Bh9nMkRX3vC7DVzrN83N0z1xs1f58zmSaOQvXMQ0oeRE5Ect/n VfLmm3q7xLyeb/MJZSrGjYFiBbCRvYQQGZSfPkuB6NhWjGAhawp5uuweYY8ESS8Ex8KD ZYz8zTlsyCc7DUgDYkGl17tNSOdKqxud95KTOJRehhHpUbDQmnX0PClssah+OnSxbH3N orCw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=glYV/l98Ao73KMk9v/wcQMQsuIKB62MFE0MbECEB0bU=; b=kZBzyPwVGH2mzMBC6IgiiFVcLUb4pJsvysTkM0jpypJvulrwv52hxf4Hh+obHg0c1J 1omjNwEFPvmteTlTPkvto+rlpZ2h9M/39jjcRFi/BCCe9wKZLhmX4JTnD7kH9xc11vKT 0xK8XxBwTs0Xk/K3PVG8pEP6iPvOFyCwZhI63bkiqLzYflJaIsZdZQzmyfT33m6N7qWQ 6LKF5El1+YxooY0lxrKWmjrApsUZ80EIrY08d+0ET88WsI4I8zFVV0CZGWgQSYCMUN7Z U0F+ZtPzX14PnzldDlT/nooRB2uhR+D3aRFbz7pq9HWNbUOhRcwaxd8woD7mSCk8WOpv 5L7w== X-Gm-Message-State: AOAM531z+UAUpHQ9MdNE5Mc2CYQJ3FiC35A7LIHydplp7BuNoNrzY7dn BNYamR+HkZuk+pcs8pHDrnt32g== X-Google-Smtp-Source: ABdhPJwWzWTtKrbU/L85VXBDuYFm4dEdyrUa/oHetBMpYkSefD5/LRCLbjqFtOCTrMCYytD8j93sPQ== X-Received: by 2002:a65:6a97:0:b0:398:ae2:d207 with SMTP id q23-20020a656a97000000b003980ae2d207mr11843101pgu.197.1649353590776; Thu, 07 Apr 2022 10:46:30 -0700 (PDT) Received: from p14s (S0106889e681aac74.cg.shawcable.net. [68.147.0.187]) by smtp.gmail.com with ESMTPSA id x6-20020a17090aa38600b001ca2f87d271sm9611813pjp.15.2022.04.07.10.46.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Apr 2022 10:46:29 -0700 (PDT) Date: Thu, 7 Apr 2022 11:46:27 -0600 From: Mathieu Poirier To: Mike Leach Cc: suzuki.poulose@arm.com, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, peterz@infradead.org, mingo@redhat.com, acme@kernel.org, linux-perf-users@vger.kernel.org, leo.yan@linaro.org Subject: Re: [PATCH 06/10] coresight: perf: traceid: Add perf notifiers for trace ID Message-ID: <20220407174627.GA102085@p14s> References: <20220308205000.27646-1-mike.leach@linaro.org> <20220308205000.27646-7-mike.leach@linaro.org> <20220406171132.GA16110@p14s> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220407_104634_525699_36D24138 X-CRM114-Status: GOOD ( 41.94 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Good morning, On Wed, Apr 06, 2022 at 08:38:08PM +0100, Mike Leach wrote: > Hi Mathieu, > > On Wed, 6 Apr 2022 at 18:11, Mathieu Poirier wrote: > > > > On Tue, Mar 08, 2022 at 08:49:56PM +0000, Mike Leach wrote: > > > Adds in notifier calls to the trace ID allocator that perf > > > events are starting and stopping. > > > > > > This ensures that Trace IDs associated with CPUs remain the same > > > throughout the perf session, and are only release when all perf > > > sessions are complete. > > > > > > Signed-off-by: Mike Leach > > > --- > > > drivers/hwtracing/coresight/coresight-etm-perf.c | 16 +++++++++++++++- > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > > > index c039b6ae206f..008f9dac429d 100644 > > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > > > @@ -22,6 +22,7 @@ > > > #include "coresight-etm-perf.h" > > > #include "coresight-priv.h" > > > #include "coresight-syscfg.h" > > > +#include "coresight-trace-id.h" > > > > > > static struct pmu etm_pmu; > > > static bool etm_perf_up; > > > @@ -223,11 +224,21 @@ static void free_event_data(struct work_struct *work) > > > struct list_head **ppath; > > > > > > ppath = etm_event_cpu_path_ptr(event_data, cpu); > > > - if (!(IS_ERR_OR_NULL(*ppath))) > > > + if (!(IS_ERR_OR_NULL(*ppath))) { > > > coresight_release_path(*ppath); > > > + /* > > > + * perf may have read a trace id for a cpu, but never actually > > > + * executed code on that cpu - which means the trace id would > > > + * not release on disable. Re-release here to be sure. > > > + */ > > > + coresight_trace_id_put_cpu_id(cpu, coresight_get_trace_id_map()); > > > > A CPU gets a traceID in event_etm_start() when the event is installed for > > running. Do you see a scenario where etm_free_aux() is called without > > previously calling event_etm_stop()? > > > > To ensure that IDs are obtained in a timely manner, they assign on > read. Therefore when cs_etm.c::cs_etm_info_fill() is called, > potentially the ID will be read for all CPUs - and dumped into the > AUXINFO data at the top of the perf.data file. Right, I realised that when I got to the perf tools part. If we end up keeping the current approach it would be nice to see this explanation in the comment. Otherwise it will be very difficult for anyone new to the project to understand what is going on. > However, a --per-thread execution may actually only start the event on > a subset of cpus, hence we ensure that all cpus are released. > This was proven during testing when I ran both --per-thread and cpu > wide tests with logging monitoring the ID assignments. > > I have programmed this deliberately defensively - on the basis that > the timings and orderings of the code/callbacks in todays perf may not > necessarily be the same in tomorrows. perf. > > In future we may be able to use Suzuki's idea of embedding the ID into > an alternative packet in the perf.data file. but I think that should > be a separate change as it affects decode in a big way. > I really like Suzuki's idea of using a PERF_RECORD_AUX_OUTPUT_HW_ID to convey the traceID to user space for perf sessions. At the very least it is worth prototyping. I generally prefer an incremental approach but in this case it might be better to move forward the right way, right away. We would also avoid having to maintain the old way, the intermediate way and the new way. Thanks, Mathieu > Regards > > Mike > > > > > + } > > > *ppath = NULL; > > > } > > > > > > + /* mark perf event as done for trace id allocator */ > > > + coresight_trace_id_perf_stop(); > > > + > > > free_percpu(event_data->path); > > > kfree(event_data); > > > } > > > @@ -314,6 +325,9 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, > > > sink = user_sink = coresight_get_sink_by_id(id); > > > } > > > > > > + /* tell the trace ID allocator that a perf event is starting up */ > > > + coresight_trace_id_perf_start(); > > > + > > > /* check if user wants a coresight configuration selected */ > > > cfg_hash = (u32)((event->attr.config2 & GENMASK_ULL(63, 32)) >> 32); > > > if (cfg_hash) { > > > -- > > > 2.17.1 > > > > > > > -- > Mike Leach > Principal Engineer, ARM Ltd. > Manchester Design Centre. UK _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel