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 X-Spam-Level: X-Spam-Status: No, score=-10.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 80515C43461 for ; Thu, 17 Sep 2020 11:54:43 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1E20E2072E for ; Thu, 17 Sep 2020 11:54:40 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="HJZEXntL"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="hRlfO4Wv" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1E20E2072E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Message-ID:References:In-Reply-To:Subject:To:From: Date:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=bnkqXIIKRkawpQe0CctKaW4D9G7jqgnrUidIxIblbVo=; b=HJZEXntLy0EYg23zZns3TzHWw ze8Uv48FMr9do2m1CFTVe2gCoquWkXQ+yEk1Vm0UeHTBcg+GEGF5M30i5zFANHc38yNQs5QUh3Zne xkubzu66XnhaxaaTBtXhqbKHotwKEBxFKlxF5PnEEO1Hpk1LmzmJa/JiKYXuUTNMPsRSRlYj/YwWF tcTB9iA9nL/ryEhOSR7UjkkQBIGTmuNq+ivU5XrjdsFtvPqonnEFpqLI1ao/FPF1bXeQhRd4HlJk/ bsed8T3tthPBpj4bJ9i8/w3P0JlWMlE6suERnh15hjXgYbiaOcZ/ydjbFLMP0ybv9foxUGZltD8W5 P7LdsNp5A==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kIsTN-0006pk-Ao; Thu, 17 Sep 2020 11:53:09 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kIsTK-0006oE-Gq for linux-arm-kernel@lists.infradead.org; Thu, 17 Sep 2020 11:53:07 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 9FCAD2072E; Thu, 17 Sep 2020 11:53:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600343584; bh=LgHVyCnfHVJpVCFssZUccF8/Y4bMHlpZUZppm8PrH/w=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=hRlfO4WvBy0pDog+vP/5Z5LGlkODLpML2T8jDREoCEwPcT6CaiMiFx8+iLYJWdEBn zCJJULGnYYMyDgyuXIzTYuk2b103CaDJEgRZBNgEPlhsPkayxhId+L6jxdlWo4bvRG d5DtsmaANCQ+scgeEzGjt9TpT037OORaMPD2v9IE= Received: from disco-boy.misterjones.org ([51.254.78.96] helo=www.loen.fr) by disco-boy.misterjones.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1kIsTG-00CcmC-Hf; Thu, 17 Sep 2020 12:53:02 +0100 MIME-Version: 1.0 Date: Thu, 17 Sep 2020 12:53:02 +0100 From: Marc Zyngier To: Leo Yan Subject: Re: [PATCHv3] perf kvm: add kvm-stat for arm64 In-Reply-To: <20200917114231.GE12548@leoy-ThinkPad-X240s> References: <20200917003645.689665-1-sergey.senozhatsky@gmail.com> <20200917100950.GC12548@leoy-ThinkPad-X240s> <20200917101219.GD12548@leoy-ThinkPad-X240s> <652f10660f09bd608b825233713f775a@kernel.org> <20200917114231.GE12548@leoy-ThinkPad-X240s> User-Agent: Roundcube Webmail/1.4.8 Message-ID: X-Sender: maz@kernel.org X-SA-Exim-Connect-IP: 51.254.78.96 X-SA-Exim-Rcpt-To: leo.yan@linaro.org, sergey.senozhatsky@gmail.com, acme@kernel.org, mark.rutland@arm.com, peterz@infradead.org, will@kernel.org, john.garry@huawei.com, mathieu.poirier@linaro.org, namhyung@kernel.org, suleiman@google.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200917_075306_681782_DD7C5A42 X-CRM114-Status: GOOD ( 33.84 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Mathieu Poirier , Peter Zijlstra , John Garry , linux-kernel@vger.kernel.org, Arnaldo Carvalho de Melo , Sergey Senozhatsky , Suleiman Souhlal , Namhyung Kim , Will Deacon , linux-arm-kernel@lists.infradead.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2020-09-17 12:42, Leo Yan wrote: > On Thu, Sep 17, 2020 at 11:21:15AM +0100, Marc Zyngier wrote: > > [...] > >> > > > +const char *vcpu_id_str = "id"; >> > > >> > > On Arm64, ftrace tracepoint "kvm_entry" doesn't contain the field "id" >> > > or field "vcpu_id", thus it always reads out the "id" is 0 and it is >> > > recorded into Perf's structure vcpu_event_record::vcpu_id and assigned >> > > to perf thread's private data "thread::private". >> > > >> > > With current code, it will not mess up different vcpus' samples >> > > because >> > > now the samples are analyzed based on thread context, but since all >> > > threads' "vcpu_id" is zero, thus all samples are accounted for >> > > "vcpu_id=0" and cannot print out correct result with option "--vcpu": >> > > >> > > >> > > $ perf kvm stat report --vcpu 4 >> > > >> > > Analyze events for all VMs, VCPU 4: >> > > >> > > VM-EXIT Samples Samples% Time% Min Time >> > > Max Time Avg time >> > > >> > > Total Samples:0, Total events handled time:0.00us. >> > > >> > > >> > > This is an issue I observed, if we want to support option "--vcpu", >> > > seems we need to change ftrace event for "kvm_entry", but this will >> > > break ABI. >> > > >> > > Essentially, this issue is caused by different archs using different >> > > format for ftrace event "kvm_entry", on x86 it contains feild >> > > "vcpu_id" but arm64 only just records "vcpu_pc". >> > > >> > > @Marc, @Will, do you have any suggestion for this? Do you think it's >> > > feasible to add a new field "vcpu_id" into the tracepoint "kvm_entry" >> > > for Arm64's version? >> >> The question really is: how will you handle the ABI breackage? >> I don't see a good solution for it, apart from having a *separate* >> tracepoint that collects all the information you need. And even that >> is >> really ugly. > > I searched a bit and found in practice it's not impossible to add new > parameters for existed tracepoint, e.g. [1][2] are two examples to add > new parameters for existed tracepoints and have been merged into > mainline kernel. IIUC, we keep the old parameters for a tracepoint > so this can avoid to break ABI if any apps have used this tracepoint, > and adding a new parameter for the tracepoint should be safe. > > If you agree with this, I'd like to suggest to apply below change. > How about you think for this? > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 46dc3d75cf13..d9f9b8e1df77 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -736,7 +736,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > /************************************************************** > * Enter the guest > */ > - trace_kvm_entry(*vcpu_pc(vcpu)); > + trace_kvm_entry(vcpu->vcpu_id, *vcpu_pc(vcpu)); > guest_enter_irqoff(); > > ret = kvm_call_hyp_ret(__kvm_vcpu_run, vcpu); > diff --git a/arch/arm64/kvm/trace_arm.h b/arch/arm64/kvm/trace_arm.h > index 4691053c5ee4..e1d3e7a67e8b 100644 > --- a/arch/arm64/kvm/trace_arm.h > +++ b/arch/arm64/kvm/trace_arm.h > @@ -12,18 +12,20 @@ > * Tracepoints for entry/exit to guest > */ > TRACE_EVENT(kvm_entry, > - TP_PROTO(unsigned long vcpu_pc), > - TP_ARGS(vcpu_pc), > + TP_PROTO(unsigned int vcpu_id, unsigned long vcpu_pc), > + TP_ARGS(vcpu_id, vcpu_pc), > > TP_STRUCT__entry( > + __field( unsigned int, vcpu_id ) > __field( unsigned long, vcpu_pc ) > ), > > TP_fast_assign( > + __entry->vcpu_id = vcpu_id; > __entry->vcpu_pc = vcpu_pc; > ), > > - TP_printk("PC: 0x%08lx", __entry->vcpu_pc) > + TP_printk("vcpu: %u, PC: 0x%08lx", __entry->vcpu_id, > __entry->vcpu_pc) > ); > > TRACE_EVENT(kvm_exit, > How is that not breaking the ABI? You are adding a new field, and anything that expect to read 'PC: 0x.....' at the beginning of the line now fails. The examples you give are also blatant ABI breakages. because it is done somewhere else doesn't make it valid. Anything that can be parsed by userspace is ABI. If you don't believe me, please read the entertaining discussion we had when we tried to drop Bogomips from /proc/cpuinfo. So unless you get me Linus' stamp of approval for this, it's not happening. Feel free to add a *new* tracepoint instead. M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel