Re: [PATCH 0/2] handle memoryless nodes more appropriately

[Qi Zheng <zhengqi.arch@bytedance.com>]

On 2023/2/16 16:37, Michal Hocko wrote:
> On Thu 16-02-23 16:21:54, Qi Zheng wrote:
>>
>>
>> On 2023/2/16 15:51, Michal Hocko wrote:
>>> On Thu 16-02-23 07:11:19, Qi Zheng wrote:
>>>>
>>>>
>>>> On 2023/2/16 00:36, Michal Hocko wrote:
>>>>> On Wed 15-02-23 23:24:10, Qi Zheng wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Currently, in the process of initialization or offline memory, memoryless
>>>>>> nodes will still be built into the fallback list of itself or other nodes.
>>>>>>
>>>>>> This is not what we expected, so this patch series removes memoryless
>>>>>> nodes from the fallback list entirely.
>>>>>>
>>>>>> Comments and suggestions are welcome.
>>>>
>>>> Hi Michal,
>>>>
>>>>>
>>>>> This is a tricky area full of surprises and it is really easy to
>>>>
>>>> Would you mind giving an example of a "new problem"?
>>>
>>> The initialization is spread over several places and it is quite easy to
>>> introduce bugs because it is hard to review this area. Been there done
>>> that. Just look into the git log.
>>
>> I understand your concern, but should we therefore reject all revisions
>> to this?
> 
> No, but either somebode is willing to invest a non-trivial amount of
> time and unify the NUMA initialization code that is spread over arch
> specific code in different places or we should just focus on addressing
> bugs.
> 
>>>>> introduce new problems. What kind of problem/issue are you trying to
>>>>> solve/handle by these changes?
>>>>
>>>> IIUC, I think there are two reasons:
>>>>
>>>> Firstly, as mentioned in commit message, the memoryless node has no
>>>> memory to allocate (If it can be allocated, it may also cause the panic
>>>> I mentioned in [1]), so we should not continue to traverse it when
>>>> allocating memory at runtime, which will have a certain overhead.
>>>
>>> Sure that is not the most optimal implementation but does this matter in
>>> practice? Can you observe any actual measurable performance penalty?
>>
>> No, and the original reason for noticing this place was the panic I
>> mentioned in [1] (< NODE_MIN_SIZE). And if we had handled the memoryless
>> node's zonelist correctly before, we wouldn't have had that panic at
>> all.
> 
> Yes, this is another good example of how subtle the code is. Mike has
> posted a patch that simply drops the NODE_MIN_SIZE constrain and I
> believe that is the right thing to do at this stage. There is a non-zero
> risk of regression but at least we will be forced to fix the original
> problem properly or at least document is properly.
> 
>>> Currently we are just sacrificing some tiny performance for a
>>> simplicity.
>> Hmm, I don't think my modification complicates the code.
>>
>>>> Secondly, from the perspective of semantic correctness, why do we remove
>>>> the memoryless node from the fallback list of other normal nodes
>>>> (N_MEMORY), but not from its own fallback list (PATCH[1/2])? Why should
>>>> an upcoming memoryless node continue exist in the fallback list of
>>>> itself and other normal nodes (PATCH[2/2])?
>>>
>>> I am not sure I follow. What is the semantic correctness issue?
>>
>> Sorry for the ambiguity, what I meant was that memoryless nodes should
>> never have been built into any fallback list, not just for performance
>> optimizations.
> 
> Well, I am not 100% sure I agree with you here. The performance would be
> the only reason why to drop those nodes from zonelists. Other than that
> zonelists are a useful abstraction for the node distance ordering. Even
> if those nodes do not have any memory at all in principle there is no
> big difference from depleted nodes.

I see what you mean, no more code for no more bugs (in cases where is no
obvious gain). But I still feel that the current implementation is
rather weird (deleted some, and kept some), and my changes are actually 
very small.

Anyway, let's wait for other people's opinions. :)

-- 
Thanks,
Qi