Messages in this thread Patch in this message | | | From | Pasha Tatashin <> | Date | Fri, 13 Jan 2023 09:44:02 -0500 | Subject | Re: [PATCH v2] mm/page_ext: Do not allocate space for page_ext->flags if not needed |
| |
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
| |