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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 76B2BEB64DA for ; Fri, 30 Jun 2023 21:02:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231508AbjF3VCy convert rfc822-to-8bit (ORCPT ); Fri, 30 Jun 2023 17:02:54 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58484 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232375AbjF3VCx (ORCPT ); Fri, 30 Jun 2023 17:02:53 -0400 Received: from mail-yb1-f170.google.com (mail-yb1-f170.google.com [209.85.219.170]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5AAC93AAF; Fri, 30 Jun 2023 14:02:52 -0700 (PDT) Received: by mail-yb1-f170.google.com with SMTP id 3f1490d57ef6-bfee66a5db6so2180424276.0; Fri, 30 Jun 2023 14:02:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688158971; x=1690750971; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=QaJR3XXXCrhbjYtKCZ1X+dPieH6DxQBprcivRtbfQUY=; b=gVTRCWp6g1EyckUqz/MVo9qU+keBJqzfj6cYAeeomlMHxJ1wm/34Crkxjxsu2g6Suz BQ6dlDvBui2tz16xHg5KtCwjru95Ui5KMbn7o2fXs/h7gxZFHt8+91Erotfg1zn9T38G Jm2LV0q7FI6RLn7qpfJpsKOpOC8sU/VUIi6YrWGrAMxOOtF2BiEh6P+KGtRP/ZJ0QB3z tdBqAdmpDGTF68AnXIYxAnfyAMF9JQkoeSF5zlfAnzN5LCyMKRLr/gKQ6IqfLT0D+gW8 PC5AKYZ4qC66LklWxarFhW8LSFiGiTXp8ZHY8yyV+MjpsqF7o6ZCyGyZboXPaGj9OrcE iKBg== X-Gm-Message-State: ABy/qLZbCGev5ka5qRJ1f/HOS2+Zl0pLIXA4T2F9TuvsHgag/dneWncu ufG5l01kcVHYWY2aEPuQ0iY/3ClCLaUZQUNNmt8= X-Google-Smtp-Source: APBJJlFUh7jF98u3G1CNzoNLEJHzfbIQW3ootq9oZwxEGAIzPEvLBI5WrxqwcVcpo1/Qaq1hhd6xn6HyeUrqFHcrYYk= X-Received: by 2002:a25:26c4:0:b0:c40:f090:98bf with SMTP id m187-20020a2526c4000000b00c40f09098bfmr3019764ybm.28.1688158971230; Fri, 30 Jun 2023 14:02:51 -0700 (PDT) MIME-Version: 1.0 References: <20230626161059.324046-1-james.clark@arm.com> <20230626161059.324046-3-james.clark@arm.com> In-Reply-To: From: Namhyung Kim Date: Fri, 30 Jun 2023 14:02:38 -0700 Message-ID: Subject: Re: [PATCH 2/2] perf report: Don't add to histogram when there is no thread found To: James Clark Cc: Ian Rogers , linux-perf-users@vger.kernel.org, Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Adrian Hunter , Suzuki K Poulose , Mike Leach , Leo Yan , John Garry , Will Deacon , linux-kernel@vger.kernel.org, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT Precedence: bulk List-ID: X-Mailing-List: linux-perf-users@vger.kernel.org On Wed, Jun 28, 2023 at 1:06 PM Namhyung Kim wrote: > > On Wed, Jun 28, 2023 at 3:34 AM James Clark wrote: > > > > > > > > On 27/06/2023 18:19, Ian Rogers wrote: > > > On Tue, Jun 27, 2023 at 9:58 AM Namhyung Kim wrote: > > >> > > >> On Tue, Jun 27, 2023 at 9:43 AM Ian Rogers wrote: > > >>> > > >>> On Mon, Jun 26, 2023 at 5:02 PM Namhyung Kim wrote: > > >>>> > > >>>> On Mon, Jun 26, 2023 at 05:10:58PM +0100, James Clark wrote: > > >>>>> thread__find_map() chooses to exit without assigning a thread to the > > >>>>> addr_location in some scenarios, for example when there are samples from > > >>>>> a guest and perf_guest == false. This results in a segfault when adding > > >>>>> to the histogram because it uses unguarded accesses to the thread member > > >>>>> of the addr_location. > > >>>> > > >>>> Looking at the commit 0dd5041c9a0ea ("perf addr_location: Add > > >>>> init/exit/copy functions") that introduced the change, I'm not sure if > > >>>> it's the intend behavior. > > >>>> > > >>>> It might change maps and map, but not thread. Then I think no reason > > >>>> to not set the al->thread at the beginning. > > >>>> > > >>>> How about this? Ian? > > >>>> (I guess we can get rid of the duplicate 'al->map = NULL' part) > > >>> > > >>> It seemed strange that we were failing to find a map (the function's > > >>> purpose) but then populating the address_location. The change below > > >>> brings back that somewhat odd behavior. I'm okay with reverting to the > > >>> old behavior, clearly there were users relying on it. We should > > >>> probably also copy maps and not just thread, as that was the previous > > >>> behavior. > > >> > > >> Probably. But it used to support samples without maps and I think > > >> that's why it ignores the return value of thread__find_map(). So > > >> we can expect al.map is NULL and maybe fine to leave it for now. > > >> > > >> As machine__resolve() returns -1 if it gets no thread, we should set > > >> al.thread when it returns 0. > > >> > > >> Can I get your Acked-by? > > > > > > Yep: > > > Acked-by: Ian Rogers > > > > Looks good to me too. Should I resend the set with this change instead > > of my one? > > No, I can take care of that. I'll take this as your Acked-by. :) This part is applied to perf-tools-next, thanks!