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=-9.6 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=unavailable 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 C05F6C282C2 for ; Wed, 23 Jan 2019 23:45:29 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 83F3B21019 for ; Wed, 23 Jan 2019 23:45:29 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="ika0z7Uh"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=linaro.org header.i=@linaro.org header.b="J8GRzmmb" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 83F3B21019 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-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=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject: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=VfZHgzzyDeUL9OLn3DBi3EXlSG6pRnL1NAxcvz5tfjA=; b=ika0z7UhsaOVnE 2JpHTi7EiqFd0kaZPqY88fz8zrt4UwRKEtHnAXOtgTQHq4hjjmHiHYWUX/2fInflovYsuxv4JrN+A el2ClU//WRTpPmLZt/5hXz78Q2w6nHdKLgpGOjj1c7A7EAS2nt3YzenF7u+DgNpnUAnamYdsKocQD NhG5b+ErWSZ64P+veYs7u3ocXo1WT7HWN33XVWKrBuGFYTi/bGF8gdSJpZujxPl4brJhjluLED4dq a8eWVQztTa5jEaItZWbhCAo5M9iRysu/PmDgy1yaXxljkVI5Kpu4RGoQjj3F4qp0iB5f8gnj2q3em Cs5xanqfOO7T11d+AAXA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1gmSD1-00077I-Ji; Wed, 23 Jan 2019 23:45:27 +0000 Received: from mail-ot1-x344.google.com ([2607:f8b0:4864:20::344]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1gmSCx-00076q-MW for linux-arm-kernel@lists.infradead.org; Wed, 23 Jan 2019 23:45:25 +0000 Received: by mail-ot1-x344.google.com with SMTP id i20so3654384otl.0 for ; Wed, 23 Jan 2019 15:45:23 -0800 (PST) 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:user-agent; bh=a3th3BKt1EQbGJOKU8oizYvxZ8/FDSRcU1watEvglZY=; b=J8GRzmmb+RYWJS+uMs/9xie4QPR1lTnfvKcmFiOrNBDuNGHenKIlH3hBeg2WaLiq6z w7h8auWxHllDUuocciDn7NYF0eu3uw7M6FCs0QCzPdiAwFiOwySCJcGApBGQiE2lhFh0 a7Y7OTb90q0abhigjRr94WVFMz0p1//jNvN2o= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=a3th3BKt1EQbGJOKU8oizYvxZ8/FDSRcU1watEvglZY=; b=OeuK/mIzeFQpCvdU/iAbMpiltWve/2mhMMHoepUxRLsnFhlbz+KAN/2bxBCGgY5/RN gofZwxywAT7hDKISdHdurLIYK1+lC/EcuRQ7A1RvY/KwT/G6C6vy1xnatlv42/Y29wSQ kXXho/QFgvgx2UoD/GM0oRmz3CPKu2o+e3Bl0V7KKnxnnOdrZ74VGph93BHF4pkmBeyJ 0sv8YrutL7pCXgEdAb5kK4mFq4kghkufDytq+rkuQj1nz8SkBEhGClCtQdev/cVK8IEp QSazE81LPeSTYd4arVI+78eAQz6FKUJCMfXpw10wyXhppYBzgeNu8CWYJZDNLzy5/+7l kJ4w== X-Gm-Message-State: AJcUukeqkPuRmZYwwvaQsWsldj4q7+QaoxeNDr9JCtydyoFwdquKkfMl nwIAxhyqTru1mfUqWQgq6AfGsw== X-Google-Smtp-Source: ALg8bN6dupU3zeKkd1JJr9ER4sChtX/C+3HJ4kq0r7sDpLOcxBRptmnNucd2o6OoMtOmCRHKuYJ9Cg== X-Received: by 2002:a9d:2aea:: with SMTP id e97mr2888299otb.206.1548287122477; Wed, 23 Jan 2019 15:45:22 -0800 (PST) Received: from leoy-ThinkPad-X240s (li1118-108.members.linode.com. [45.79.19.108]) by smtp.gmail.com with ESMTPSA id y25sm8406759otk.50.2019.01.23.15.45.18 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 23 Jan 2019 15:45:21 -0800 (PST) Date: Thu, 24 Jan 2019 07:45:14 +0800 From: Leo Yan To: Mathieu Poirier Subject: Re: [PATCH v6 5/8] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata Message-ID: <20190123234514.GB3268@leoy-ThinkPad-X240s> References: <20190119014347.27441-1-leo.yan@linaro.org> <20190119014347.27441-6-leo.yan@linaro.org> <20190123211300.GE620@xps15> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190123211300.GE620@xps15> User-Agent: Mutt/1.9.4 (2018-02-28) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190123_154523_740521_79B362A9 X-CRM114-Status: GOOD ( 32.52 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Suzuki K Poulose , Alexander Shishkin , Coresight ML , linux-kernel@vger.kernel.org, Arnaldo Carvalho de Melo , Namhyung Kim , Robert Walker , Jiri Olsa , linux-arm-kernel@lists.infradead.org, Mike Leach Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Jan 23, 2019 at 02:13:00PM -0700, Mathieu Poirier wrote: > On Sat, Jan 19, 2019 at 09:43:44AM +0800, Leo Yan wrote: > > If packet processing wants to know the packet is bound with which ETM > > version, it needs to access metadata to decide that based on metadata > > magic number; but we cannot simply to use CPU logic ID number as index > > to access metadata sequential array, especially when system have > > hotplugged off CPUs, the metadata array are only allocated for online > > CPUs but not offline CPUs, so the CPU logic number doesn't match with > > its index in the array. > > > > For this reason, a reliable way for accessing metadata array is to use > > traceID to find associated metadata; by accessing metadata content we > > can know not only the CPU number but also for ETM version, which can be > > used for sequential change for setting sample flags for exception > > packets. > > This paragraph is not needed to understand why this patch is needed. Please > remove. Will do. > > This patch is to change tuple from traceID-CPU# to traceID-metadata, > > thus it can use the tuple to retrieve metadata pointer according to > > traceID. > > > > For safe accessing metadata fields, this patch provides helper function > > cs_etm__get_cpu() which is used to return CPU number according to > > traceID; cs_etm_decoder__buffer_packet() is the first consumer for this > > helper function. > > > > Signed-off-by: Leo Yan > > --- > > .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 8 ++-- > > tools/perf/util/cs-etm.c | 37 ++++++++++++++++--- > > tools/perf/util/cs-etm.h | 4 +- > > 3 files changed, 37 insertions(+), 12 deletions(-) > > > > diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > > index 294efa76c9e3..cdd38ffd10d2 100644 > > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > > @@ -305,14 +305,12 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, > > enum cs_etm_sample_type sample_type) > > { > > u32 et = 0; > > - struct int_node *inode = NULL; > > + int cpu; > > > > if (decoder->packet_count >= MAX_BUFFER - 1) > > return OCSD_RESP_FATAL_SYS_ERR; > > > > - /* Search the RB tree for the cpu associated with this traceID */ > > - inode = intlist__find(traceid_list, trace_chan_id); > > - if (!inode) > > + if (cs_etm__get_cpu(trace_chan_id, &cpu) < 0) > > return OCSD_RESP_FATAL_SYS_ERR; > > > > et = decoder->tail; > > @@ -322,7 +320,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, > > > > decoder->packet_buffer[et].sample_type = sample_type; > > decoder->packet_buffer[et].isa = CS_ETM_ISA_UNKNOWN; > > - decoder->packet_buffer[et].cpu = *((int *)inode->priv); > > + decoder->packet_buffer[et].cpu = cpu; > > decoder->packet_buffer[et].start_addr = CS_ETM_INVAL_ADDR; > > decoder->packet_buffer[et].end_addr = CS_ETM_INVAL_ADDR; > > decoder->packet_buffer[et].instr_count = 0; > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > > index 1aa29633ce77..e89989fe0a5c 100644 > > --- a/tools/perf/util/cs-etm.c > > +++ b/tools/perf/util/cs-etm.c > > @@ -97,6 +97,20 @@ static u32 cs_etm__get_v7_protocol_version(u32 etmidr) > > return CS_ETM_PROTO_ETMV3; > > } > > > > +int cs_etm__get_cpu(u8 trace_chan_id, int *cpu) > > +{ > > + struct int_node *inode; > > + u64 *metadata; > > + > > + inode = intlist__find(traceid_list, trace_chan_id); > > + if (!inode) > > + return -EINVAL; > > + > > + metadata = inode->priv; > > + *cpu = (int)metadata[CS_ETM_CPU]; > > + return 0; > > +} > > + > > static void cs_etm__packet_dump(const char *pkt_string) > > { > > const char *color = PERF_COLOR_BLUE; > > @@ -252,7 +266,7 @@ static void cs_etm__free(struct perf_session *session) > > cs_etm__free_events(session); > > session->auxtrace = NULL; > > > > - /* First remove all traceID/CPU# nodes for the RB tree */ > > + /* First remove all traceID/metadata nodes for the RB tree */ > > intlist__for_each_entry_safe(inode, tmp, traceid_list) > > intlist__remove(traceid_list, inode); > > /* Then the RB tree itself */ > > @@ -1519,9 +1533,20 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > > 0xffffffff); > > > > /* > > - * Create an RB tree for traceID-CPU# tuple. Since the conversion has > > - * to be made for each packet that gets decoded, optimizing access in > > - * anything other than a sequential array is worth doing. > > + * Create an RB tree for traceID-metadata tuple. > > + * > > + * The conversion between traceID and CPU logic ID number has to > > + * be made for each packet that gets decoded: firstly retrieve > > + * metadata pointer from trace ID by using traceID-metadata tuple, > > + * then read CPU logic ID number in metadata. > > + * > > + * It's not safe to directly use CPU logic ID number as index to > > + * access metadata sequential array, e.g. when system have > > + * hotplugged out CPUs, the metadata array are only allocated for > > + * online CPUs but not offline CPUs, thus the CPU logic number is > > + * not consistent with its index in the arrary. For this reason, > > + * we need to fallback to use TraceID-metadata tuple as a reliable > > + * method to access metadata. > > Why adding this long comment? To me all that is needed is > s/traceID-CPU#/traceID-metadata . Here just want to give more information for why we create traceID-metadata tuple (and hope can give the reason why we need to use traceID-metadata rather than traceID-CPU# tuple). If you think it's redundant, will drop it. > > */ > > traceid_list = intlist__new(NULL); > > if (!traceid_list) { > > @@ -1587,8 +1612,8 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > > err = -EINVAL; > > goto err_free_metadata; > > } > > - /* All good, associate the traceID with the CPU# */ > > - inode->priv = &metadata[j][CS_ETM_CPU]; > > + /* All good, associate the traceID with the metadata pointer */ > > + inode->priv = metadata[j]; > > } > > > > /* > > diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h > > index 37f8d48179ca..5d70d10f3907 100644 > > --- a/tools/perf/util/cs-etm.h > > +++ b/tools/perf/util/cs-etm.h > > @@ -53,7 +53,7 @@ enum { > > CS_ETMV4_PRIV_MAX, > > }; > > > > -/* RB tree for quick conversion between traceID and CPUs */ > > +/* RB tree for quick conversion between traceID and metadata pointers */ > > struct intlist *traceid_list; > > > > #define KiB(x) ((x) * 1024) > > @@ -78,4 +78,6 @@ cs_etm__process_auxtrace_info(union perf_event *event __maybe_unused, > > } > > #endif > > > > +int cs_etm__get_cpu(u8 trace_chan_id, int *cpu); > > This function is part of a public header that can theoretically be included by > any other file. As such it has to be defined within the HAVE_CSTRACE_SUPPORT > define. My stupid. Will move it into HAVE_CSTRACE_SUPPORT blocks. Thanks a lot for reviewing. > > + > > #endif > > -- > > 2.17.1 > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel