| Message ID | 20230315094316.26772-2-yangyicong@huawei.com |
|---|---|
| State | New |
| Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org>
Delivered-To: ouuuleilei@gmail.com
Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp2232267wrd;
Wed, 15 Mar 2023 02:51:13 -0700 (PDT)
X-Google-Smtp-Source:
AK7set9N7xQHxlzq2sgRP1zsMQx3vI1/Y5RsqRlXTwVfrdJvEwGEHmwWcASGtz9a02CKuWosHiy1
X-Received: by 2002:aa7:9805:0:b0:624:7aac:ab1 with SMTP id
e5-20020aa79805000000b006247aac0ab1mr9345208pfl.21.1678873873429;
Wed, 15 Mar 2023 02:51:13 -0700 (PDT)
ARC-Seal: i=1; a=rsa-sha256; t=1678873873; cv=none;
d=google.com; s=arc-20160816;
b=0FrmhpOi98u6tlxG2Qqil8cvpmSYwj6UdWJOLfis5XpopGiA4WxubLcXcJvjyK12qR
pyc9Po4C/cUYhYvgBXrpn2DaTyhdiPUSjxKCzx1LFOf17zhhgnooNKhDiLaIX7bi4dbE
gxeNKQOl7FEBrsM/cQ1R1zA9YzBOCQ2WAoCZo0W5Hhq3W5qk4TUfDs5BBGWXKam+YyOC
EXDOn4KeN5oECdqWC0xEiW0rztKS2lgfldDsyflqnc16jzwkiKTCw0i1PcQyeQYt8+gu
SPNRGNf20PqClB04nQ4zz17qeBoEYmN4xThTmjXogoIg/SV8osxjdlfh3fujhzKWSDoP
7X5Q==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;
s=arc-20160816;
h=list-id:precedence:content-transfer-encoding:mime-version
:references:in-reply-to:message-id:date:subject:cc:to:from;
bh=UKiBBbOCLnHsDzWiXpKTMlT51P3bnvkoDadIhz3InCE=;
b=odQ3N6j7hAOzuaAZV97Ywf4c07mdANIT9YBeevxN0nepDSF0IuSzgGF7zTcCTRuWhI
dLIUf8H1b7nXTX1apNn1qC6pAh2plL8cw9lXH4vMIFmSZ6isxVtd2h/ojTYicbm/yu6O
dU7iQHxQ49VD3pj3vEa3uBN/Z+yJd1kbeEo9G2diF7RtMpNeX82nC63rDRUa3VH/01am
pIgIQe9je8aM16fkcgX3gE8QOuQsjwGIJimea/MnthTW+Fs3pY+8Ty5zze5HbjuYF+FH
Clk7aFLxNLX2f+/Cww+k+EVWU/nETE6gBgEiRvlmu/H7q8OYZP95ejEviMrYKPF6eCFE
fG/A==
ARC-Authentication-Results: i=1; mx.google.com;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com
Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20])
by mx.google.com with ESMTP id
a64-20020a639043000000b0050be02f0893si771348pge.697.2023.03.15.02.51.00;
Wed, 15 Mar 2023 02:51:13 -0700 (PDT)
Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
client-ip=2620:137:e000::1:20;
Authentication-Results: mx.google.com;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S232125AbjCOJoM (ORCPT <rfc822;realc9580@gmail.com> + 99 others);
Wed, 15 Mar 2023 05:44:12 -0400
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54512 "EHLO
lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
with ESMTP id S232088AbjCOJn7 (ORCPT
<rfc822;linux-kernel@vger.kernel.org>);
Wed, 15 Mar 2023 05:43:59 -0400
Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187])
by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 42C5D60D65;
Wed, 15 Mar 2023 02:43:47 -0700 (PDT)
Received: from canpemm500009.china.huawei.com (unknown [172.30.72.56])
by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4Pc56M2svSzrT4m;
Wed, 15 Mar 2023 17:42:51 +0800 (CST)
Received: from localhost.localdomain (10.50.163.32) by
canpemm500009.china.huawei.com (7.192.105.203) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
15.1.2507.21; Wed, 15 Mar 2023 17:43:44 +0800
From: Yicong Yang <yangyicong@huawei.com>
To: <mathieu.poirier@linaro.org>, <suzuki.poulose@arm.com>,
<jonathan.cameron@huawei.com>, <corbet@lwn.net>,
<linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>
CC: <alexander.shishkin@linux.intel.com>, <helgaas@kernel.org>,
<linux-pci@vger.kernel.org>, <prime.zeng@huawei.com>,
<linuxarm@huawei.com>
Subject: [PATCH 1/4] hwtracing: hisi_ptt: Make cpumask only present online
CPUs
Date: Wed, 15 Mar 2023 17:43:13 +0800
Message-ID: <20230315094316.26772-2-yangyicong@huawei.com>
X-Mailer: git-send-email 2.31.0
In-Reply-To: <20230315094316.26772-1-yangyicong@huawei.com>
References: <20230315094316.26772-1-yangyicong@huawei.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7BIT
Content-Type: text/plain; charset=US-ASCII
X-Originating-IP: [10.50.163.32]
X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To
canpemm500009.china.huawei.com (7.192.105.203)
X-CFilter-Loop: Reflected
X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED,
SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?=
X-GMAIL-THRID: =?utf-8?q?1760426850855881970?=
X-GMAIL-MSGID: =?utf-8?q?1760426850855881970?=
|
| Series |
Improve PTT filter interface and some fixes
|
|
Commit Message
Yicong Yang
March 15, 2023, 9:43 a.m. UTC
From: Yicong Yang <yangyicong@hisilicon.com> perf will try to start PTT trace on every CPU presented in cpumask sysfs attribute and it will fail to start on offline CPUs(see the comments in perf_event_open()). But the driver is using cpumask_of_node() to export the available cpumask which may include offline CPUs and may fail the perf unintendedly. Fix this by only export the online CPUs of the node. Fixes: ff0de066b463 ("hwtracing: hisi_ptt: Add trace function support for HiSilicon PCIe Tune and Trace device") Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> --- drivers/hwtracing/ptt/hisi_ptt.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
Comments
On Wed, 15 Mar 2023 17:43:13 +0800 Yicong Yang <yangyicong@huawei.com> wrote: > From: Yicong Yang <yangyicong@hisilicon.com> > > perf will try to start PTT trace on every CPU presented in cpumask sysfs > attribute and it will fail to start on offline CPUs(see the comments in > perf_event_open()). But the driver is using cpumask_of_node() to export > the available cpumask which may include offline CPUs and may fail the > perf unintendedly. Fix this by only export the online CPUs of the node. There isn't clear documentation that I can find for cpumask_of_node() and chasing through on arm64 (which is what we care about for this driver) it's maintained via numa_add_cpu() numa_remove_cpu() Those are called in arch/arm64/kernel/smp.c in locations that are closely coupled with set_cpu_online(cpu, XXX); https://elixir.bootlin.com/linux/v6.3-rc4/source/arch/arm64/kernel/smp.c#L246 https://elixir.bootlin.com/linux/v6.3-rc4/source/arch/arm64/kernel/smp.c#L303 Now there are races when the two might not be in sync but in this case we are just exposing the result to userspace, so chances of a race after this sysfs attribute has been read seems much higher to me and I don't think we can do anything about that. Is there another path that I'm missing where online and node masks are out of sync? Jonathan > > Fixes: ff0de066b463 ("hwtracing: hisi_ptt: Add trace function support for HiSilicon PCIe Tune and Trace device") > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/hwtracing/ptt/hisi_ptt.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c > index 30f1525639b5..0a10c7ec46ad 100644 > --- a/drivers/hwtracing/ptt/hisi_ptt.c > +++ b/drivers/hwtracing/ptt/hisi_ptt.c > @@ -487,9 +487,18 @@ static ssize_t cpumask_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > struct hisi_ptt *hisi_ptt = to_hisi_ptt(dev_get_drvdata(dev)); > - const cpumask_t *cpumask = cpumask_of_node(dev_to_node(&hisi_ptt->pdev->dev)); > + cpumask_var_t mask; > + ssize_t n; > > - return cpumap_print_to_pagebuf(true, buf, cpumask); > + if (!alloc_cpumask_var(&mask, GFP_KERNEL)) > + return 0; > + > + cpumask_and(mask, cpumask_of_node(dev_to_node(&hisi_ptt->pdev->dev)), > + cpu_online_mask); > + n = cpumap_print_to_pagebuf(true, buf, mask); > + free_cpumask_var(mask); > + > + return n; > } > static DEVICE_ATTR_RO(cpumask); >
On 2023/3/29 0:24, Jonathan Cameron wrote: > On Wed, 15 Mar 2023 17:43:13 +0800 > Yicong Yang <yangyicong@huawei.com> wrote: > >> From: Yicong Yang <yangyicong@hisilicon.com> >> >> perf will try to start PTT trace on every CPU presented in cpumask sysfs >> attribute and it will fail to start on offline CPUs(see the comments in >> perf_event_open()). But the driver is using cpumask_of_node() to export >> the available cpumask which may include offline CPUs and may fail the >> perf unintendedly. Fix this by only export the online CPUs of the node. > > There isn't clear documentation that I can find for cpumask_of_node() > and chasing through on arm64 (which is what we care about for this driver) > it's maintained via numa_add_cpu() numa_remove_cpu() > Those are called in arch/arm64/kernel/smp.c in locations that are closely coupled > with set_cpu_online(cpu, XXX); > https://elixir.bootlin.com/linux/v6.3-rc4/source/arch/arm64/kernel/smp.c#L246 > https://elixir.bootlin.com/linux/v6.3-rc4/source/arch/arm64/kernel/smp.c#L303 > > Now there are races when the two might not be in sync but in this case > we are just exposing the result to userspace, so chances of a race > after this sysfs attribute has been read seems much higher to me and > I don't think we can do anything about that. > > Is there another path that I'm missing where online and node masks are out > of sync? > maybe no. This patch maybe incorrect and I need more investigation, so let's me drop it from the series. Tested and everything seems fine now. I found this problem and referred to commit 064f0e9302af ("mm: only display online cpus of the numa node") which might be the same problem. But seems unnecessary that cpumask_of_node() already include online CPUs only. Thanks. > Jonathan > > >> >> Fixes: ff0de066b463 ("hwtracing: hisi_ptt: Add trace function support for HiSilicon PCIe Tune and Trace device") >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > >> --- >> drivers/hwtracing/ptt/hisi_ptt.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c >> index 30f1525639b5..0a10c7ec46ad 100644 >> --- a/drivers/hwtracing/ptt/hisi_ptt.c >> +++ b/drivers/hwtracing/ptt/hisi_ptt.c >> @@ -487,9 +487,18 @@ static ssize_t cpumask_show(struct device *dev, struct device_attribute *attr, >> char *buf) >> { >> struct hisi_ptt *hisi_ptt = to_hisi_ptt(dev_get_drvdata(dev)); >> - const cpumask_t *cpumask = cpumask_of_node(dev_to_node(&hisi_ptt->pdev->dev)); >> + cpumask_var_t mask; >> + ssize_t n; >> >> - return cpumap_print_to_pagebuf(true, buf, cpumask); >> + if (!alloc_cpumask_var(&mask, GFP_KERNEL)) >> + return 0; >> + >> + cpumask_and(mask, cpumask_of_node(dev_to_node(&hisi_ptt->pdev->dev)), >> + cpu_online_mask); >> + n = cpumap_print_to_pagebuf(true, buf, mask); >> + free_cpumask_var(mask); >> + >> + return n; >> } >> static DEVICE_ATTR_RO(cpumask); >> > > . >
On Thu, 30 Mar 2023 11:53:14 +0800 Yicong Yang <yangyicong@huawei.com> wrote: > On 2023/3/29 0:24, Jonathan Cameron wrote: > > On Wed, 15 Mar 2023 17:43:13 +0800 > > Yicong Yang <yangyicong@huawei.com> wrote: > > > >> From: Yicong Yang <yangyicong@hisilicon.com> > >> > >> perf will try to start PTT trace on every CPU presented in cpumask sysfs > >> attribute and it will fail to start on offline CPUs(see the comments in > >> perf_event_open()). But the driver is using cpumask_of_node() to export > >> the available cpumask which may include offline CPUs and may fail the > >> perf unintendedly. Fix this by only export the online CPUs of the node. > > > > There isn't clear documentation that I can find for cpumask_of_node() > > and chasing through on arm64 (which is what we care about for this driver) > > it's maintained via numa_add_cpu() numa_remove_cpu() > > Those are called in arch/arm64/kernel/smp.c in locations that are closely coupled > > with set_cpu_online(cpu, XXX); > > https://elixir.bootlin.com/linux/v6.3-rc4/source/arch/arm64/kernel/smp.c#L246 > > https://elixir.bootlin.com/linux/v6.3-rc4/source/arch/arm64/kernel/smp.c#L303 > > > > Now there are races when the two might not be in sync but in this case > > we are just exposing the result to userspace, so chances of a race > > after this sysfs attribute has been read seems much higher to me and > > I don't think we can do anything about that. > > > > Is there another path that I'm missing where online and node masks are out > > of sync? > > > > maybe no. This patch maybe incorrect and I need more investigation, so let's me > drop it from the series. Tested and everything seems fine now. > > I found this problem and referred to commit 064f0e9302af ("mm: only display online cpus of the numa node") > which might be the same problem. But seems unnecessary that cpumask_of_node() > already include online CPUs only. Seems it was fixed up for arm64 in 7f954aa1a ("arm64: smp: remove cpu and numa topology information when hotplugging out CPMU") If we could audit all the other architectures it would be great to document the properties of this cpmuask and possibly simplify the code in the path you highlight above (assuming no race conditions etc) Jonathan > > Thanks. > > > Jonathan > > > > > >> > >> Fixes: ff0de066b463 ("hwtracing: hisi_ptt: Add trace function support for HiSilicon PCIe Tune and Trace device") > >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > > > >> --- > >> drivers/hwtracing/ptt/hisi_ptt.c | 13 +++++++++++-- > >> 1 file changed, 11 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c > >> index 30f1525639b5..0a10c7ec46ad 100644 > >> --- a/drivers/hwtracing/ptt/hisi_ptt.c > >> +++ b/drivers/hwtracing/ptt/hisi_ptt.c > >> @@ -487,9 +487,18 @@ static ssize_t cpumask_show(struct device *dev, struct device_attribute *attr, > >> char *buf) > >> { > >> struct hisi_ptt *hisi_ptt = to_hisi_ptt(dev_get_drvdata(dev)); > >> - const cpumask_t *cpumask = cpumask_of_node(dev_to_node(&hisi_ptt->pdev->dev)); > >> + cpumask_var_t mask; > >> + ssize_t n; > >> > >> - return cpumap_print_to_pagebuf(true, buf, cpumask); > >> + if (!alloc_cpumask_var(&mask, GFP_KERNEL)) > >> + return 0; > >> + > >> + cpumask_and(mask, cpumask_of_node(dev_to_node(&hisi_ptt->pdev->dev)), > >> + cpu_online_mask); > >> + n = cpumap_print_to_pagebuf(true, buf, mask); > >> + free_cpumask_var(mask); > >> + > >> + return n; > >> } > >> static DEVICE_ATTR_RO(cpumask); > >> > > > > . > >
diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c index 30f1525639b5..0a10c7ec46ad 100644 --- a/drivers/hwtracing/ptt/hisi_ptt.c +++ b/drivers/hwtracing/ptt/hisi_ptt.c @@ -487,9 +487,18 @@ static ssize_t cpumask_show(struct device *dev, struct device_attribute *attr, char *buf) { struct hisi_ptt *hisi_ptt = to_hisi_ptt(dev_get_drvdata(dev)); - const cpumask_t *cpumask = cpumask_of_node(dev_to_node(&hisi_ptt->pdev->dev)); + cpumask_var_t mask; + ssize_t n; - return cpumap_print_to_pagebuf(true, buf, cpumask); + if (!alloc_cpumask_var(&mask, GFP_KERNEL)) + return 0; + + cpumask_and(mask, cpumask_of_node(dev_to_node(&hisi_ptt->pdev->dev)), + cpu_online_mask); + n = cpumap_print_to_pagebuf(true, buf, mask); + free_cpumask_var(mask); + + return n; } static DEVICE_ATTR_RO(cpumask);