From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from szxga06-in.huawei.com (szxga06-in.huawei.com [45.249.212.32]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 48ED2192D80 for ; Tue, 24 Dec 2024 12:23:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=45.249.212.32 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735043016; cv=none; b=NvEMRdxeGjT8o2p1/3xSExCSVduFrg23xX9rpAQqc/PjwV3RHFARvyj1i8zUimwva/xqJ/Ej8PMkP5+ysscK+OQcgBA6VJ2frczUgiFRUT1P9rjNH4kUPcAmOh/flGzrTbnaBtoma1h/LLToHQinImglOiSLBG8Txjel6oTHbF8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735043016; c=relaxed/simple; bh=JbaNfMyXyBn6AMtoHSyFOzB7LdGSSj1q93ufDGvXtFg=; h=CC:Subject:To:References:From:Message-ID:Date:MIME-Version: In-Reply-To:Content-Type; b=fZcN3J3g1tLdbAgzslsnqaKvZdm/aOfPePbC1LKHyl4Tj6hmf3PjI7Pta6Y26MGM7f/iArKxUBvYf4U0SKj9mc/p3sG4HM3VnCtvg/fEfDieGNU3aNUR3oOevRLRJIFIeAw6DSffDFzRSNvl4bzYYqS2YrXQ3Z/MsXZpo1Ig0o8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com; spf=pass smtp.mailfrom=huawei.com; arc=none smtp.client-ip=45.249.212.32 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=huawei.com Received: from mail.maildlp.com (unknown [172.19.162.112]) by szxga06-in.huawei.com (SkyGuard) with ESMTP id 4YHYw121gjz20mf9; Tue, 24 Dec 2024 20:23:45 +0800 (CST) Received: from kwepemd200014.china.huawei.com (unknown [7.221.188.8]) by mail.maildlp.com (Postfix) with ESMTPS id 964531400F4; Tue, 24 Dec 2024 20:23:25 +0800 (CST) Received: from [10.67.121.177] (10.67.121.177) by kwepemd200014.china.huawei.com (7.221.188.8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1258.34; Tue, 24 Dec 2024 20:23:24 +0800 CC: , , , , , , , , , , , , , , , , , , , , , , , Subject: Re: [PATCH v10 2/4] arch_topology: Support SMT control for OF based system To: Jonathan Cameron References: <20241220075313.51502-1-yangyicong@huawei.com> <20241220075313.51502-3-yangyicong@huawei.com> <20241223163314.00006fed@huawei.com> From: Yicong Yang Message-ID: <935a29ae-7bf4-fcaa-061c-8ff77e53385d@huawei.com> Date: Tue, 24 Dec 2024 20:23:23 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.5.1 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 In-Reply-To: <20241223163314.00006fed@huawei.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To kwepemd200014.china.huawei.com (7.221.188.8) On 2024/12/24 0:33, Jonathan Cameron wrote: > On Fri, 20 Dec 2024 15:53:11 +0800 > Yicong Yang wrote: > >> From: Yicong Yang >> >> On building the topology from the devicetree, we've already >> gotten the SMT thread number of each core. Update the largest >> SMT thread number and enable the SMT control by the end of >> topology parsing. >> >> The core's SMT control provides two interface to the users [1]: >> 1) enable/disable SMT by writing on/off >> 2) enable/disable SMT by writing thread number 1/max_thread_number >> >> If a system have more than one SMT thread number the 2) may >> not handle it well, since there're multiple thread numbers in the >> system and 2) only accept 1/max_thread_number. So issue a warning >> to notify the users if such system detected. >> >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-devices-system-cpu#n542 >> Signed-off-by: Yicong Yang > Hi Yicong, > > Apologies that I'm late to the game on this one. > > A few comments inline. Only important one is whether to bail out early > on error from parse_cluster() > thanks for the comments. > Thanks, > > Jonathan > > >> --- >> drivers/base/arch_topology.c | 26 ++++++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c >> index 3ebe77566788..9e81060144c7 100644 >> --- a/drivers/base/arch_topology.c >> +++ b/drivers/base/arch_topology.c >> @@ -11,6 +11,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -506,6 +507,10 @@ core_initcall(free_raw_capacity); >> #endif >> >> #if defined(CONFIG_ARM64) || defined(CONFIG_RISCV) >> + >> +/* Maximum SMT thread number detected used to enable the SMT control */ >> +static unsigned int max_smt_thread_num; >> + >> /* >> * This function returns the logic cpu number of the node. >> * There are basically three kinds of return values: >> @@ -565,6 +570,17 @@ static int __init parse_core(struct device_node *core, int package_id, >> i++; >> } while (1); >> >> + /* >> + * If max_smt_thread_num has been initialized and doesn't match >> + * the thread number of this entry, then the system has >> + * heterogeneous SMT topology. >> + */ >> + if (max_smt_thread_num && max_smt_thread_num != i) >> + pr_warn_once("Heterogeneous SMT topology is partly supported by SMT control\n"); >> + >> + if (max_smt_thread_num < i) >> + max_smt_thread_num = i; > > Maybe more self documenting if you use min()? I'm not sure... > max_smt_thread_num = min(max_smt_thread_num, i); > sure, will use max_t here (since we'd like to know the maximum thread number). > >> + >> cpu = get_cpu_for_node(core); >> if (cpu >= 0) { >> if (!leaf) { >> @@ -677,6 +693,16 @@ static int __init parse_socket(struct device_node *socket) >> if (!has_socket) >> ret = parse_cluster(socket, 0, -1, 0); > > Is it appropriate to check ret before setting num threads? > if (!has_socket) { > ret = parse_cluster(socket, 0, -1, 0); > if (ret) > return ret; > } > ... > > return 0; > thanks for catching this. since we always need to notify the framework of the SMT thread number if HOTPLUG_SMT selected, we should make it to 1 if topology parsing failed. Since on failure the topology information will be reset, a thread_number of 1 will be handled as SMT not supported. >> >> + /* >> + * Notify the CPU framework of the SMT support. Initialize the >> + * max_smt_thread_num to 1 if no SMT support detected. A thread >> + * number of 1 can be handled by the framework so we don't need >> + * to check max_smt_thread_num to see we support SMT or not. >> + */ >> + if (!max_smt_thread_num) >> + max_smt_thread_num = 1; >> + >> + cpu_smt_set_num_threads(max_smt_thread_num, max_smt_thread_num); > > Trivial but I'd put a blank line here. > ok. Thanks.