On 2017/11/29 21:33, Michal Hocko wrote:
> On Wed 29-11-17 21:26:19, zhong jiang wrote:
>> On 2017/11/29 21:01, Michal Hocko wrote:
>>> On Wed 29-11-17 20:41:25, zhong jiang wrote:
>>>> On 2017/11/29 20:03, Michal Hocko wrote:
>>>>> On Wed 29-11-17 17:13:27, zhong jiang wrote:
>>>>>> Currently, Arm64 and x86 use the common code wehn parsing numa node
>>>>>> in a acpi way. The arm64 will set the parsed node in numa_add_memblk,
>>>>>> but the x86 is not set in that , then it will result in the repeatly
>>>>>> setting. And the parsed node maybe is unreasonable to the system.
>>>>>>
>>>>>> we would better not set it although it also still works. because the
>>>>>> parsed node is unresonable. so we should skip related operate in this
>>>>>> node. This patch just set node in various architecture individually.
>>>>>> it is no functional change.
>>>>> I really have hard time to understand what you try to say above. Could
>>>>> you start by the problem description and then how you are addressing it?
>>>> I am so sorry for that. I will make the issue clear.
>>>>
>>>> Arm64 get numa information through acpi. The code flow is as follows.
>>>>
>>>> arm64_acpi_numa_init
>>>> acpi_parse_memory_affinity
>>>> acpi_numa_memory_affinity_init
>>>> numa_add_memblk(nid, start, end); //it will set node to numa_nodes_parsed successfully.
>>>> node_set(node, numa_nodes_parsed); // numa_add_memblk had set that. it will repeat.
>>>>
>>>> the root cause is that X86 parse numa also go through above code. and arch-related
>>>> numa_add_memblk is not set the parsed node to numa_nodes_parsed. it need
>>>> additional node_set(node, numa_parsed) to handle. therefore, the issue will be introduced.
>>>>
>>> No it is not much more clear. I would have to go and re-study the whole
>>> code flow to see what you mean here. So you could simply state what _the
>>> issue_ is? How can user observe it and what are the consequences?
>> The patch do not fix a real issue. it is a cleanup.
>> because the acpi code is public, I find they are messy between
>> Arch64 and X86 when parsing numa message . therefore, I try to
>> make the code more clear between them.
> So make this explicit in the changelog. Your previous wording sounded
> like there is a _problem_ in the code.
>
:-[ please take some time to check. if it works. I will resend v2 with detailed changelog.
Thanks
zhongjiang
From 1585425299217537826@xxx Wed Nov 29 18:16:28 +0000 2017
X-GM-THRID: 1585391832743441876
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
Hi Jiang,
At 11/29/2017 09:44 PM, zhong jiang wrote:
> On 2017/11/29 21:33, Michal Hocko wrote:
>> On Wed 29-11-17 21:26:19, zhong jiang wrote:
>>> On 2017/11/29 21:01, Michal Hocko wrote:
>>>> On Wed 29-11-17 20:41:25, zhong jiang wrote:
>>>>> On 2017/11/29 20:03, Michal Hocko wrote:
>>>>>> On Wed 29-11-17 17:13:27, zhong jiang wrote:
>>>>>>> Currently, Arm64 and x86 use the common code wehn parsing numa node
>>>>>>> in a acpi way. The arm64 will set the parsed node in numa_add_memblk,
>>>>>>> but the x86 is not set in that , then it will result in the repeatly
>>>>>>> setting. And the parsed node maybe is unreasonable to the system.
>>>>>>>
>>>>>>> we would better not set it although it also still works. because the
>>>>>>> parsed node is unresonable. so we should skip related operate in this
>>>>>>> node. This patch just set node in various architecture individually.
>>>>>>> it is no functional change.
>>>>>> I really have hard time to understand what you try to say above. Could
>>>>>> you start by the problem description and then how you are addressing it?
>>>>> I am so sorry for that. I will make the issue clear.
>>>>>
>>>>> Arm64 get numa information through acpi. The code flow is as follows.
>>>>>
>>>>> arm64_acpi_numa_init
>>>>> acpi_parse_memory_affinity
>>>>> acpi_numa_memory_affinity_init
>>>>> numa_add_memblk(nid, start, end); //it will set node to numa_nodes_parsed successfully.
>>>>> node_set(node, numa_nodes_parsed); // numa_add_memblk had set that. it will repeat.
>>>>>
>>>>> the root cause is that X86 parse numa also go through above code. and arch-related
>>>>> numa_add_memblk is not set the parsed node to numa_nodes_parsed. it need
>>>>> additional node_set(node, numa_parsed) to handle. therefore, the issue will be introduced.
>>>>>
>>>> No it is not much more clear. I would have to go and re-study the whole
>>>> code flow to see what you mean here. So you could simply state what _the
>>>> issue_ is? How can user observe it and what are the consequences?
>>> The patch do not fix a real issue. it is a cleanup.
> @@ -294,7 +294,9 @@ void __init acpi_numa_slit_init(struct
acpi_table_slit *slit)
> goto out_err_bad_srat;
> }
>
> - node_set(node, numa_nodes_parsed);
> + /* some architecture is likely to ignore a unreasonable node */
> + if (!node_isset(node, numa_nodes_parsed))
> + goto out;
>
It is not just a cleanup patch, Here you change the original logic.
With this patch, we just set the *numa_nodes_parsed* after NUMA adds a
memblk successfully and also add a check here for bypassing the invalid
memblk node.
I am not sure which arch may meet this situation? did you test this
patch?
Anyway, AFAIK, The ACPI tables are very much like user input in that
respect and they are unreasonable. So the patch is better.
Thanks,
dou.
>>> because the acpi code is public, I find they are messy between
>>> Arch64 and X86 when parsing numa message . therefore, I try to
>>> make the code more clear between them.
>> So make this explicit in the changelog. Your previous wording sounded
>> like there is a _problem_ in the code.
>>
> :-[ please take some time to check. if it works. I will resend v2 with detailed changelog.
>
> Thanks
> zhongjiang
>
>
>
>
From 1585409727607941634@xxx Wed Nov 29 14:08:58 +0000 2017
X-GM-THRID: 1585391832743441876
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread