Message ID | 166604559954.5345.14619487558472213422.stgit@bmoger-ubuntu |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp1669615wrs; Mon, 17 Oct 2022 15:29:37 -0700 (PDT) X-Google-Smtp-Source: AMsMyM5XZ7Ngad6Ho9pJtoIhvfwq7xsF74mr6wvdfukUYLWr1KXy2HbKZnxs0pe0qcq+a2HUcyJQ X-Received: by 2002:a05:6a00:bc7:b0:562:d2e5:adfd with SMTP id x7-20020a056a000bc700b00562d2e5adfdmr31768pfu.4.1666045777731; Mon, 17 Oct 2022 15:29:37 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1666045777; cv=pass; d=google.com; s=arc-20160816; b=nM6WHquheSldR2Tb5aQwJwgJGjT8897IYbvuPbHlI30dSCD7jd1aoOFgWml+BPDldz a1DaUjMv308Y40MkHquHRAqj/Bq6uR9bLjmqRfUvlT13vX3TpnBoRUYx80omWkdfGE1/ gxxbjmzIGLTHNWdQTJNbGOOVIhCP7Ww2fdxMI5VQgB8vg8hkDL+L1ImVVsZb3i6LJQH+ 6bT2ej9jf2iFGY5vNjvr+3fk4oxezvkTGO0PKnKjeMQe9S6AS7jiGbXBEQ7xrybZd/uD t2dGqlwvTRVAItHBszmq0PJJR0SsnXJfsDPXJW8aPJhNAiWfzbYMVQQGEOQGp8ChyPyU Vo/w== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :user-agent:references:in-reply-to:message-id:date:cc:to:from :subject:dkim-signature; bh=JfBGki9HRVqT7przs0JHfyPLhzpAd1Zf8oMs+cVxOWY=; b=Y8bXsTwZ/pyXi1WrD5PfdK7IC3YbTuVmn9URgTjrvhDzn0Ml0+SdK1v1Bc6slNR7vL BW1gJlNSE1K/yzXtdfNO39g0bH4RysNupK3g7o9WTMLY8uO7B91hpDSaBcRhoGoBEzQ+ hjB+MY46OzzTjYlOFSBqxPPqTCnwamfm+WpBVb+a5M7iJkrcr3ulfY5K7+xIRgE5tUFO M7EGilV04Q9ZOjaDH+8QzjskcfEK8qoIH6SkjllEJD84BiRgQ0M5IZDkCYZVthdssudc dW0YM8zquS/oCUs66RHev0q3YgFKxoZBJjfwK0qakirvK9E87AcgSjLEpVWPn/gQgR7q vVLA== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@amd.com header.s=selector1 header.b=KYxctwft; arc=pass (i=1 spf=pass spfdomain=amd.com dmarc=pass fromdomain=amd.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=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=amd.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id s10-20020a056a00178a00b00565a581ecc0si14660477pfg.11.2022.10.17.15.29.24; Mon, 17 Oct 2022 15:29:37 -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; dkim=pass header.i=@amd.com header.s=selector1 header.b=KYxctwft; arc=pass (i=1 spf=pass spfdomain=amd.com dmarc=pass fromdomain=amd.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=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=amd.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230257AbiJQW1Q (ORCPT <rfc822;kernel.ruili@gmail.com> + 99 others); Mon, 17 Oct 2022 18:27:16 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49502 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230239AbiJQW1D (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 17 Oct 2022 18:27:03 -0400 Received: from NAM11-BN8-obe.outbound.protection.outlook.com (mail-bn8nam11on2040.outbound.protection.outlook.com [40.107.236.40]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0247F816A9; Mon, 17 Oct 2022 15:26:47 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=iF0xC5v3yn/JKRXn8RoZLCAdaf858ar/mOZ46RcSyrHOgIC07Tg+GEHaYf4tEq9JwGeh6kdAxaPrzfSKvoilvPkKIQxA3obInD8zFXuVa263i/XKJgV+mLe1tsieBD6S1CdImvM1bhGgVayVO9lBkoj0LjfQ+zdqvUQdEdZIanJ5Q1J7wX2viuM+lgTHkEhcn57LNMeyasLfNZF51ozhI+uoXClEWtNPk8MmSTp6sqMNC99vUP18ihEWqVauH8mBIE4ZPNQd6VUjJzgSaew+z8FaBoVbo/vvqN+pe55nDB1sKbfI/1+xnSyVksJ6JcvB6kV69FEBfwgDU7PqbtWHFw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=JfBGki9HRVqT7przs0JHfyPLhzpAd1Zf8oMs+cVxOWY=; b=YKVW7Zed6VP966zclUJU8aW/c3bd74ottFBP9erChrRRYVcanGx15xt8hw8UQYv40Y80gu9XSPTzjn5z8D7ggU8bWBoPjo5Kxl4te2fJl1Y6cs9f8Nf/YQUkMI+HXnztrY4eD9wHxh0OVhPh4BuQTssjztLjnIFRWuSb2GlR+cuiOGrCMoud1W+hGY9Jv30kZAHHLRZRjKgci3BqZrbITKSvnow4GleVtASyIno1OMLm/g09nddJ+QqwcGQwQtMsLPYlvuRjUio0iS6zyDtljd5PwpFPeIDpdOxDGyDLcZkhuJ7cZTSdhbQ65NuEO/tEmHOBzGhVye75PFyRrpQ1qA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=alien8.de smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=JfBGki9HRVqT7przs0JHfyPLhzpAd1Zf8oMs+cVxOWY=; b=KYxctwftGgt8AtsG0Ixf3ASEXurtAFmJMakwIsUCF9uGI2rbkntBeSDLHWtB0LQAyS538Ep2ugcnc5aWONusp7rg1oTk9v+Gt2AZ3NidcOsdj9IsRpUqB/QnHLHhEbhL6f6KzL7YMbpnwhM06N7fXOR2YvEiKUOgmGHMHyK1+Dg= Received: from BN6PR17CA0028.namprd17.prod.outlook.com (2603:10b6:405:75::17) by CH2PR12MB4970.namprd12.prod.outlook.com (2603:10b6:610:67::9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5723.32; Mon, 17 Oct 2022 22:26:43 +0000 Received: from BN8NAM11FT021.eop-nam11.prod.protection.outlook.com (2603:10b6:405:75:cafe::2b) by BN6PR17CA0028.outlook.office365.com (2603:10b6:405:75::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5723.26 via Frontend Transport; Mon, 17 Oct 2022 22:26:42 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 165.204.84.17) smtp.mailfrom=amd.com; dkim=none (message not signed) header.d=none;dmarc=pass action=none header.from=amd.com; Received-SPF: Pass (protection.outlook.com: domain of amd.com designates 165.204.84.17 as permitted sender) receiver=protection.outlook.com; client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C Received: from SATLEXMB04.amd.com (165.204.84.17) by BN8NAM11FT021.mail.protection.outlook.com (10.13.177.114) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.20.5723.20 via Frontend Transport; Mon, 17 Oct 2022 22:26:41 +0000 Received: from [127.0.1.1] (10.180.168.240) by SATLEXMB04.amd.com (10.181.40.145) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Mon, 17 Oct 2022 17:26:40 -0500 Subject: [PATCH v7 05/12] x86/resctrl: Detect and configure Slow Memory Bandwidth allocation From: Babu Moger <babu.moger@amd.com> To: <corbet@lwn.net>, <reinette.chatre@intel.com>, <tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de> CC: <fenghua.yu@intel.com>, <dave.hansen@linux.intel.com>, <x86@kernel.org>, <hpa@zytor.com>, <paulmck@kernel.org>, <akpm@linux-foundation.org>, <quic_neeraju@quicinc.com>, <rdunlap@infradead.org>, <damien.lemoal@opensource.wdc.com>, <songmuchun@bytedance.com>, <peterz@infradead.org>, <jpoimboe@kernel.org>, <pbonzini@redhat.com>, <babu.moger@amd.com>, <chang.seok.bae@intel.com>, <pawan.kumar.gupta@linux.intel.com>, <jmattson@google.com>, <daniel.sneddon@linux.intel.com>, <sandipan.das@amd.com>, <tony.luck@intel.com>, <james.morse@arm.com>, <linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <bagasdotme@gmail.com>, <eranian@google.com> Date: Mon, 17 Oct 2022 17:26:39 -0500 Message-ID: <166604559954.5345.14619487558472213422.stgit@bmoger-ubuntu> In-Reply-To: <166604543832.5345.9696970469830919982.stgit@bmoger-ubuntu> References: <166604543832.5345.9696970469830919982.stgit@bmoger-ubuntu> User-Agent: StGit/1.1.dev103+g5369f4c MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-Originating-IP: [10.180.168.240] X-ClientProxiedBy: SATLEXMB03.amd.com (10.181.40.144) To SATLEXMB04.amd.com (10.181.40.145) X-EOPAttributedMessage: 0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: BN8NAM11FT021:EE_|CH2PR12MB4970:EE_ X-MS-Office365-Filtering-Correlation-Id: 999886e7-6780-4f2e-537b-08dab08eaa8e X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: DerA3sPrPoqXYwvZb4jEBDJ0zWIodrVyhONBgP6vV2X8OsNu1ZB+qe6mhKIMuQlPOlISV21mmXlU93KnLd+OMbT1FvwhOpK4hXb8+RA+GTVYbmLJaHLF1qKs3m82VgxZOMN1X+xaMdgAsA7dXH8fW/TJG2HIAfB1ftqyOh23jge9Fgl5yOyl179I5GW+t+xQ+5koLojAOiizaXMsT2mMFYwurqbfm+ADa/rQC001FJIQe/lDgYETH9XcbMJL9h5B1lCi0UknjOwFndaJ5/ldDA+PdXPYlw7wL75aUOULXnEEdWGqq+is1NQ8VUmLl5KPZ32Ipk0VaBM7vhXcAa508UAHQ00dHDa58wSKYAVvyx/ThSIy7UjbjCp9OSedZG285uZ3tiX2KME/EfMksP+pxhP+OnqA2MfBR9BIW3S8GDNPL++m0ZnqctvYf3cZq3NqeLVOQRQJzOCryKoU5NKa1vkMZIHi0PqvCwjTxPFTd9f2d2I+ZBPcXYpourWq3qfj8Q4pb8YdyLMq17ZteuM8mPjUNe16vTzZX72l88CQ0Ot46tvpEeZFvFc6n52qgnT1w7RIZzTQzofhnGXNMM/b/8z3BmPuKLXzgkjOcdhkPt12HBOEIZTMs/q4GmcbRnQPOp574djbDugJMeIop5g8GnQA3I1KY6/F5cGUbA44OTXSztB37yjPJLS8jvkvadGkWHAHb9He3ZVCudHWPqAa/Nf3m5RhiSsUPConvRPg4BWAw9IIsBxOxX1KEYT3Ef5LvZ7grp+Nu65x6GPrQc6LUjfjybfu1f01lqegi2RXGd+k+dJQNpTWgx0t/GpQH5PH/PbMor7+fYmRjGlzvNHE3Q== X-Forefront-Antispam-Report: CIP:165.204.84.17;CTRY:US;LANG:en;SCL:1;SRV:;IPV:CAL;SFV:NSPM;H:SATLEXMB04.amd.com;PTR:InfoDomainNonexistent;CAT:NONE;SFS:(13230022)(7916004)(4636009)(376002)(136003)(39860400002)(346002)(396003)(451199015)(36840700001)(46966006)(40470700004)(9686003)(26005)(478600001)(36860700001)(83380400001)(336012)(44832011)(186003)(16526019)(2906002)(5660300002)(7416002)(40480700001)(40460700003)(16576012)(33716001)(82310400005)(54906003)(110136005)(316002)(4326008)(8676002)(8936002)(41300700001)(70206006)(70586007)(47076005)(426003)(86362001)(103116003)(81166007)(356005)(82740400003)(71626007)(36900700001);DIR:OUT;SFP:1101; X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Oct 2022 22:26:41.9176 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 999886e7-6780-4f2e-537b-08dab08eaa8e X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com] X-MS-Exchange-CrossTenant-AuthSource: BN8NAM11FT021.eop-nam11.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB4970 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,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?1746975617421028724?= X-GMAIL-MSGID: =?utf-8?q?1746975617421028724?= |
Series |
x86/resctrl: Support for AMD QoS new features
|
|
Commit Message
Moger, Babu
Oct. 17, 2022, 10:26 p.m. UTC
The QoS slow memory configuration details are available via
CPUID_Fn80000020_EDX_x02. Detect the available details and
initialize the rest to defaults.
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
arch/x86/kernel/cpu/resctrl/core.c | 29 +++++++++++++++++++++++++++--
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 2 +-
arch/x86/kernel/cpu/resctrl/internal.h | 1 +
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 16 ++++++++++------
4 files changed, 39 insertions(+), 9 deletions(-)
Comments
Hi Babu, Nitpick in Subject ... "allocation" -> "Allocation"? On 10/17/2022 3:26 PM, Babu Moger wrote: ... > @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp) > > list_for_each_entry(s, &resctrl_schema_all, list) { > r = s->res; > - if (r->rid == RDT_RESOURCE_MBA) { > + if (r->rid == RDT_RESOURCE_MBA || > + r->rid == RDT_RESOURCE_SMBA) { > rdtgroup_init_mba(r, rdtgrp->closid); > if (is_mba_sc(r)) > continue; The above hunk and the ones that follow are unexpected. Note that the software controller, when resctrl is mounted with "mba_MBps", is only supported by RDT_RESOURCE_MBA. At this time this really is hard coded all over the place, for example: static int set_mba_sc(bool mba_sc) { struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl; ... } Since SMBA hardcodes "delay_linear = false" I do not expect it to support the software controller ... but these hunks appear to treat SMBA as though it does. It is the "MBA software controller", not "SMBA software controller". Why does it check above if the MBA software controller is enabled on SMBA? > @@ -3287,7 +3289,8 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d) > { > lockdep_assert_held(&rdtgroup_mutex); > > - if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA) > + if (supports_mba_mbps() && > + (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_SMBA)) > mba_sc_domain_destroy(r, d); > > if (!r->mon_capable) > @@ -3354,8 +3357,9 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d) > > lockdep_assert_held(&rdtgroup_mutex); > > - if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA) > - /* RDT_RESOURCE_MBA is never mon_capable */ > + if (supports_mba_mbps() && > + (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_MBA)) > + /* RDT_RESOURCE_MBA (or SMBA) is never mon_capable */ What does this change do? Did you mean to add a r->rid == RDT_RESOURCE_SMBA check? > return mba_sc_domain_allocate(r, d); > > if (!r->mon_capable) > > Why are the MBA software controller resources allocated/destroyed for a SMBA resource? If you want to support the software controller for SMBA then there are a lot of other changes missing. Reinette
Hi Reinette, On 10/25/22 18:43, Reinette Chatre wrote: > Hi Babu, > > Nitpick in Subject ... "allocation" -> "Allocation"? Sure. > > On 10/17/2022 3:26 PM, Babu Moger wrote: > > ... > >> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp) >> >> list_for_each_entry(s, &resctrl_schema_all, list) { >> r = s->res; >> - if (r->rid == RDT_RESOURCE_MBA) { >> + if (r->rid == RDT_RESOURCE_MBA || >> + r->rid == RDT_RESOURCE_SMBA) { >> rdtgroup_init_mba(r, rdtgrp->closid); >> if (is_mba_sc(r)) >> continue; > The above hunk and the ones that follow are unexpected. I am thinking the above check is required, It is updating the staged_config with default values. Right now, the default value for SMBA is same as MBA default value. So, I used this code to initialize. Did I miss something? > > Note that the software controller, when resctrl is mounted with "mba_MBps", is > only supported by RDT_RESOURCE_MBA. At this time this really is hard coded all > over the place, for example: > > static int set_mba_sc(bool mba_sc) > { > struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl; > ... > > } > > Since SMBA hardcodes "delay_linear = false" I do not expect it to support the software > controller ... but these hunks appear to treat SMBA as though it does. It is the "MBA software > controller", not "SMBA software controller". Why does it check above if the MBA software > controller is enabled on SMBA? There is no plan to support SMBA software controller. Yes, I think below checks are not required. > > >> @@ -3287,7 +3289,8 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d) >> { >> lockdep_assert_held(&rdtgroup_mutex); >> >> - if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA) >> + if (supports_mba_mbps() && >> + (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_SMBA)) >> mba_sc_domain_destroy(r, d); This check is not required. >> >> if (!r->mon_capable) >> @@ -3354,8 +3357,9 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d) >> >> lockdep_assert_held(&rdtgroup_mutex); >> >> - if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA) >> - /* RDT_RESOURCE_MBA is never mon_capable */ >> + if (supports_mba_mbps() && >> + (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_MBA)) >> + /* RDT_RESOURCE_MBA (or SMBA) is never mon_capable */ > What does this change do? Did you mean to add a r->rid == RDT_RESOURCE_SMBA check? Good catch. I meant r->rid == RDT_RESOURCE_SMBA. But this check is not required at all. > >> return mba_sc_domain_allocate(r, d); >> >> if (!r->mon_capable) >> >> > Why are the MBA software controller resources allocated/destroyed for a SMBA resource? If > you want to support the software controller for SMBA then there are a lot of other changes No..There is no plan to support software controller for SMBA. Thanks Babu
Hi Babu, On 10/26/2022 12:07 PM, Moger, Babu wrote: > On 10/25/22 18:43, Reinette Chatre wrote: >> On 10/17/2022 3:26 PM, Babu Moger wrote: >> >> ... >> >>> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp) >>> >>> list_for_each_entry(s, &resctrl_schema_all, list) { >>> r = s->res; >>> - if (r->rid == RDT_RESOURCE_MBA) { >>> + if (r->rid == RDT_RESOURCE_MBA || >>> + r->rid == RDT_RESOURCE_SMBA) { >>> rdtgroup_init_mba(r, rdtgrp->closid); >>> if (is_mba_sc(r)) >>> continue; >> The above hunk and the ones that follow are unexpected. > > I am thinking the above check is required, It is updating the > staged_config with default values. Right now, the default value for SMBA > is same as MBA default value. So, I used this code to initialize. > > Did I miss something? As I described in the following comments my concern is related to all the software controller code still executing for SMBA. Yes, in the above hunk SMBA would need (some of) rdtgroup_init_mba() ... but note that it contains software controller checks and in the above hunk its call is also followed by another software controller check. The software controller is just applicable to MBA and these checks have been isolated to the MBA resource. Using it for SMBA that does not support software controller at all is making the code harder to follow and sets this code up for future mistakes. I think it would make the code easier to understand if this is made very clear that software controller is not applicable to SMBA at all instead of repurposing these flows. >> Note that the software controller, when resctrl is mounted with "mba_MBps", is >> only supported by RDT_RESOURCE_MBA. At this time this really is hard coded all >> over the place, for example: >> >> static int set_mba_sc(bool mba_sc) >> { >> struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl; >> ... >> >> } >> >> Since SMBA hardcodes "delay_linear = false" I do not expect it to support the software >> controller ... but these hunks appear to treat SMBA as though it does. It is the "MBA software >> controller", not "SMBA software controller". Why does it check above if the MBA software >> controller is enabled on SMBA? > > There is no plan to support SMBA software controller. Yes, I think below > checks are not required. Thank you for clarifying. Having the code reflect that clearly would make everything easier to understand and maintain. Reinette
Hi Reinette, On 10/26/22 15:23, Reinette Chatre wrote: > Hi Babu, > > On 10/26/2022 12:07 PM, Moger, Babu wrote: >> On 10/25/22 18:43, Reinette Chatre wrote: >>> On 10/17/2022 3:26 PM, Babu Moger wrote: >>> >>> ... >>> >>>> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp) >>>> >>>> list_for_each_entry(s, &resctrl_schema_all, list) { >>>> r = s->res; >>>> - if (r->rid == RDT_RESOURCE_MBA) { >>>> + if (r->rid == RDT_RESOURCE_MBA || >>>> + r->rid == RDT_RESOURCE_SMBA) { >>>> rdtgroup_init_mba(r, rdtgrp->closid); >>>> if (is_mba_sc(r)) >>>> continue; >>> The above hunk and the ones that follow are unexpected. >> I am thinking the above check is required, It is updating the >> staged_config with default values. Right now, the default value for SMBA >> is same as MBA default value. So, I used this code to initialize. >> >> Did I miss something? > As I described in the following comments my concern is related to all the > software controller code still executing for SMBA. Yes, in the above hunk > SMBA would need (some of) rdtgroup_init_mba() ... but note that it contains > software controller checks and in the above hunk its call is also followed > by another software controller check. > > The software controller is just applicable to MBA and these checks have been > isolated to the MBA resource. Using it for SMBA that does not support > software controller at all is making the code harder to follow and sets this > code up for future mistakes. I think it would make the code easier to understand > if this is made very clear that software controller is not applicable to SMBA at > all instead of repurposing these flows. Yes. Understood. How about this? I feel this is much more cleaner. diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c index e5a48f05e787..d91a6a513681 100644 --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c @@ -2845,16 +2845,18 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp) list_for_each_entry(s, &resctrl_schema_all, list) { r = s->res; - if (r->rid == RDT_RESOURCE_MBA) { + if (r->rid == RDT_RESOURCE_MBA || + r->rid == RDT_RESOURCE_SMBA) { rdtgroup_init_mba(r, rdtgrp->closid); - if (is_mba_sc(r)) - continue; } else { ret = rdtgroup_init_cat(s, rdtgrp->closid); if (ret < 0) return ret; } + if (is_mba_sc(r)) + continue; + ret = resctrl_arch_update_domains(r, rdtgrp->closid); if (ret < 0) { rdt_last_cmd_puts("Failed to initialize allocations\n"); Thanks Babu > >>> Note that the software controller, when resctrl is mounted with "mba_MBps", is >>> only supported by RDT_RESOURCE_MBA. At this time this really is hard coded all >>> over the place, for example: >>> >>> static int set_mba_sc(bool mba_sc) >>> { >>> struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl; >>> ... >>> >>> } >>> >>> Since SMBA hardcodes "delay_linear = false" I do not expect it to support the software >>> controller ... but these hunks appear to treat SMBA as though it does. It is the "MBA software >>> controller", not "SMBA software controller". Why does it check above if the MBA software >>> controller is enabled on SMBA? >> There is no plan to support SMBA software controller. Yes, I think below >> checks are not required. > Thank you for clarifying. Having the code reflect that clearly would make everything > easier to understand and maintain. > > Reinette >
Hi Babu, On 10/27/2022 8:30 AM, Moger, Babu wrote: > On 10/26/22 15:23, Reinette Chatre wrote: >> On 10/26/2022 12:07 PM, Moger, Babu wrote: >>> On 10/25/22 18:43, Reinette Chatre wrote: >>>> On 10/17/2022 3:26 PM, Babu Moger wrote: >>>> >>>> ... >>>> >>>>> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp) >>>>> >>>>> list_for_each_entry(s, &resctrl_schema_all, list) { >>>>> r = s->res; >>>>> - if (r->rid == RDT_RESOURCE_MBA) { >>>>> + if (r->rid == RDT_RESOURCE_MBA || >>>>> + r->rid == RDT_RESOURCE_SMBA) { >>>>> rdtgroup_init_mba(r, rdtgrp->closid); >>>>> if (is_mba_sc(r)) >>>>> continue; >>>> The above hunk and the ones that follow are unexpected. >>> I am thinking the above check is required, It is updating the >>> staged_config with default values. Right now, the default value for SMBA >>> is same as MBA default value. So, I used this code to initialize. >>> >>> Did I miss something? >> As I described in the following comments my concern is related to all the >> software controller code still executing for SMBA. Yes, in the above hunk >> SMBA would need (some of) rdtgroup_init_mba() ... but note that it contains >> software controller checks and in the above hunk its call is also followed >> by another software controller check. >> >> The software controller is just applicable to MBA and these checks have been >> isolated to the MBA resource. Using it for SMBA that does not support >> software controller at all is making the code harder to follow and sets this >> code up for future mistakes. I think it would make the code easier to understand >> if this is made very clear that software controller is not applicable to SMBA at >> all instead of repurposing these flows. > > Yes. Understood. How about this? I feel this is much more cleaner. > > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > index e5a48f05e787..d91a6a513681 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -2845,16 +2845,18 @@ static int rdtgroup_init_alloc(struct rdtgroup > *rdtgrp) > > list_for_each_entry(s, &resctrl_schema_all, list) { > r = s->res; > - if (r->rid == RDT_RESOURCE_MBA) { > + if (r->rid == RDT_RESOURCE_MBA || > + r->rid == RDT_RESOURCE_SMBA) { > rdtgroup_init_mba(r, rdtgrp->closid); > - if (is_mba_sc(r)) > - continue; > } else { > ret = rdtgroup_init_cat(s, rdtgrp->closid); > if (ret < 0) > return ret; > } > > + if (is_mba_sc(r)) > + continue; > + > ret = resctrl_arch_update_domains(r, rdtgrp->closid); > if (ret < 0) { > rdt_last_cmd_puts("Failed to initialize > allocations\n"); > I do not see how that move changes what is run in the SMBA case and it ignores the is_mba_sc() call within rdtgroup_init_mba(). How about making is_mba_sc() more robust in support of your original snippet? Something like: bool is_mba_sc(struct rdt_resource *r) { if (!r) return rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl.membw.mba_sc; if (r->rid != RDT_RESOURCE_MBA) return false; return r->membw.mba_sc; } Reinette
[AMD Official Use Only - General] Hi Reinette, > -----Original Message----- > From: Reinette Chatre <reinette.chatre@intel.com> > Sent: Thursday, October 27, 2022 1:38 PM > To: Moger, Babu <Babu.Moger@amd.com>; corbet@lwn.net; > tglx@linutronix.de; mingo@redhat.com; bp@alien8.de > Cc: fenghua.yu@intel.com; dave.hansen@linux.intel.com; x86@kernel.org; > hpa@zytor.com; paulmck@kernel.org; akpm@linux-foundation.org; > quic_neeraju@quicinc.com; rdunlap@infradead.org; > damien.lemoal@opensource.wdc.com; songmuchun@bytedance.com; > peterz@infradead.org; jpoimboe@kernel.org; pbonzini@redhat.com; > chang.seok.bae@intel.com; pawan.kumar.gupta@linux.intel.com; > jmattson@google.com; daniel.sneddon@linux.intel.com; Das1, Sandipan > <Sandipan.Das@amd.com>; tony.luck@intel.com; james.morse@arm.com; > linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; > bagasdotme@gmail.com; eranian@google.com > Subject: Re: [PATCH v7 05/12] x86/resctrl: Detect and configure Slow Memory > Bandwidth allocation > > Hi Babu, > > On 10/27/2022 8:30 AM, Moger, Babu wrote: > > On 10/26/22 15:23, Reinette Chatre wrote: > >> On 10/26/2022 12:07 PM, Moger, Babu wrote: > >>> On 10/25/22 18:43, Reinette Chatre wrote: > >>>> On 10/17/2022 3:26 PM, Babu Moger wrote: > >>>> > >>>> ... > >>>> > >>>>> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct > >>>>> rdtgroup *rdtgrp) > >>>>> > >>>>> list_for_each_entry(s, &resctrl_schema_all, list) { > >>>>> r = s->res; > >>>>> - if (r->rid == RDT_RESOURCE_MBA) { > >>>>> + if (r->rid == RDT_RESOURCE_MBA || > >>>>> + r->rid == RDT_RESOURCE_SMBA) { > >>>>> rdtgroup_init_mba(r, rdtgrp->closid); > >>>>> if (is_mba_sc(r)) > >>>>> continue; > >>>> The above hunk and the ones that follow are unexpected. > >>> I am thinking the above check is required, It is updating the > >>> staged_config with default values. Right now, the default value for > >>> SMBA is same as MBA default value. So, I used this code to initialize. > >>> > >>> Did I miss something? > >> As I described in the following comments my concern is related to all > >> the software controller code still executing for SMBA. Yes, in the > >> above hunk SMBA would need (some of) rdtgroup_init_mba() ... but note > >> that it contains software controller checks and in the above hunk its > >> call is also followed by another software controller check. > >> > >> The software controller is just applicable to MBA and these checks > >> have been isolated to the MBA resource. Using it for SMBA that does > >> not support software controller at all is making the code harder to > >> follow and sets this code up for future mistakes. I think it would > >> make the code easier to understand if this is made very clear that > >> software controller is not applicable to SMBA at all instead of repurposing > these flows. > > > > Yes. Understood. How about this? I feel this is much more cleaner. > > > > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > > b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > > index e5a48f05e787..d91a6a513681 100644 > > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > > @@ -2845,16 +2845,18 @@ static int rdtgroup_init_alloc(struct rdtgroup > > *rdtgrp) > > > > list_for_each_entry(s, &resctrl_schema_all, list) { > > r = s->res; > > - if (r->rid == RDT_RESOURCE_MBA) { > > + if (r->rid == RDT_RESOURCE_MBA || > > + r->rid == RDT_RESOURCE_SMBA) { > > rdtgroup_init_mba(r, rdtgrp->closid); > > - if (is_mba_sc(r)) > > - continue; > > } else { > > ret = rdtgroup_init_cat(s, rdtgrp->closid); > > if (ret < 0) > > return ret; > > } > > > > + if (is_mba_sc(r)) > > + continue; > > + > > ret = resctrl_arch_update_domains(r, rdtgrp->closid); > > if (ret < 0) { > > rdt_last_cmd_puts("Failed to initialize > > allocations\n"); > > > > I do not see how that move changes what is run in the SMBA case and it ignores > the > is_mba_sc() call within rdtgroup_init_mba(). How about making is_mba_sc() > more robust in support of your original snippet? > > Something like: > > bool is_mba_sc(struct rdt_resource *r) > { > if (!r) > return > rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl.membw.mba_sc; > > if (r->rid != RDT_RESOURCE_MBA) > return false; > > return r->membw.mba_sc; > } Yes. Sure. That should work. Thanks Babu
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c index c7561c613209..d79f494a4e91 100644 --- a/arch/x86/kernel/cpu/resctrl/core.c +++ b/arch/x86/kernel/cpu/resctrl/core.c @@ -231,9 +231,15 @@ static bool __rdt_get_mem_config_amd(struct rdt_resource *r) struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); union cpuid_0x10_3_eax eax; union cpuid_0x10_x_edx edx; - u32 ebx, ecx; + u32 ebx, ecx, subleaf; + + /* + * Query CPUID_Fn80000020_EDX_x01 for MBA and + * CPUID_Fn80000020_EDX_x02 for SMBA + */ + subleaf = (r->rid == RDT_RESOURCE_SMBA) ? 2 : 1; - cpuid_count(0x80000020, 1, &eax.full, &ebx, &ecx, &edx.full); + cpuid_count(0x80000020, subleaf, &eax.full, &ebx, &ecx, &edx.full); hw_res->num_closid = edx.split.cos_max + 1; r->default_ctrl = MAX_MBA_BW_AMD; @@ -756,6 +762,19 @@ static __init bool get_mem_config(void) return false; } +static __init bool get_slow_mem_config(void) +{ + struct rdt_hw_resource *hw_res = &rdt_resources_all[RDT_RESOURCE_SMBA]; + + if (!rdt_cpu_has(X86_FEATURE_SMBA)) + return false; + + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) + return __rdt_get_mem_config_amd(&hw_res->r_resctrl); + + return false; +} + static __init bool get_rdt_alloc_resources(void) { struct rdt_resource *r; @@ -786,6 +805,9 @@ static __init bool get_rdt_alloc_resources(void) if (get_mem_config()) ret = true; + if (get_slow_mem_config()) + ret = true; + return ret; } @@ -875,6 +897,9 @@ static __init void rdt_init_res_defs_amd(void) } else if (r->rid == RDT_RESOURCE_MBA) { hw_res->msr_base = MSR_IA32_MBA_BW_BASE; hw_res->msr_update = mba_wrmsr_amd; + } else if (r->rid == RDT_RESOURCE_SMBA) { + hw_res->msr_base = MSR_IA32_SMBA_BW_BASE; + hw_res->msr_update = mba_wrmsr_amd; } } } diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c index 1dafbdc5ac31..42e2ef6fbdb8 100644 --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c @@ -210,7 +210,7 @@ static int parse_line(char *line, struct resctrl_schema *s, unsigned long dom_id; if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP && - r->rid == RDT_RESOURCE_MBA) { + (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_SMBA)) { rdt_last_cmd_puts("Cannot pseudo-lock MBA resource\n"); return -EINVAL; } diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h index 43d9f6f5a931..16e3c6e03c79 100644 --- a/arch/x86/kernel/cpu/resctrl/internal.h +++ b/arch/x86/kernel/cpu/resctrl/internal.h @@ -14,6 +14,7 @@ #define MSR_IA32_L2_CBM_BASE 0xd10 #define MSR_IA32_MBA_THRTL_BASE 0xd50 #define MSR_IA32_MBA_BW_BASE 0xc0000200 +#define MSR_IA32_SMBA_BW_BASE 0xc0000280 #define MSR_IA32_QM_CTR 0x0c8e #define MSR_IA32_QM_EVTSEL 0x0c8d diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c index e5a48f05e787..1271fd1ae2f3 100644 --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c @@ -1213,7 +1213,7 @@ static bool rdtgroup_mode_test_exclusive(struct rdtgroup *rdtgrp) list_for_each_entry(s, &resctrl_schema_all, list) { r = s->res; - if (r->rid == RDT_RESOURCE_MBA) + if (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_SMBA) continue; has_cache = true; list_for_each_entry(d, &r->domains, list) { @@ -1402,7 +1402,8 @@ static int rdtgroup_size_show(struct kernfs_open_file *of, ctrl = resctrl_arch_get_config(r, d, closid, type); - if (r->rid == RDT_RESOURCE_MBA) + if (r->rid == RDT_RESOURCE_MBA || + r->rid == RDT_RESOURCE_SMBA) size = ctrl; else size = rdtgroup_cbm_to_size(r, d, ctrl); @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp) list_for_each_entry(s, &resctrl_schema_all, list) { r = s->res; - if (r->rid == RDT_RESOURCE_MBA) { + if (r->rid == RDT_RESOURCE_MBA || + r->rid == RDT_RESOURCE_SMBA) { rdtgroup_init_mba(r, rdtgrp->closid); if (is_mba_sc(r)) continue; @@ -3287,7 +3289,8 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d) { lockdep_assert_held(&rdtgroup_mutex); - if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA) + if (supports_mba_mbps() && + (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_SMBA)) mba_sc_domain_destroy(r, d); if (!r->mon_capable) @@ -3354,8 +3357,9 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d) lockdep_assert_held(&rdtgroup_mutex); - if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA) - /* RDT_RESOURCE_MBA is never mon_capable */ + if (supports_mba_mbps() && + (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_MBA)) + /* RDT_RESOURCE_MBA (or SMBA) is never mon_capable */ return mba_sc_domain_allocate(r, d); if (!r->mon_capable)