Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230112153348.3202173-1-pasha.tatashin@soleen.com> <20230112132957.6cb97c2afad17cdeb06fcd10@linux-foundation.org>
In-Reply-To: <20230112132957.6cb97c2afad17cdeb06fcd10@linux-foundation.org>
From:   Pasha Tatashin <pasha.tatashin@soleen.com>
Date:   Fri, 13 Jan 2023 09:44:02 -0500
Message-ID: <CA+CK2bB1TqWnafpfio6QQo5ifx54vyk+RVMxsVJr6pD+KAOVqA@mail.gmail.com>
Subject: Re: [PATCH v2] mm/page_ext: Do not allocate space for page_ext->flags
 if not needed
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     mhocko@suse.com, vbabka@suse.cz, david@redhat.com,
        quic_charante@quicinc.com, lizhe.67@bytedance.com,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jan 12, 2023 at 4:29 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 12 Jan 2023 15:33:48 +0000 Pasha Tatashin <pasha.tatashin@soleen.com> wrote:
>
> > There is 8 byte page_ext->flags field allocated per page whenever
> > CONFIG_PAGE_EXTENSION is enabled. However, not every user of page_ext
> > uses flags. Therefore, check whether flags is needed at least by one
> > user and if so allocate space for it.
> >
> > For example when page_table_check is enabled, on a machine with 128G
> > of memory before the fix:
> >
> > [    2.244288] allocated 536870912 bytes of page_ext
> > after the fix:
> > [    2.160154] allocated 268435456 bytes of page_ext
> >
>
> Looks nice.

Hi Andrew,

Thanks for looking at this patch.

>
> Does it work correctly with early_page_ext=1?

Yes, early_page_ext=1 moves page_ext_init() earlier, and disables
deferred page initialization, but that is not an issue for the changes
in this patch.

>
> > --- a/include/linux/page_ext.h
> > +++ b/include/linux/page_ext.h
> > @@ -12,10 +12,14 @@ struct page_ext_operations {
> >       size_t size;
> >       bool (*need)(void);
> >       void (*init)(void);
> > +     bool using_shared_ext_flags;
> >  };
>
> Seems overly complicated.  Can we change the three early-init
> functions such as setup_early_page_ext() to simply set some global bool
> flag?

We can't because , in the following loop:

       for (i = 0; i < entries; i++) {
               if (page_ext_ops[i]->need && page_ext_ops[i]->need()) {
                       page_ext_ops[i]->offset = page_ext_size;

page_ext_size must already either include or not the space for the
shared flag field.

>
> >  #endif
> >  };
> >
> > -unsigned long page_ext_size = sizeof(struct page_ext);
> > +unsigned long page_ext_size;
> >
> >  static unsigned long total_usage;
> >  static struct page_ext *lookup_page_ext(const struct page *page);
> > @@ -105,6 +106,15 @@ static bool __init invoke_need_callbacks(void)
> >       int entries = ARRAY_SIZE(page_ext_ops);
> >       bool need = false;
> >
> > +     for (i = 0; i < entries; i++) {
> > +             if (page_ext_ops[i]->need && page_ext_ops[i]->need()) {
>
> I don't think this (or the below) need to check for ->need==NULL?

Good point, I will remove the check, and also update the comment that
the "need" function must be defined for every item in the page_ext_ops
array.

Thanks,
Pasha