Messages in this thread | | | Date | Thu, 16 Feb 2023 18:50:55 +0800 | Subject | Re: [PATCH 0/2] handle memoryless nodes more appropriately | From | Qi Zheng <> |
| |
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
| |