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