Message-ID: <767893ef-f8c2-c478-f1a0-e785bbf2da09@bytedance.com>
Date:   Thu, 16 Feb 2023 16:21:54 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.7.2
Subject: Re: [PATCH 0/2] handle memoryless nodes more appropriately
Content-Language: en-US
To:     Michal Hocko <mhocko@suse.com>
Cc:     akpm@linux-foundation.org, vbabka@suse.cz, david@redhat.com,
        rppt@kernel.org, willy@infradead.org, mgorman@techsingularity.net,
        osalvador@suse.de, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Muchun Song <muchun.song@linux.dev>
References: <20230215152412.13368-1-zhengqi.arch@bytedance.com>
 <Y+0KKnN8BU6ky6oP@dhcp22.suse.cz>
 <3426457c-99bf-9f7c-f663-c29474d9fa73@bytedance.com>
 <Y+3gb/blCDJnQ0Ik@dhcp22.suse.cz>
From:   Qi Zheng <zhengqi.arch@bytedance.com>
In-Reply-To: <Y+3gb/blCDJnQ0Ik@dhcp22.suse.cz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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?

> 
>>> 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.

> 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.

> 

-- 
Thanks,
Qi