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

[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