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. -- Michal Hocko SUSE Labs
