[{"id":1787392,"web_url":"http://patchwork.ozlabs.org/comment/1787392/","msgid":"<871sm3l1c1.fsf@concordia.ellerman.id.au>","date":"2017-10-16T12:33:02","subject":"Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for\n\toperations","submitter":{"id":46580,"url":"http://patchwork.ozlabs.org/api/people/46580/","name":"Michael Ellerman","email":"mpe@ellerman.id.au"},"content":"Michael Bringmann writes:\n\n> powerpc/nodes: On systems like PowerPC which allow 'hot-add' of CPU\n\nThis is a powerpc-only patch, so saying \"systems like PowerPC\" is\nconfusing. What you should be saying is \"On pseries systems\".\n\n> or memory resources, it may occur that the new resources are to be\n> inserted into nodes that were not used for these resources at bootup.\n> In the kernel, any node that is used must be defined and initialized\n> at boot.\n>\n> This patch extracts the value of the lowest domain level (number of\n> allocable resources) from the \"rtas\" device tree property\n> \"ibm,current-associativity-domains\" or the device tree property\n\nWhat is current associativity domains? I've not heard of it, where is it\ndocumented, and what does it mean.\n\nWhy would use the \"current\" set vs the \"max\"? I thought the whole point\nwas to discover the maximum possible set of nodes that could be\nhotplugged.\n\n> \"ibm,max-associativity-domains\" to use as the maximum number of nodes\n> to setup as possibly available in the system. This new setting will\n> override the instruction,\n>\n> nodes_and(node_possible_map, node_possible_map, node_online_map);\n>\n> presently seen in the function arch/powerpc/mm/numa.c:initmem_init().\n>\n> If the property is not present at boot, no operation will be performed\n> to define or enable additional nodes.\n>\n> Signed-off-by: Michael Bringmann \n> ---\n> arch/powerpc/mm/numa.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 47 insertions(+)\n>\n> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c\n> index ec098b3..b385cd0 100644\n> --- a/arch/powerpc/mm/numa.c\n> +++ b/arch/powerpc/mm/numa.c\n> @@ -892,6 +892,51 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)\n> \tNODE_DATA(nid)->node_spanned_pages = spanned_pages;\n> }\n> \n> +static void __init node_associativity_setup(void)\n\nThis should really be called \"find_possible_nodes()\" or something more\ndescriptive.\n\n> +{\n> +\tstruct device_node *rtas;\n> +\n> +\trtas = of_find_node_by_path(\"/rtas\");\n> +\tif (rtas) {\n\nIf you just short-circuit that return the whole function body can be\ndeintented, making it significantly more readable.\n\nie:\n+\trtas = of_find_node_by_path(\"/rtas\");\n+\tif (!rtas)\n+\t\treturn;\n\n> +\t\tconst __be32 *prop;\n> +\t\tu32 len, entries, numnodes, i;\n> +\n> +\t\tprop = of_get_property(rtas,\n> +\t\t\t\t\t\"ibm,current-associativity-domains\", &len);\n\nPlease don't use of_get_property() in new code, we have much better\naccessors these days, which do better error checking and handle the\nendian conversions for you.\n\nIn this case you'd use eg:\n\n\tu32 entries;\n\trc = of_property_read_u32(rtas, \"ibm,current-associativity-domains\", &entries);\n\n> +\t\tif (!prop || len < sizeof(unsigned int)) {\n> +\t\t\tprop = of_get_property(rtas,\n> +\t\t\t\t\t\"ibm,max-associativity-domains\", &len);\n> +\t\t\tgoto endit;\n> +\t\t}\n> +\n> +\t\tentries = of_read_number(prop++, 1);\n> +\n> +\t\tif (len < (entries * sizeof(unsigned int)))\n> +\t\t\tgoto endit;\n> +\n> +\t\tif ((0 <= min_common_depth) && (min_common_depth <= (entries-1)))\n> +\t\t\tentries = min_common_depth;\n> +\t\telse\n> +\t\t\tentries -= 1;\n\t\t\t^\n You can't just guess that will be the right entry.\n\nIf min_common_depth is < 0 the function should have just returned\nimmediately at the top.\n\nIf min_common_depth is outside the range of the property that's a buggy\ndevice tree, you should print a warning and return.\n\n> +\t\tnumnodes = of_read_number(&prop[entries], 1);\n\n\tu32 num_nodes;\n\trc = of_property_read_u32_index(rtas, \"ibm,current-associativity-domains\", min_common_depth, &num_nodes);\n> +\n> +\t\tprintk(KERN_INFO \"numa: Nodes = %d (mcd = %d)\\n\", numnodes,\n> +\t\t\tmin_common_depth);\n> +\n> +\t\tfor (i = 0; i < numnodes; i++) {\n> +\t\t\tif (!node_possible(i)) {\n> +\t\t\t\tsetup_node_data(i, 0, 0);\n\nDo we actually need to setup the NODE_DATA() yet? Doing it now ensures\nit will not be allocated node local, which sucks.\n\n> +\t\t\t\tnode_set(i, node_possible_map);\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +endit:\n\n\"out\" would be the normal name.\n\n> +\tif (rtas)\n> +\t\tof_node_put(rtas);\n> +}\n> +\n> void __init initmem_init(void)\n> {\n> \tint nid, cpu;\n> @@ -911,6 +956,8 @@ void __init initmem_init(void)\n> \t */\n\nYou need to update the comment above here which is contradicted by the\nnew function you're adding.\n\n> \tnodes_and(node_possible_map, node_possible_map, node_online_map);\n> \n> +\tnode_associativity_setup();\n> +\n> \tfor_each_online_node(nid) {\n> \t\tunsigned long start_pfn, end_pfn;\n> \n\ncheers","headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yFyS13Pkvz9t1t\n\tfor ;\n\tMon, 16 Oct 2017 23:34:09 +1100 (AEDT)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3yFyS12T12zDrSm\n\tfor ;\n\tMon, 16 Oct 2017 23:34:09 +1100 (AEDT)","from ozlabs.org (ozlabs.org [IPv6:2401:3900:2:1::2])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3yFyQl1zD7zDrG1\n\tfor ;\n\tMon, 16 Oct 2017 23:33:03 +1100 (AEDT)","from authenticated.ozlabs.org (localhost [127.0.0.1])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPSA id 3yFyQk6nqgz9t1t;\n\tMon, 16 Oct 2017 23:33:02 +1100 (AEDT)"],"From":"Michael Ellerman ","To":"Michael Bringmann , linuxppc-dev@lists.ozlabs.org,\n\tlinux-kernel@vger.kernel.org","Subject":"Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for\n\toperations","In-Reply-To":"","References":"","Date":"Mon, 16 Oct 2017 23:33:02 +1100","Message-ID":"<871sm3l1c1.fsf@concordia.ellerman.id.au>","MIME-Version":"1.0","Content-Type":"text/plain","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"John Allen ,\n\tMichael Bringmann ,\n\tNathan Fontenot ","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"}},{"id":1788611,"web_url":"http://patchwork.ozlabs.org/comment/1788611/","msgid":"","date":"2017-10-17T16:14:47","subject":"Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for\n\toperations","submitter":{"id":65104,"url":"http://patchwork.ozlabs.org/api/people/65104/","name":"Michael Bringmann","email":"mwb@linux.vnet.ibm.com"},"content":"See below.\n\nOn 10/16/2017 07:33 AM, Michael Ellerman wrote:\n> Michael Bringmann writes:\n> \n>> powerpc/nodes: On systems like PowerPC which allow 'hot-add' of CPU\n> \n> This is a powerpc-only patch, so saying \"systems like PowerPC\" is\n> confusing. What you should be saying is \"On pseries systems\".\n> \n>> or memory resources, it may occur that the new resources are to be\n>> inserted into nodes that were not used for these resources at bootup.\n>> In the kernel, any node that is used must be defined and initialized\n>> at boot.\n>>\n>> This patch extracts the value of the lowest domain level (number of\n>> allocable resources) from the \"rtas\" device tree property\n>> \"ibm,current-associativity-domains\" or the device tree property\n> \n> What is current associativity domains? I've not heard of it, where is it\n> documented, and what does it mean.\n> \n> Why would use the \"current\" set vs the \"max\"? I thought the whole point\n> was to discover the maximum possible set of nodes that could be\n> hotplugged.\n> \n>> \"ibm,max-associativity-domains\" to use as the maximum number of nodes\n>> to setup as possibly available in the system. This new setting will\n>> override the instruction,\n>>\n>> nodes_and(node_possible_map, node_possible_map, node_online_map);\n>>\n>> presently seen in the function arch/powerpc/mm/numa.c:initmem_init().\n>>\n>> If the property is not present at boot, no operation will be performed\n>> to define or enable additional nodes.\n>>\n>> Signed-off-by: Michael Bringmann \n>> ---\n>> arch/powerpc/mm/numa.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n>> 1 file changed, 47 insertions(+)\n>>\n>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c\n>> index ec098b3..b385cd0 100644\n>> --- a/arch/powerpc/mm/numa.c\n>> +++ b/arch/powerpc/mm/numa.c\n>> @@ -892,6 +892,51 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)\n>> \tNODE_DATA(nid)->node_spanned_pages = spanned_pages;\n>> }\n>> \n>> +static void __init node_associativity_setup(void)\n> \n> This should really be called \"find_possible_nodes()\" or something more\n> descriptive.\n\nOkay.\n> \n>> +{\n>> +\tstruct device_node *rtas;\n>> +\n>> +\trtas = of_find_node_by_path(\"/rtas\");\n>> +\tif (rtas) {\n> \n> If you just short-circuit that return the whole function body can be\n> deintented, making it significantly more readable.\n> \n> ie:\n> +\trtas = of_find_node_by_path(\"/rtas\");\n> +\tif (!rtas)\n> +\t\treturn;\n\nOkay.\n\n> \n>> +\t\tconst __be32 *prop;\n>> +\t\tu32 len, entries, numnodes, i;\n>> +\n>> +\t\tprop = of_get_property(rtas,\n>> +\t\t\t\t\t\"ibm,current-associativity-domains\", &len);\n> \n> Please don't use of_get_property() in new code, we have much better\n> accessors these days, which do better error checking and handle the\n> endian conversions for you.\n> \n> In this case you'd use eg:\n> \n> \tu32 entries;\n> \trc = of_property_read_u32(rtas, \"ibm,current-associativity-domains\", &entries);\n\nThe property 'ibm,current-associativity-domains' has the same format as the property\n'ibm,max-associativity-domains' i.e. it is an integer array. The accessor of_property_read_32,\nhowever, expects it to be an integer singleton value. Instead, it needs:\n\n> \n>> +\t\tif (!prop || len < sizeof(unsigned int)) {\n>> +\t\t\tprop = of_get_property(rtas,\n>> +\t\t\t\t\t\"ibm,max-associativity-domains\", &len);\n if (!prop || len < sizeof(unsigned int))\n>> +\t\t\t\tgoto endit;\n>> +\t\t}\n>> +\n>> +\t\tentries = of_read_number(prop++, 1);\n>> +\n>> +\t\tif (len < (entries * sizeof(unsigned int)))\n>> +\t\t\tgoto endit;\n>> +\n>> +\t\tif ((0 <= min_common_depth) && (min_common_depth <= (entries-1)))\n>> +\t\t\tentries = min_common_depth;\n>> +\t\telse\n>> +\t\t\tentries -= 1;\n> \t\t\t^\n> You can't just guess that will be the right entry.\n> \n> If min_common_depth is < 0 the function should have just returned\n> immediately at the top.\n\nOkay.\n\n> \n> If min_common_depth is outside the range of the property that's a buggy\n> device tree, you should print a warning and return.\n> \n>> +\t\tnumnodes = of_read_number(&prop[entries], 1);\n> \n> \tu32 num_nodes;\n> \trc = of_property_read_u32_index(rtas, \"ibm,current-associativity-domains\", min_common_depth, &num_nodes);\n>> +\n>> +\t\tprintk(KERN_INFO \"numa: Nodes = %d (mcd = %d)\\n\", numnodes,\n>> +\t\t\tmin_common_depth);\n>> +\n>> +\t\tfor (i = 0; i < numnodes; i++) {\n>> +\t\t\tif (!node_possible(i)) {\n>> +\t\t\t\tsetup_node_data(i, 0, 0);\n> \n> Do we actually need to setup the NODE_DATA() yet? Doing it now ensures\n> it will not be allocated node local, which sucks.\n\nOkay.\n\n> \n>> +\t\t\t\tnode_set(i, node_possible_map);\n>> +\t\t\t}\n>> +\t\t}\n>> +\t}\n>> +\n>> +endit:\n> \n> \"out\" would be the normal name.\n\nOkay.\n\n> \n>> +\tif (rtas)\n>> +\t\tof_node_put(rtas);\n>> +}\n>> +\n>> void __init initmem_init(void)\n>> {\n>> \tint nid, cpu;\n>> @@ -911,6 +956,8 @@ void __init initmem_init(void)\n>> \t */\n> \n> You need to update the comment above here which is contradicted by the\n> new function you're adding.\n\nOkay.\n\n> \n>> \tnodes_and(node_possible_map, node_possible_map, node_online_map);\n>> \n>> +\tnode_associativity_setup();\n>> +\n>> \tfor_each_online_node(nid) {\n>> \t\tunsigned long start_pfn, end_pfn;\n>> \n> \n> cheers\n> \n>","headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yGgMf32Ptz9t3Z\n\tfor ;\n\tWed, 18 Oct 2017 03:17:50 +1100 (AEDT)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3yGgMf05nszDqF9\n\tfor ;\n\tWed, 18 Oct 2017 03:17:49 +1100 (AEDT)","from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com\n\t[148.163.158.5])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3yGgK241SSzDqF9\n\tfor ;\n\tWed, 18 Oct 2017 03:15:34 +1100 (AEDT)","from pps.filterd (m0098417.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv9HGFM4b146827\n\tfor ; Tue, 17 Oct 2017 12:15:30 -0400","from e35.co.us.ibm.com (e35.co.us.ibm.com [32.97.110.153])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2dnk97y0r9-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 17 Oct 2017 12:15:25 -0400","from localhost\n\tby e35.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tTue, 17 Oct 2017 10:14:57 -0600","from b03cxnp08025.gho.boulder.ibm.com (9.17.130.17)\n\tby e35.co.us.ibm.com (192.168.1.135) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tTue, 17 Oct 2017 10:14:54 -0600","from b03ledav004.gho.boulder.ibm.com\n\t(b03ledav004.gho.boulder.ibm.com [9.17.130.235])\n\tby b03cxnp08025.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v9HGErNM459030; Tue, 17 Oct 2017 09:14:53 -0700","from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id B07F478068;\n\tTue, 17 Oct 2017 10:14:53 -0600 (MDT)","from oc1554177480.ibm.com (unknown [9.53.92.241])\n\tby b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP id 4ABDB7805C;\n\tTue, 17 Oct 2017 10:14:53 -0600 (MDT)"],"Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=linux.vnet.ibm.com\n\t(client-ip=148.163.158.5; helo=mx0a-001b2d01.pphosted.com;\n\tenvelope-from=mwb@linux.vnet.ibm.com; receiver=)","Subject":"Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for\n\toperations","To":"Michael Ellerman , linuxppc-dev@lists.ozlabs.org,\n\tlinux-kernel@vger.kernel.org","References":"\n\t<871sm3l1c1.fsf@concordia.ellerman.id.au>","From":"Michael Bringmann ","Organization":"IBM Linux Technology Center","Date":"Tue, 17 Oct 2017 11:14:47 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.0","MIME-Version":"1.0","In-Reply-To":"<871sm3l1c1.fsf@concordia.ellerman.id.au>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","X-TM-AS-GCONF":"00","x-cbid":"17101716-0012-0000-0000-00001528B67A","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007906; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000237; SDB=6.00932487; UDB=6.00469588;\n\tIPR=6.00712778; \n\tBA=6.00005642; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017576;\n\tXFM=3.00000015; UTC=2017-10-17 16:14:56","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17101716-0013-0000-0000-00004FE958E7","Message-Id":"","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-10-17_11:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1710170229","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"John Allen ,\n\tMichael Bringmann from Kernel Team ,\n\tNathan Fontenot ","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"}},{"id":1788663,"web_url":"http://patchwork.ozlabs.org/comment/1788663/","msgid":"<94ac2dd9-625c-d453-adf5-32703efaebcc@linux.vnet.ibm.com>","date":"2017-10-17T17:02:54","subject":"Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for\n\toperations","submitter":{"id":17146,"url":"http://patchwork.ozlabs.org/api/people/17146/","name":"Nathan Fontenot","email":"nfont@linux.vnet.ibm.com"},"content":"On 10/17/2017 11:14 AM, Michael Bringmann wrote:\n> See below.\n> \n> On 10/16/2017 07:33 AM, Michael Ellerman wrote:\n>> Michael Bringmann writes:\n>>\n>>> powerpc/nodes: On systems like PowerPC which allow 'hot-add' of CPU\n>>\n>> This is a powerpc-only patch, so saying \"systems like PowerPC\" is\n>> confusing. What you should be saying is \"On pseries systems\".\n>>\n>>> or memory resources, it may occur that the new resources are to be\n>>> inserted into nodes that were not used for these resources at bootup.\n>>> In the kernel, any node that is used must be defined and initialized\n>>> at boot.\n>>>\n>>> This patch extracts the value of the lowest domain level (number of\n>>> allocable resources) from the \"rtas\" device tree property\n>>> \"ibm,current-associativity-domains\" or the device tree property\n>>\n>> What is current associativity domains? I've not heard of it, where is it\n>> documented, and what does it mean.\n>>\n>> Why would use the \"current\" set vs the \"max\"? I thought the whole point\n>> was to discover the maximum possible set of nodes that could be\n>> hotplugged.\n>>\n>>> \"ibm,max-associativity-domains\" to use as the maximum number of nodes\n>>> to setup as possibly available in the system. This new setting will\n>>> override the instruction,\n>>>\n>>> nodes_and(node_possible_map, node_possible_map, node_online_map);\n>>>\n>>> presently seen in the function arch/powerpc/mm/numa.c:initmem_init().\n>>>\n>>> If the property is not present at boot, no operation will be performed\n>>> to define or enable additional nodes.\n>>>\n>>> Signed-off-by: Michael Bringmann \n>>> ---\n>>> arch/powerpc/mm/numa.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n>>> 1 file changed, 47 insertions(+)\n>>>\n>>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c\n>>> index ec098b3..b385cd0 100644\n>>> --- a/arch/powerpc/mm/numa.c\n>>> +++ b/arch/powerpc/mm/numa.c\n>>> @@ -892,6 +892,51 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)\n>>> \tNODE_DATA(nid)->node_spanned_pages = spanned_pages;\n>>> }\n>>> \n>>> +static void __init node_associativity_setup(void)\n>>\n>> This should really be called \"find_possible_nodes()\" or something more\n>> descriptive.\n> \n> Okay.\n>>\n>>> +{\n>>> +\tstruct device_node *rtas;\n>>> +\n>>> +\trtas = of_find_node_by_path(\"/rtas\");\n>>> +\tif (rtas) {\n>>\n>> If you just short-circuit that return the whole function body can be\n>> deintented, making it significantly more readable.\n>>\n>> ie:\n>> +\trtas = of_find_node_by_path(\"/rtas\");\n>> +\tif (!rtas)\n>> +\t\treturn;\n> \n> Okay.\n> \n>>\n>>> +\t\tconst __be32 *prop;\n>>> +\t\tu32 len, entries, numnodes, i;\n>>> +\n>>> +\t\tprop = of_get_property(rtas,\n>>> +\t\t\t\t\t\"ibm,current-associativity-domains\", &len);\n>>\n>> Please don't use of_get_property() in new code, we have much better\n>> accessors these days, which do better error checking and handle the\n>> endian conversions for you.\n>>\n>> In this case you'd use eg:\n>>\n>> \tu32 entries;\n>> \trc = of_property_read_u32(rtas, \"ibm,current-associativity-domains\", &entries);\n> \n> The property 'ibm,current-associativity-domains' has the same format as the property\n> 'ibm,max-associativity-domains' i.e. it is an integer array. The accessor of_property_read_32,\n> however, expects it to be an integer singleton value. Instead, it needs:\n\nI think for this case where the property is an array of values you could use\nof_property_count_elems_of_size() to get the number of elements in the array\nand then use of_property_read_u32_array() to read the array.\n\n-Nathan\n\n> \n>>\n>>> +\t\tif (!prop || len < sizeof(unsigned int)) {\n>>> +\t\t\tprop = of_get_property(rtas,\n>>> +\t\t\t\t\t\"ibm,max-associativity-domains\", &len);\n> if (!prop || len < sizeof(unsigned int))\n>>> +\t\t\t\tgoto endit;\n>>> +\t\t}\n>>> +\n>>> +\t\tentries = of_read_number(prop++, 1);\n>>> +\n>>> +\t\tif (len < (entries * sizeof(unsigned int)))\n>>> +\t\t\tgoto endit;\n>>> +\n>>> +\t\tif ((0 <= min_common_depth) && (min_common_depth <= (entries-1)))\n>>> +\t\t\tentries = min_common_depth;\n>>> +\t\telse\n>>> +\t\t\tentries -= 1;\n>> \t\t\t^\n>> You can't just guess that will be the right entry.\n>>\n>> If min_common_depth is < 0 the function should have just returned\n>> immediately at the top.\n> \n> Okay.\n> \n>>\n>> If min_common_depth is outside the range of the property that's a buggy\n>> device tree, you should print a warning and return.\n>>\n>>> +\t\tnumnodes = of_read_number(&prop[entries], 1);\n>>\n>> \tu32 num_nodes;\n>> \trc = of_property_read_u32_index(rtas, \"ibm,current-associativity-domains\", min_common_depth, &num_nodes);\n>>> +\n>>> +\t\tprintk(KERN_INFO \"numa: Nodes = %d (mcd = %d)\\n\", numnodes,\n>>> +\t\t\tmin_common_depth);\n>>> +\n>>> +\t\tfor (i = 0; i < numnodes; i++) {\n>>> +\t\t\tif (!node_possible(i)) {\n>>> +\t\t\t\tsetup_node_data(i, 0, 0);\n>>\n>> Do we actually need to setup the NODE_DATA() yet? Doing it now ensures\n>> it will not be allocated node local, which sucks.\n> \n> Okay.\n> \n>>\n>>> +\t\t\t\tnode_set(i, node_possible_map);\n>>> +\t\t\t}\n>>> +\t\t}\n>>> +\t}\n>>> +\n>>> +endit:\n>>\n>> \"out\" would be the normal name.\n> \n> Okay.\n> \n>>\n>>> +\tif (rtas)\n>>> +\t\tof_node_put(rtas);\n>>> +}\n>>> +\n>>> void __init initmem_init(void)\n>>> {\n>>> \tint nid, cpu;\n>>> @@ -911,6 +956,8 @@ void __init initmem_init(void)\n>>> \t */\n>>\n>> You need to update the comment above here which is contradicted by the\n>> new function you're adding.\n> \n> Okay.\n> \n>>\n>>> \tnodes_and(node_possible_map, node_possible_map, node_online_map);\n>>> \n>>> +\tnode_associativity_setup();\n>>> +\n>>> \tfor_each_online_node(nid) {\n>>> \t\tunsigned long start_pfn, end_pfn;\n>>> \n>>\n>> cheers\n>>\n>>\n>","headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yGhRV4Wlrz9t38\n\tfor ;\n\tWed, 18 Oct 2017 04:06:14 +1100 (AEDT)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3yGhRS6mcMzDrJV\n\tfor ;\n\tWed, 18 Oct 2017 04:06:12 +1100 (AEDT)","from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com\n\t[148.163.158.5])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3yGhMp5pW4zDqJ7\n\tfor ;\n\tWed, 18 Oct 2017 04:03:01 +1100 (AEDT)","from pps.filterd (m0098417.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv9HGxKT3103872\n\tfor ; Tue, 17 Oct 2017 13:02:59 -0400","from e38.co.us.ibm.com (e38.co.us.ibm.com [32.97.110.159])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2dnk980jwf-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 17 Oct 2017 13:02:58 -0400","from localhost\n\tby e38.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ; \n\tTue, 17 Oct 2017 11:02:58 -0600","from b03cxnp08027.gho.boulder.ibm.com (9.17.130.19)\n\tby e38.co.us.ibm.com (192.168.1.138) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tTue, 17 Oct 2017 11:02:56 -0600","from b03ledav003.gho.boulder.ibm.com\n\t(b03ledav003.gho.boulder.ibm.com [9.17.130.234])\n\tby b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v9HH2tMq61997106; Tue, 17 Oct 2017 10:02:55 -0700","from b03ledav003.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 26A406A043;\n\tTue, 17 Oct 2017 11:02:56 -0600 (MDT)","from [9.41.92.186] (unknown [9.41.92.186])\n\tby b03ledav003.gho.boulder.ibm.com (Postfix) with ESMTP id AD7806A04A;\n\tTue, 17 Oct 2017 11:02:55 -0600 (MDT)"],"Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=linux.vnet.ibm.com\n\t(client-ip=148.163.158.5; helo=mx0a-001b2d01.pphosted.com;\n\tenvelope-from=nfont@linux.vnet.ibm.com; receiver=)","Subject":"Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for\n\toperations","To":"Michael Bringmann ,\n\tMichael Ellerman , linuxppc-dev@lists.ozlabs.org, \n\tlinux-kernel@vger.kernel.org","References":"\n\t<871sm3l1c1.fsf@concordia.ellerman.id.au>\n\t","From":"Nathan Fontenot ","Date":"Tue, 17 Oct 2017 12:02:54 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.4.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-TM-AS-GCONF":"00","x-cbid":"17101717-0028-0000-0000-00000883BFBB","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007907; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000237; SDB=6.00932503; UDB=6.00469597;\n\tIPR=6.00712793; \n\tBA=6.00005642; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017576;\n\tXFM=3.00000015; UTC=2017-10-17 17:02:57","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17101717-0029-0000-0000-000037F917CF","Message-Id":"<94ac2dd9-625c-d453-adf5-32703efaebcc@linux.vnet.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-10-17_11:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1710170240","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"John Allen ,\n\tMichael Bringmann from Kernel Team ","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"}},{"id":1788680,"web_url":"http://patchwork.ozlabs.org/comment/1788680/","msgid":"<095d9828-b9d9-8dc7-8ef4-5dd7bf34c31d@linux.vnet.ibm.com>","date":"2017-10-17T17:22:11","subject":"Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for\n\toperations","submitter":{"id":65104,"url":"http://patchwork.ozlabs.org/api/people/65104/","name":"Michael Bringmann","email":"mwb@linux.vnet.ibm.com"},"content":"On 10/17/2017 12:02 PM, Nathan Fontenot wrote:\n> On 10/17/2017 11:14 AM, Michael Bringmann wrote:\n>> See below.\n>>\n>> On 10/16/2017 07:33 AM, Michael Ellerman wrote:\n>>> Michael Bringmann writes:\n>>>\n>>>> powerpc/nodes: On systems like PowerPC which allow 'hot-add' of CPU\n>>>\n>>> This is a powerpc-only patch, so saying \"systems like PowerPC\" is\n>>> confusing. What you should be saying is \"On pseries systems\".\n>>>\n>>>> or memory resources, it may occur that the new resources are to be\n>>>> inserted into nodes that were not used for these resources at bootup.\n>>>> In the kernel, any node that is used must be defined and initialized\n>>>> at boot.\n>>>>\n>>>> This patch extracts the value of the lowest domain level (number of\n>>>> allocable resources) from the \"rtas\" device tree property\n>>>> \"ibm,current-associativity-domains\" or the device tree property\n>>>\n>>> What is current associativity domains? I've not heard of it, where is it\n>>> documented, and what does it mean.\n>>>\n>>> Why would use the \"current\" set vs the \"max\"? I thought the whole point\n>>> was to discover the maximum possible set of nodes that could be\n>>> hotplugged.\n>>>\n>>>> \"ibm,max-associativity-domains\" to use as the maximum number of nodes\n>>>> to setup as possibly available in the system. This new setting will\n>>>> override the instruction,\n>>>>\n>>>> nodes_and(node_possible_map, node_possible_map, node_online_map);\n>>>>\n>>>> presently seen in the function arch/powerpc/mm/numa.c:initmem_init().\n>>>>\n>>>> If the property is not present at boot, no operation will be performed\n>>>> to define or enable additional nodes.\n>>>>\n>>>> Signed-off-by: Michael Bringmann \n>>>> ---\n>>>> arch/powerpc/mm/numa.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n>>>> 1 file changed, 47 insertions(+)\n>>>>\n>>>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c\n>>>> index ec098b3..b385cd0 100644\n>>>> --- a/arch/powerpc/mm/numa.c\n>>>> +++ b/arch/powerpc/mm/numa.c\n>>>> @@ -892,6 +892,51 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)\n>>>> \tNODE_DATA(nid)->node_spanned_pages = spanned_pages;\n>>>> }\n>>>> \n>>>> +static void __init node_associativity_setup(void)\n>>>\n>>> This should really be called \"find_possible_nodes()\" or something more\n>>> descriptive.\n>>\n>> Okay.\n>>>\n>>>> +{\n>>>> +\tstruct device_node *rtas;\n>>>> +\n>>>> +\trtas = of_find_node_by_path(\"/rtas\");\n>>>> +\tif (rtas) {\n>>>\n>>> If you just short-circuit that return the whole function body can be\n>>> deintented, making it significantly more readable.\n>>>\n>>> ie:\n>>> +\trtas = of_find_node_by_path(\"/rtas\");\n>>> +\tif (!rtas)\n>>> +\t\treturn;\n>>\n>> Okay.\n>>\n>>>\n>>>> +\t\tconst __be32 *prop;\n>>>> +\t\tu32 len, entries, numnodes, i;\n>>>> +\n>>>> +\t\tprop = of_get_property(rtas,\n>>>> +\t\t\t\t\t\"ibm,current-associativity-domains\", &len);\n>>>\n>>> Please don't use of_get_property() in new code, we have much better\n>>> accessors these days, which do better error checking and handle the\n>>> endian conversions for you.\n>>>\n>>> In this case you'd use eg:\n>>>\n>>> \tu32 entries;\n>>> \trc = of_property_read_u32(rtas, \"ibm,current-associativity-domains\", &entries);\n>>\n>> The property 'ibm,current-associativity-domains' has the same format as the property\n>> 'ibm,max-associativity-domains' i.e. it is an integer array. The accessor of_property_read_32,\n>> however, expects it to be an integer singleton value. Instead, it needs:\n> \n> I think for this case where the property is an array of values you could use\n> of_property_count_elems_of_size() to get the number of elements in the array\n> and then use of_property_read_u32_array() to read the array.\n> \n> -Nathan\n\nWe only need one value from the array which is why I am using,\n\n>>>> +\t\tnumnodes = of_read_number(&prop[min_common_depth], 1);\n\nWith this implementation I do not need to allocate memory for\nan array, nor execute code to read all elements of the array.\n\nMichael\n\n> \n>>\n>>>\n>>>> +\t\tif (!prop || len < sizeof(unsigned int)) {\n>>>> +\t\t\tprop = of_get_property(rtas,\n>>>> +\t\t\t\t\t\"ibm,max-associativity-domains\", &len);\n>> if (!prop || len < sizeof(unsigned int))\n>>>> +\t\t\t\tgoto endit;\n>>>> +\t\t}\n>>>> +\n>>>> +\t\tentries = of_read_number(prop++, 1);\n>>>> +\n>>>> +\t\tif (len < (entries * sizeof(unsigned int)))\n>>>> +\t\t\tgoto endit;\n>>>> +\n>>>> +\t\tif ((0 <= min_common_depth) && (min_common_depth <= (entries-1)))\n>>>> +\t\t\tentries = min_common_depth;\n>>>> +\t\telse\n>>>> +\t\t\tentries -= 1;\n>>> \t\t\t^\n>>> You can't just guess that will be the right entry.\n>>>\n>>> If min_common_depth is < 0 the function should have just returned\n>>> immediately at the top.\n>>\n>> Okay.\n>>\n>>>\n>>> If min_common_depth is outside the range of the property that's a buggy\n>>> device tree, you should print a warning and return.\n>>>\n>>>> +\t\tnumnodes = of_read_number(&prop[entries], 1);\n>>>\n>>> \tu32 num_nodes;\n>>> \trc = of_property_read_u32_index(rtas, \"ibm,current-associativity-domains\", min_common_depth, &num_nodes);\n>>>> +\n>>>> +\t\tprintk(KERN_INFO \"numa: Nodes = %d (mcd = %d)\\n\", numnodes,\n>>>> +\t\t\tmin_common_depth);\n>>>> +\n>>>> +\t\tfor (i = 0; i < numnodes; i++) {\n>>>> +\t\t\tif (!node_possible(i)) {\n>>>> +\t\t\t\tsetup_node_data(i, 0, 0);\n>>>\n>>> Do we actually need to setup the NODE_DATA() yet? Doing it now ensures\n>>> it will not be allocated node local, which sucks.\n>>\n>> Okay.\n>>\n>>>\n>>>> +\t\t\t\tnode_set(i, node_possible_map);\n>>>> +\t\t\t}\n>>>> +\t\t}\n>>>> +\t}\n>>>> +\n>>>> +endit:\n>>>\n>>> \"out\" would be the normal name.\n>>\n>> Okay.\n>>\n>>>\n>>>> +\tif (rtas)\n>>>> +\t\tof_node_put(rtas);\n>>>> +}\n>>>> +\n>>>> void __init initmem_init(void)\n>>>> {\n>>>> \tint nid, cpu;\n>>>> @@ -911,6 +956,8 @@ void __init initmem_init(void)\n>>>> \t */\n>>>\n>>> You need to update the comment above here which is contradicted by the\n>>> new function you're adding.\n>>\n>> Okay.\n>>\n>>>\n>>>> \tnodes_and(node_possible_map, node_possible_map, node_online_map);\n>>>> \n>>>> +\tnode_associativity_setup();\n>>>> +\n>>>> \tfor_each_online_node(nid) {\n>>>> \t\tunsigned long start_pfn, end_pfn;\n>>>> \n>>>\n>>> cheers\n>>>\n>>>\n>>\n> \n>","headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yGhrR0m6rz9t2m\n\tfor ;\n\tWed, 18 Oct 2017 04:24:23 +1100 (AEDT)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3yGhrQ5hr1zDrG1\n\tfor ;\n\tWed, 18 Oct 2017 04:24:22 +1100 (AEDT)","from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com\n\t[148.163.158.5])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3yGhp76bRCzDqJ7\n\tfor ;\n\tWed, 18 Oct 2017 04:22:22 +1100 (AEDT)","from pps.filterd (m0098420.ppops.net [127.0.0.1])\n\tby mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv9HHKDei076256\n\tfor ; Tue, 17 Oct 2017 13:22:20 -0400","from e31.co.us.ibm.com (e31.co.us.ibm.com [32.97.110.149])\n\tby mx0b-001b2d01.pphosted.com with ESMTP id 2dnkyyevs4-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 17 Oct 2017 13:22:19 -0400","from localhost\n\tby e31.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tTue, 17 Oct 2017 11:22:19 -0600","from b03cxnp08026.gho.boulder.ibm.com (9.17.130.18)\n\tby e31.co.us.ibm.com (192.168.1.131) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tTue, 17 Oct 2017 11:22:17 -0600","from b03ledav004.gho.boulder.ibm.com\n\t(b03ledav004.gho.boulder.ibm.com [9.17.130.235])\n\tby b03cxnp08026.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v9HHMG4A64553062; Tue, 17 Oct 2017 10:22:16 -0700","from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id DF01478047;\n\tTue, 17 Oct 2017 11:22:16 -0600 (MDT)","from oc1554177480.ibm.com (unknown [9.53.92.241])\n\tby b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP id 7525878041;\n\tTue, 17 Oct 2017 11:22:16 -0600 (MDT)"],"Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=linux.vnet.ibm.com\n\t(client-ip=148.163.158.5; helo=mx0a-001b2d01.pphosted.com;\n\tenvelope-from=mwb@linux.vnet.ibm.com; receiver=)","Subject":"Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for\n\toperations","To":"Nathan Fontenot ,\n\tMichael Ellerman , linuxppc-dev@lists.ozlabs.org, \n\tlinux-kernel@vger.kernel.org","References":"\n\t<871sm3l1c1.fsf@concordia.ellerman.id.au>\n\t\n\t<94ac2dd9-625c-d453-adf5-32703efaebcc@linux.vnet.ibm.com>","From":"Michael Bringmann ","Organization":"IBM Linux Technology Center","Date":"Tue, 17 Oct 2017 12:22:11 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.0","MIME-Version":"1.0","In-Reply-To":"<94ac2dd9-625c-d453-adf5-32703efaebcc@linux.vnet.ibm.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","X-TM-AS-GCONF":"00","x-cbid":"17101717-8235-0000-0000-00000C6CCD55","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007907; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000237; SDB=6.00932510; UDB=6.00469601;\n\tIPR=6.00712800; \n\tBA=6.00005642; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017576;\n\tXFM=3.00000015; UTC=2017-10-17 17:22:18","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17101717-8236-0000-0000-00003E13D048","Message-Id":"<095d9828-b9d9-8dc7-8ef4-5dd7bf34c31d@linux.vnet.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-10-17_11:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1710170245","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"John Allen ,\n\tMichael Bringmann from Kernel Team ","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"}},{"id":1788708,"web_url":"http://patchwork.ozlabs.org/comment/1788708/","msgid":"","date":"2017-10-17T17:41:02","subject":"Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for\n\toperations","submitter":{"id":17146,"url":"http://patchwork.ozlabs.org/api/people/17146/","name":"Nathan Fontenot","email":"nfont@linux.vnet.ibm.com"},"content":"On 10/17/2017 12:22 PM, Michael Bringmann wrote:\n> \n> \n> On 10/17/2017 12:02 PM, Nathan Fontenot wrote:\n>> On 10/17/2017 11:14 AM, Michael Bringmann wrote:\n>>> See below.\n>>>\n>>> On 10/16/2017 07:33 AM, Michael Ellerman wrote:\n>>>> Michael Bringmann writes:\n>>>>\n>>>>> powerpc/nodes: On systems like PowerPC which allow 'hot-add' of CPU\n>>>>\n>>>> This is a powerpc-only patch, so saying \"systems like PowerPC\" is\n>>>> confusing. What you should be saying is \"On pseries systems\".\n>>>>\n>>>>> or memory resources, it may occur that the new resources are to be\n>>>>> inserted into nodes that were not used for these resources at bootup.\n>>>>> In the kernel, any node that is used must be defined and initialized\n>>>>> at boot.\n>>>>>\n>>>>> This patch extracts the value of the lowest domain level (number of\n>>>>> allocable resources) from the \"rtas\" device tree property\n>>>>> \"ibm,current-associativity-domains\" or the device tree property\n>>>>\n>>>> What is current associativity domains? I've not heard of it, where is it\n>>>> documented, and what does it mean.\n>>>>\n>>>> Why would use the \"current\" set vs the \"max\"? I thought the whole point\n>>>> was to discover the maximum possible set of nodes that could be\n>>>> hotplugged.\n>>>>\n>>>>> \"ibm,max-associativity-domains\" to use as the maximum number of nodes\n>>>>> to setup as possibly available in the system. This new setting will\n>>>>> override the instruction,\n>>>>>\n>>>>> nodes_and(node_possible_map, node_possible_map, node_online_map);\n>>>>>\n>>>>> presently seen in the function arch/powerpc/mm/numa.c:initmem_init().\n>>>>>\n>>>>> If the property is not present at boot, no operation will be performed\n>>>>> to define or enable additional nodes.\n>>>>>\n>>>>> Signed-off-by: Michael Bringmann \n>>>>> ---\n>>>>> arch/powerpc/mm/numa.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n>>>>> 1 file changed, 47 insertions(+)\n>>>>>\n>>>>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c\n>>>>> index ec098b3..b385cd0 100644\n>>>>> --- a/arch/powerpc/mm/numa.c\n>>>>> +++ b/arch/powerpc/mm/numa.c\n>>>>> @@ -892,6 +892,51 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)\n>>>>> \tNODE_DATA(nid)->node_spanned_pages = spanned_pages;\n>>>>> }\n>>>>> \n>>>>> +static void __init node_associativity_setup(void)\n>>>>\n>>>> This should really be called \"find_possible_nodes()\" or something more\n>>>> descriptive.\n>>>\n>>> Okay.\n>>>>\n>>>>> +{\n>>>>> +\tstruct device_node *rtas;\n>>>>> +\n>>>>> +\trtas = of_find_node_by_path(\"/rtas\");\n>>>>> +\tif (rtas) {\n>>>>\n>>>> If you just short-circuit that return the whole function body can be\n>>>> deintented, making it significantly more readable.\n>>>>\n>>>> ie:\n>>>> +\trtas = of_find_node_by_path(\"/rtas\");\n>>>> +\tif (!rtas)\n>>>> +\t\treturn;\n>>>\n>>> Okay.\n>>>\n>>>>\n>>>>> +\t\tconst __be32 *prop;\n>>>>> +\t\tu32 len, entries, numnodes, i;\n>>>>> +\n>>>>> +\t\tprop = of_get_property(rtas,\n>>>>> +\t\t\t\t\t\"ibm,current-associativity-domains\", &len);\n>>>>\n>>>> Please don't use of_get_property() in new code, we have much better\n>>>> accessors these days, which do better error checking and handle the\n>>>> endian conversions for you.\n>>>>\n>>>> In this case you'd use eg:\n>>>>\n>>>> \tu32 entries;\n>>>> \trc = of_property_read_u32(rtas, \"ibm,current-associativity-domains\", &entries);\n>>>\n>>> The property 'ibm,current-associativity-domains' has the same format as the property\n>>> 'ibm,max-associativity-domains' i.e. it is an integer array. The accessor of_property_read_32,\n>>> however, expects it to be an integer singleton value. Instead, it needs:\n>>\n>> I think for this case where the property is an array of values you could use\n>> of_property_count_elems_of_size() to get the number of elements in the array\n>> and then use of_property_read_u32_array() to read the array.\n>>\n>> -Nathan\n> \n> We only need one value from the array which is why I am using,\n> \n>>>>> +\t\tnumnodes = of_read_number(&prop[min_common_depth], 1);\n> \n> With this implementation I do not need to allocate memory for\n> an array, nor execute code to read all elements of the array.\n>\n> Michael\n\nOK, I didn't see that you just needed a single value from the array.\n\nIn this case you could do\n\n\tof_property_read_u32_index(rtas, \"ibm,current-associativity-domains\",\n\t\t\t\t min_common_depth, &numnodes);\n\n-Nathan\n\n> \n>>\n>>>\n>>>>\n>>>>> +\t\tif (!prop || len < sizeof(unsigned int)) {\n>>>>> +\t\t\tprop = of_get_property(rtas,\n>>>>> +\t\t\t\t\t\"ibm,max-associativity-domains\", &len);\n>>> if (!prop || len < sizeof(unsigned int))\n>>>>> +\t\t\t\tgoto endit;\n>>>>> +\t\t}\n>>>>> +\n>>>>> +\t\tentries = of_read_number(prop++, 1);\n>>>>> +\n>>>>> +\t\tif (len < (entries * sizeof(unsigned int)))\n>>>>> +\t\t\tgoto endit;\n>>>>> +\n>>>>> +\t\tif ((0 <= min_common_depth) && (min_common_depth <= (entries-1)))\n>>>>> +\t\t\tentries = min_common_depth;\n>>>>> +\t\telse\n>>>>> +\t\t\tentries -= 1;\n>>>> \t\t\t^\n>>>> You can't just guess that will be the right entry.\n>>>>\n>>>> If min_common_depth is < 0 the function should have just returned\n>>>> immediately at the top.\n>>>\n>>> Okay.\n>>>\n>>>>\n>>>> If min_common_depth is outside the range of the property that's a buggy\n>>>> device tree, you should print a warning and return.\n>>>>\n>>>>> +\t\tnumnodes = of_read_number(&prop[entries], 1);\n>>>>\n>>>> \tu32 num_nodes;\n>>>> \trc = of_property_read_u32_index(rtas, \"ibm,current-associativity-domains\", min_common_depth, &num_nodes);\n>>>>> +\n>>>>> +\t\tprintk(KERN_INFO \"numa: Nodes = %d (mcd = %d)\\n\", numnodes,\n>>>>> +\t\t\tmin_common_depth);\n>>>>> +\n>>>>> +\t\tfor (i = 0; i < numnodes; i++) {\n>>>>> +\t\t\tif (!node_possible(i)) {\n>>>>> +\t\t\t\tsetup_node_data(i, 0, 0);\n>>>>\n>>>> Do we actually need to setup the NODE_DATA() yet? Doing it now ensures\n>>>> it will not be allocated node local, which sucks.\n>>>\n>>> Okay.\n>>>\n>>>>\n>>>>> +\t\t\t\tnode_set(i, node_possible_map);\n>>>>> +\t\t\t}\n>>>>> +\t\t}\n>>>>> +\t}\n>>>>> +\n>>>>> +endit:\n>>>>\n>>>> \"out\" would be the normal name.\n>>>\n>>> Okay.\n>>>\n>>>>\n>>>>> +\tif (rtas)\n>>>>> +\t\tof_node_put(rtas);\n>>>>> +}\n>>>>> +\n>>>>> void __init initmem_init(void)\n>>>>> {\n>>>>> \tint nid, cpu;\n>>>>> @@ -911,6 +956,8 @@ void __init initmem_init(void)\n>>>>> \t */\n>>>>\n>>>> You need to update the comment above here which is contradicted by the\n>>>> new function you're adding.\n>>>\n>>> Okay.\n>>>\n>>>>\n>>>>> \tnodes_and(node_possible_map, node_possible_map, node_online_map);\n>>>>> \n>>>>> +\tnode_associativity_setup();\n>>>>> +\n>>>>> \tfor_each_online_node(nid) {\n>>>>> \t\tunsigned long start_pfn, end_pfn;\n>>>>> \n>>>>\n>>>> cheers\n>>>>\n>>>>\n>>>\n>>\n>>\n>","headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yGjGd5S85z9ryQ\n\tfor ;\n\tWed, 18 Oct 2017 04:43:37 +1100 (AEDT)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3yGjGd4KQbzDrJh\n\tfor ;\n\tWed, 18 Oct 2017 04:43:37 +1100 (AEDT)","from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com\n\t[148.163.158.5])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3yGjCp0fWrzDqlv\n\tfor ;\n\tWed, 18 Oct 2017 04:41:09 +1100 (AEDT)","from pps.filterd (m0098413.ppops.net [127.0.0.1])\n\tby mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv9HHePaj136188\n\tfor ; Tue, 17 Oct 2017 13:41:06 -0400","from e31.co.us.ibm.com (e31.co.us.ibm.com [32.97.110.149])\n\tby mx0b-001b2d01.pphosted.com with ESMTP id 2dnkudr8wn-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 17 Oct 2017 13:41:05 -0400","from localhost\n\tby e31.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ; \n\tTue, 17 Oct 2017 11:41:05 -0600","from b03cxnp07029.gho.boulder.ibm.com (9.17.130.16)\n\tby e31.co.us.ibm.com (192.168.1.131) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tTue, 17 Oct 2017 11:41:03 -0600","from b03ledav003.gho.boulder.ibm.com\n\t(b03ledav003.gho.boulder.ibm.com [9.17.130.234])\n\tby b03cxnp07029.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v9HHf3HP10224054; Tue, 17 Oct 2017 10:41:03 -0700","from b03ledav003.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 613716A042;\n\tTue, 17 Oct 2017 11:41:03 -0600 (MDT)","from [9.41.92.186] (unknown [9.41.92.186])\n\tby b03ledav003.gho.boulder.ibm.com (Postfix) with ESMTP id EA5B56A041;\n\tTue, 17 Oct 2017 11:41:02 -0600 (MDT)"],"Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=linux.vnet.ibm.com\n\t(client-ip=148.163.158.5; helo=mx0a-001b2d01.pphosted.com;\n\tenvelope-from=nfont@linux.vnet.ibm.com; receiver=)","Subject":"Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for\n\toperations","To":"Michael Bringmann ,\n\tMichael Ellerman , linuxppc-dev@lists.ozlabs.org, \n\tlinux-kernel@vger.kernel.org","References":"\n\t<871sm3l1c1.fsf@concordia.ellerman.id.au>\n\t\n\t<94ac2dd9-625c-d453-adf5-32703efaebcc@linux.vnet.ibm.com>\n\t<095d9828-b9d9-8dc7-8ef4-5dd7bf34c31d@linux.vnet.ibm.com>","From":"Nathan Fontenot ","Date":"Tue, 17 Oct 2017 12:41:02 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.4.0","MIME-Version":"1.0","In-Reply-To":"<095d9828-b9d9-8dc7-8ef4-5dd7bf34c31d@linux.vnet.ibm.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-TM-AS-GCONF":"00","x-cbid":"17101717-8235-0000-0000-00000C6CD412","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007907; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000237; SDB=6.00932516; UDB=6.00469605;\n\tIPR=6.00712806; \n\tBA=6.00005642; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017577;\n\tXFM=3.00000015; UTC=2017-10-17 17:41:05","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17101717-8236-0000-0000-00003E13DE17","Message-Id":"","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-10-17_12:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1710170249","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"John Allen ,\n\tMichael Bringmann from Kernel Team ","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"}}]