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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 45E72C04FDF for ; Fri, 11 Aug 2023 01:15:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 87A096B0071; Thu, 10 Aug 2023 21:15:09 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 803236B0072; Thu, 10 Aug 2023 21:15:09 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 67D8B6B0074; Thu, 10 Aug 2023 21:15:09 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 54E536B0071 for ; Thu, 10 Aug 2023 21:15:09 -0400 (EDT) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 2F9431CA0A7 for ; Fri, 11 Aug 2023 01:15:09 +0000 (UTC) X-FDA: 81110055138.25.7070AC2 Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.24]) by imf04.hostedemail.com (Postfix) with ESMTP id 211FB40015 for ; Fri, 11 Aug 2023 01:15:05 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=nLezYqTs; spf=pass (imf04.hostedemail.com: domain of ying.huang@intel.com designates 134.134.136.24 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1691716506; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=kQ01JgvBOLll3xxocOQ6eyuQdPQdUVj8qOpuQZBL2Og=; b=Bk/ksmIPMjomFbL2fO9HHT1f3tyD3yCDbFMZkCpcAEBKTVJr7EfRhb18zPDnqqSmI50Djx wMQHEbzsKNhtujn2sQfA9i0ngbLcTPNuU3h3KKXXt1/d7eAfdnBrfsNtNVMFDH1GEd6aHt dJnTVOlzX/gR4irEoiseC3rptAZQwck= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1691716507; a=rsa-sha256; cv=none; b=wC3zUA0l6AyTuLnH7iFeV4SvO4kbyXhkekv3bCncs/Pu5+CJmR1TdnusRU2vI3T9rarcfn ++BEpjoOYMvXVlqa0yLUT4K30tEwmkZE/1jL4yl2Pe/NiQ29Lgu2O2BjDcP1a9kWfY0KFd sHZsoGpC//7IvO/ninPUXilEdIQrAGM= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=nLezYqTs; spf=pass (imf04.hostedemail.com: domain of ying.huang@intel.com designates 134.134.136.24 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1691716506; x=1723252506; h=from:to:cc:subject:references:date:in-reply-to: message-id:mime-version; bh=GSoNnc9ZROtEeZh8ssPdb9O3EsO5sMHiL/NsY3M9aQE=; b=nLezYqTsFaL5YTBkP9p5e5+a9X+HF6atdKwucKJd8sVY0I24sJe6xRIL ZXDTYMByoZw7bEdDJFzDtKlTwe6sjO6yRWhlBMGXJHLP6Qz3ppBXlQgbS KLd0M6bRBFgE1VW6Mm/EWWusK2RKT3PHnZ9sk3R3KvVsE/LxEW8s7uhvR 15iJsPqLMe2MieZ2aHhoTQHisZQd3ZdTe4eqCMU+4/0bmjVAGAWE2i1un MtdmgNhlSUu1kSiB5cFEC8xKfkikeMSvm3KWAQTIYV7Rk/+Y8mv3/5T8u HjTCiRR3MCYbeltjHFgtpiPNAz51n3Vi89uhcCpE9ZVNKkbNSH9ax0VLN g==; X-IronPort-AV: E=McAfee;i="6600,9927,10798"; a="374332141" X-IronPort-AV: E=Sophos;i="6.01,163,1684825200"; d="scan'208";a="374332141" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Aug 2023 18:15:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10798"; a="979042916" X-IronPort-AV: E=Sophos;i="6.01,163,1684825200"; d="scan'208";a="979042916" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by fmsmga006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Aug 2023 18:15:00 -0700 From: "Huang, Ying" To: Jonathan Cameron Cc: Andrew Morton , , , , , , "Aneesh Kumar K . V" , Wei Xu , Alistair Popple , Dan Williams , Dave Hansen , Davidlohr Bueso , "Johannes Weiner" , Michal Hocko , Yang Shi , Rafael J Wysocki Subject: Re: [PATCH RESEND 2/4] acpi, hmat: refactor hmat_register_target_initiators() References: <20230721012932.190742-1-ying.huang@intel.com> <20230721012932.190742-3-ying.huang@intel.com> <20230807175546.00001566@Huawei.com> Date: Fri, 11 Aug 2023 09:13:23 +0800 In-Reply-To: <20230807175546.00001566@Huawei.com> (Jonathan Cameron's message of "Mon, 7 Aug 2023 17:55:46 +0100") Message-ID: <87v8dmpej0.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii X-Stat-Signature: o79ks5oe18ofdqix4bi3db3zox8ppj1j X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 211FB40015 X-Rspam-User: X-HE-Tag: 1691716505-383067 X-HE-Meta: U2FsdGVkX1/8DLKFaBmeKn6K3iBrXFikVCp1W2bs2piyX4uAmurotRy1k+NTnGgb4HzLmy8eL/46OQD92X4oxYCNozB4Tx9RbsUWIzA/j+LBDtcv/0/8N35xcSY7G7mtE0y2IhFBQx4O+5Gl8dKy6vfHHrzjdtVednhDRv0iprD3O9qNBhPP3zf9MQjSUfxvdgSnRsVfKVsVpfF4WBYoNry8hPKiriaJZ7Dl2sgJO72zFfzL5IIaFaT++eaB9Nfx3s27zxky81gw6XQATGUbflDL7ifMzPq9ufUrNSyjXt82sY0XIDRGoWAGGcJU71kGOMAL3x6c5UZ5dt95miXvidLqOQ5tHxws8nwm6FJV5x7NYka2AnUQQzACXspyy8t3k4Uw/hyDcbOUaDBX7fBgBbaSAtaJ4MmkaFvc/neVQBgyjFZamWhSqdJEhqNt10TUI4fneUbLSyJjT8XGASAhjwFqmqcqIrfrYkpQutxaDXHwboW/ogzUu/oUj/D7CyCqclJz60SQztRuQ1ju3GMZrmzMK03AlhqurI2TtZLs87Em4X6rGBgCdoU2TY/0Pnt8tsrq8IUqKefdkc+v/fUE16mSj6+WQR5Z67RKHG4QuoZXvSMPPMDkeOPRCeZyKDDPnRZBHoRd4fOizrNABg5iIjAN37izzNa4tc6udvFtCPQ4pBYudK6LVGRxm1LSoYQ35PT5QB5b/UxaNjrrzsIIMhslvyPUOj7jQFb53B5KmLfDyTAim3XsrJGuW9VOfQZDxeJOwuLiVqrrrpaQauYS+Whx9NN+DHLlPxohOO14ijx0ONFncJWVfLgrn6m3IlfHPVUfT/e14uIXvpfhy73ZZt3T0vbSqZoET3udc1MB2cJ6j64yGDOEbarFhPtCqSM5mXzhZRmxR+77uNmDhkRv8iKxa7hnTmPTjY5YDtvy3DrLLwy7/a5XFxkAVcLXuj+ZJ/fO0GV01FuKHAa3Kp5 P2Dng0Ru fARcgTyo3oY8+uJU0yX/ba6umAIeSDubZ9HRdPNHvQvktVdj6Uo7raomRO2AVY1yxnVnkIOfk3HM3g2EW80/6xhWHKILwigAbu0Ef85UegTD1tdf8POuJgUAmr4XGEy6a7ZZn/ETbI2WZSAMh+rUsd9sct4vitdZzCbsh/L+QoRiRR2vJflhl3XEphWFjMqfk4tCL4ZuvnIxpL+y1etb8+YoLEa9d3kMOgvZr7tHG6ax2s2qCHH3Tpb2RNInGh4UygAyUWFyJkCGklH9/OckVUQnPEDSXwruI3n+3L0G9/tkVwTSNQzcQ6+jy3J1BgroOkhFTBSeSAvvkjsTxGVAe6gjme8m1G5Y4NCh5FwU+ca7PhcHIJf66uLDm2xzD1BrbZ+eeTvQBDCV4MwLVgwKdq8GkI1RacoW3eXyJHBIEFoIMWNow3HKwZEx5VMT7zcuL15Kn8VrbVxd0Upd31vIyRMUuzjn91fHrFrM3wGHaoWJ2lSrxNJwWLHzFvjhBaQS+QLj9XvT16AtId7U= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Hi, Jonathan, Thanks for review! Jonathan Cameron writes: > On Fri, 21 Jul 2023 09:29:30 +0800 > Huang Ying wrote: > >> Previously, in hmat_register_target_initiators(), the performance >> attributes are calculated and the corresponding sysfs links and files >> are created too. Which is called during memory onlining. >> >> But now, to calculate the abstract distance of a memory target before >> memory onlining, we need to calculate the performance attributes for >> a memory target without creating sysfs links and files. >> >> To do that, hmat_register_target_initiators() is refactored to make it >> possible to calculate performance attributes separately. >> >> Signed-off-by: "Huang, Ying" >> Cc: Aneesh Kumar K.V >> Cc: Wei Xu >> Cc: Alistair Popple >> Cc: Dan Williams >> Cc: Dave Hansen >> Cc: Davidlohr Bueso >> Cc: Johannes Weiner >> Cc: Jonathan Cameron >> Cc: Michal Hocko >> Cc: Yang Shi >> Cc: Rafael J Wysocki > > Unfortunately I don't think I still have the tables I used to test the > generic initiator and won't get time to generate them all again in > next few weeks. So just a superficial review for now. > I 'think' the cleanup looks good but the original code was rather fiddly > so I'm not 100% sure nothing is missed. > > One comment inline on the fact the list is now sorted twice. > > >> --- >> drivers/acpi/numa/hmat.c | 81 +++++++++++++++------------------------- >> 1 file changed, 30 insertions(+), 51 deletions(-) >> >> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c >> index bba268ecd802..2dee0098f1a9 100644 >> --- a/drivers/acpi/numa/hmat.c >> +++ b/drivers/acpi/numa/hmat.c >> @@ -582,28 +582,25 @@ static int initiators_to_nodemask(unsigned long *p_nodes) >> return 0; >> } >> >> -static void hmat_register_target_initiators(struct memory_target *target) >> +static void hmat_update_target_attrs(struct memory_target *target, >> + unsigned long *p_nodes, int access) >> { >> - static DECLARE_BITMAP(p_nodes, MAX_NUMNODES); >> struct memory_initiator *initiator; >> - unsigned int mem_nid, cpu_nid; >> + unsigned int cpu_nid; >> struct memory_locality *loc = NULL; >> u32 best = 0; >> - bool access0done = false; >> int i; >> >> - mem_nid = pxm_to_node(target->memory_pxm); >> + bitmap_zero(p_nodes, MAX_NUMNODES); >> /* >> - * If the Address Range Structure provides a local processor pxm, link >> + * If the Address Range Structure provides a local processor pxm, set >> * only that one. Otherwise, find the best performance attributes and >> - * register all initiators that match. >> + * collect all initiators that match. >> */ >> if (target->processor_pxm != PXM_INVAL) { >> cpu_nid = pxm_to_node(target->processor_pxm); >> - register_memory_node_under_compute_node(mem_nid, cpu_nid, 0); >> - access0done = true; >> - if (node_state(cpu_nid, N_CPU)) { >> - register_memory_node_under_compute_node(mem_nid, cpu_nid, 1); >> + if (access == 0 || node_state(cpu_nid, N_CPU)) { >> + set_bit(target->processor_pxm, p_nodes); >> return; >> } >> } >> @@ -617,47 +614,10 @@ static void hmat_register_target_initiators(struct memory_target *target) >> * We'll also use the sorting to prime the candidate nodes with known >> * initiators. >> */ >> - bitmap_zero(p_nodes, MAX_NUMNODES); >> list_sort(NULL, &initiators, initiator_cmp); >> if (initiators_to_nodemask(p_nodes) < 0) >> return; > > One result of this refactor is that a few things run twice, that previously only ran once > like this list_sort() > Not necessarily a problem though as probably fairly cheap. Yes. The original code sorts once for each target. But it appears that it's unnecessary too. We can sort the initiators list when adding new item to it in alloc_memory_initiator(). If necessary, I can add an additional patch to do that. But as you said, it may be unnecessary because the sort should be fairly cheap. -- Best Regards, Huang, Ying >> >> - if (!access0done) { >> - for (i = WRITE_LATENCY; i <= READ_BANDWIDTH; i++) { >> - loc = localities_types[i]; >> - if (!loc) >> - continue; >> - >> - best = 0; >> - list_for_each_entry(initiator, &initiators, node) { >> - u32 value; >> - >> - if (!test_bit(initiator->processor_pxm, p_nodes)) >> - continue; >> - >> - value = hmat_initiator_perf(target, initiator, >> - loc->hmat_loc); >> - if (hmat_update_best(loc->hmat_loc->data_type, value, &best)) >> - bitmap_clear(p_nodes, 0, initiator->processor_pxm); >> - if (value != best) >> - clear_bit(initiator->processor_pxm, p_nodes); >> - } >> - if (best) >> - hmat_update_target_access(target, loc->hmat_loc->data_type, >> - best, 0); >> - } >> - >> - for_each_set_bit(i, p_nodes, MAX_NUMNODES) { >> - cpu_nid = pxm_to_node(i); >> - register_memory_node_under_compute_node(mem_nid, cpu_nid, 0); >> - } >> - } >> - >> - /* Access 1 ignores Generic Initiators */ >> - bitmap_zero(p_nodes, MAX_NUMNODES); >> - if (initiators_to_nodemask(p_nodes) < 0) >> - return; >> - >> for (i = WRITE_LATENCY; i <= READ_BANDWIDTH; i++) { >> loc = localities_types[i]; >> if (!loc) >> @@ -667,7 +627,7 @@ static void hmat_register_target_initiators(struct memory_target *target) >> list_for_each_entry(initiator, &initiators, node) { >> u32 value; >> >> - if (!initiator->has_cpu) { >> + if (access == 1 && !initiator->has_cpu) { >> clear_bit(initiator->processor_pxm, p_nodes); >> continue; >> } >> @@ -681,14 +641,33 @@ static void hmat_register_target_initiators(struct memory_target *target) >> clear_bit(initiator->processor_pxm, p_nodes); >> } >> if (best) >> - hmat_update_target_access(target, loc->hmat_loc->data_type, best, 1); >> + hmat_update_target_access(target, loc->hmat_loc->data_type, best, access); >> } >> +} >> + >> +static void __hmat_register_target_initiators(struct memory_target *target, >> + unsigned long *p_nodes, >> + int access) >> +{ >> + unsigned int mem_nid, cpu_nid; >> + int i; >> + >> + mem_nid = pxm_to_node(target->memory_pxm); >> + hmat_update_target_attrs(target, p_nodes, access); >> for_each_set_bit(i, p_nodes, MAX_NUMNODES) { >> cpu_nid = pxm_to_node(i); >> - register_memory_node_under_compute_node(mem_nid, cpu_nid, 1); >> + register_memory_node_under_compute_node(mem_nid, cpu_nid, access); >> } >> } >> >> +static void hmat_register_target_initiators(struct memory_target *target) >> +{ >> + static DECLARE_BITMAP(p_nodes, MAX_NUMNODES); >> + >> + __hmat_register_target_initiators(target, p_nodes, 0); >> + __hmat_register_target_initiators(target, p_nodes, 1); >> +} >> + >> static void hmat_register_target_cache(struct memory_target *target) >> { >> unsigned mem_nid = pxm_to_node(target->memory_pxm);