Re: [PATCH v4 2/2] mm: Init page count in reserve_bootmem_region when MEMINIT_EARLY

[David Hildenbrand]

On 16.10.23 10:32, Yajun Deng wrote:
> On 2023/10/16 16:16, David Hildenbrand wrote:
> > On 16.10.23 10:10, Yajun Deng wrote:
> > > On 2023/10/16 14:33, Mike Rapoport wrote:
> > > > On Fri, Oct 13, 2023 at 05:29:19PM +0800, Yajun Deng wrote:
> > > > > On 2023/10/13 16:48, Mike Rapoport wrote:
> > > > > > On Thu, Oct 12, 2023 at 05:53:22PM +0800, Yajun Deng wrote:
> > > > > > > On 2023/10/12 17:23, David Hildenbrand wrote:
> > > > > > > > On 10.10.23 04:31, Yajun Deng wrote:
> > > > > > > > > On 2023/10/8 16:57, Yajun Deng wrote:
> > > > > > > > > > > That looks wrong. if the page count would by pure luck be 0
> > > > > > > > > > > already for hotplugged memory, you wouldn't clear the reserved
> > > > > > > > > > > flag.
> > > > > > > > > > >
> > > > > > > > > > > These changes make me a bit nervous.
> > > > > > > > > > Is 'if (page_count(page) || PageReserved(page))' be safer? Or
> > > > > > > > > > do I
> > > > > > > > > > need to do something else?
> > > > > > > > > How about the following if statement? But it needs to add more
> > > > > > > > > patch
> > > > > > > > > like v1 ([PATCH 2/4] mm: Introduce MEMINIT_LATE context).
> > > > > > > > >
> > > > > > > > > It'll be safer, but more complex. Please comment...
> > > > > > > > >
> > > > > > > > >         if (context != MEMINIT_EARLY || (page_count(page) ||
> > > > > > > > > PageReserved(page)) {
> > > > > > > > Ideally we could make initialization only depend on the context,
> > > > > > > > and not
> > > > > > > > check for count or the reserved flag.
> > > > > > > This link is v1,
> > > > > > > https://lore.kernel.org/all/20230922070923.355656-1-yajun.deng@xxxxxxxxx/
> > > > > > >
> > > > > > >
> > > > > > > If we could make initialization only depend on the context, I'll
> > > > > > > modify it
> > > > > > > based on v1.
> > > > > > Although ~20% improvement looks impressive, this is only
> > > > > > optimization of a
> > > > > > fraction of the boot time, and realistically, how much 56 msec
> > > > > > saves from
> > > > > > the total boot time when you boot a machine with 190G of RAM?
> > > > > There are a lot of factors that can affect the total boot time. 56
> > > > > msec
> > > > > saves may be insignificant.
> > > > >
> > > > > But if we look at the boot log, we'll see there's a significant
> > > > > time jump.
> > > > >
> > > > > before:
> > > > >
> > > > > [    0.250334] ACPI: PM-Timer IO Port: 0x508
> > > > > [    0.618994] Memory: 173413056K/199884452K available (18440K
> > > > > kernel code,
> > > > >
> > > > > after:
> > > > >
> > > > > [    0.260229] software IO TLB: area num 32.
> > > > > [    0.563497] Memory: 173413056K/199884452K available (18440K
> > > > > kernel code,
> > > > > Memory:
> > > > > Memory initialization is time consuming in the boot log.
> > > > You just confirmed that 56 msec is insignificant and then you send
> > > > again
> > > > the improvement of ~60 msec in memory initialization.
> > > >
> > > > What does this improvement gain in percentage of total boot time?
> > >
> > >
> > > before:
> > >
> > > [   10.692708] Run /init as init process
> > >
> > >
> > > after:
> > >
> > > [   10.666290] Run /init as init process
> > >
> > >
> > > About 0.25%. The total boot time is variable, depending on how many
> > > drivers need to be initialized.
> > >
> > >
> > > > > > I still think the improvement does not justify the churn, added
> > > > > > complexity
> > > > > > and special casing of different code paths of initialization of
> > > > > > struct pages.
> > > > >
> > > > > Because there is a loop, if the order is MAX_ORDER, the loop will
> > > > > run 1024
> > > > > times. The following 'if' would be safer:
> > > > >
> > > > > 'if (context != MEMINIT_EARLY || (page_count(page) || >>
> > > > > PageReserved(page))
> > > > > {'
> > > > No, it will not.
> > > >
> > > > As the matter of fact any condition here won't be 'safer' because it
> > > > makes
> > > > the code more complex and less maintainable.
> > > > Any future change in __free_pages_core() or one of it's callers will
> > > > have
> > > > to reason what will happen with that condition after the change.
> > >
> > >
> > > To avoid introducing MEMINIT_LATE context and make code simpler. This
> > > might be a better option.
> > >
> > > if (page_count(page) || PageReserved(page))
> >
> > I'll have to side with Mike here; this change might not be worth it.
>
> Okay, I got it. Thanks!

IMHO instead of adding more checks to that code we should try to unify 
that handling such that we can just remove it. As expressed, at least 
from the memory hotplug perspective there are still reasons why we need 
that; I can provide some guidance on how to eventually achieve that, but 
it might end up in a bit of work ...

Anyhow, thanks for bringing up that topic; it reminded me that I still 
have pending cleanups to not rely on PageReserved on the memory hotplug 
path.

--
Cheers,

David / dhildenb