Re: [PATCH makedumpfile 3/9] Share page information with extension callbacks

[Stephen Brennan <stephen.s.brennan@oracle.com>]

Tao Liu <ltao@redhat.com> writes:
> Hi Stephen,
>
> On Tue, Jul 14, 2026 at 12:46 PM Stephen Brennan
> <stephen.s.brennan@oracle.com> wrote:
>>
>> In __exclude_unnecessary_pages(), we extract several fields related
>> to the page. Some of these, like compound_order and compound_dtor, have
>> logic specific to the kernel version.
>>
>> Extensions can, of course, determine these values for themselves, but
>> it's extra work, and duplicates logic that may need to be updated
>> frequently with new kernel versions. What's more, if we put all the
>> values together in a single structure, helpers like isSlab() and others
>> can be implemented in terms of that structure and shared with the
>> extensions in order to further simplify their implementation.
>>
>> With that in mind, group the per-page variables into a structure and
>> share them with extension callbacks. This breaks the extension API,
>> but since a release hasn't yet happened, it seems reasonable to do so.
>>
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>> ---
>>  extension.c    |  8 ++---
>>  extension.h    |  3 +-
>>  makedumpfile.c | 83 +++++++++++++++++++++++++-------------------------
>>  makedumpfile.h | 16 ++++++++++
>>  4 files changed, 64 insertions(+), 46 deletions(-)
>>
>> diff --git a/extension.c b/extension.c
>> index 5188c1f..9b29f0c 100644
>> --- a/extension.c
>> +++ b/extension.c
>> @@ -10,7 +10,7 @@
>>  #include "kallsyms.h"
>>  #include "btf_info.h"
>>
>> -typedef int (*callback_fn)(unsigned long, const void *);
>> +typedef int (*callback_fn)(unsigned long, const void *, const struct pginfo *);
>>
> The function signature of callbacks are changed, it will be better to
> update them in extensions/sample.c as well:
>
> int extension_callback(unsigned long pfn, const void *pcache)
> {
> return PG_UNDECID;
> }
>
> Since it will serve as a reference for future extension authors.

Hi Tao,

Thanks for the catch, I will send a v2 with this and a few other fixes.

Thanks,
Stephen

> Thaks,
> Tao Liu
>
>>  struct extension_handle_cb {
>>         void *handle;
>> @@ -306,14 +306,14 @@ fail:
>>   * 1) Include the page if anyone says PG_INCLUDE, and
>>   * 2) Exclude the page if no one says PG_INCLUDE, but one or more say PG_EXCLUDE.
>>   */
>> -int run_extension_callback(unsigned long pfn, const void *pcache)
>> +int run_extension_callback(unsigned long pfn, const void *pcache, const struct pginfo *inf)
>>  {
>>         int result;
>>         int ret = PG_UNDECID;
>>
>>         for (int i = 0; i < handle_cbs_len; i++) {
>>                 if (handle_cbs[i]->cb) {
>> -                       result = handle_cbs[i]->cb(pfn, pcache);
>> +                       result = handle_cbs[i]->cb(pfn, pcache, inf);
>>                         if (result == PG_INCLUDE) {
>>                                 ret = result;
>>                                 goto out;
>> @@ -341,7 +341,7 @@ bool add_extension_opts(char *opt)
>>         return false;
>>  }
>>
>> -int run_extension_callback(unsigned long pfn, const void *pcache)
>> +int run_extension_callback(unsigned long pfn, const void *pcache, const struct pginfo *i)
>>  {
>>         return PG_UNDECID;
>>  }
>> diff --git a/extension.h b/extension.h
>> index ba8d32a..22af9a6 100644
>> --- a/extension.h
>> +++ b/extension.h
>> @@ -2,12 +2,13 @@
>>  #define _EXTENSION_H
>>  #include <stdbool.h>
>>
>> +struct pginfo;
>>  enum {
>>         PG_INCLUDE,     // Exntesion will keep the page
>>         PG_EXCLUDE,     // Exntesion will discard the page
>>         PG_UNDECID,     // Exntesion makes no decision
>>  };
>> -int run_extension_callback(unsigned long pfn, const void *pcache);
>> +int run_extension_callback(unsigned long pfn, const void *pcache, const struct pginfo *i);
>>  void init_extensions(void);
>>  void cleanup_extensions(void);
>>  bool add_extension_opts(char *opt);
>> diff --git a/makedumpfile.c b/makedumpfile.c
>> index e882b84..a4c9bbf 100644
>> --- a/makedumpfile.c
>> +++ b/makedumpfile.c
>> @@ -6466,12 +6466,13 @@ __exclude_unnecessary_pages(unsigned long mem_map,
>>         mdf_pfn_t pfn_read_start, pfn_read_end;
>>         unsigned char *page_cache;
>>         unsigned char *pcache;
>> -       unsigned int _count, _mapcount = 0, compound_order = 0;
>> +       struct pginfo i;
>>         unsigned int order_offset, dtor_offset;
>> -       unsigned long flags, mapping, private = 0;
>> -       unsigned long compound_dtor, compound_head = 0;
>>         int filter_pg;
>>
>> +       i._mapcount = i.compound_order = 0;
>> +       i.private = i.compound_dtor = i.compound_head = 0;
>> +
>>         /*
>>          * If a multi-page exclusion is pending, do it first
>>          */
>> @@ -6543,21 +6544,21 @@ __exclude_unnecessary_pages(unsigned long mem_map,
>>                         pfn_read_end   = pfn + pfn_mm - 1;
>>                 }
>>
>> -               flags   = ULONG(pcache + OFFSET(page.flags));
>> -               _count  = UINT(pcache + OFFSET(page._refcount));
>> -               mapping = ULONG(pcache + OFFSET(page.mapping));
>> +               i.flags   = ULONG(pcache + OFFSET(page.flags));
>> +               i._count  = UINT(pcache + OFFSET(page._refcount));
>> +               i.mapping = ULONG(pcache + OFFSET(page.mapping));
>>
>>                 if (OFFSET(page._mapcount) != NOT_FOUND_STRUCTURE)
>> -                       _mapcount = UINT(pcache + OFFSET(page._mapcount));
>> +                       i._mapcount = UINT(pcache + OFFSET(page._mapcount));
>>
>> -               compound_order = 0;
>> -               compound_dtor = 0;
>> +               i.compound_order = 0;
>> +               i.compound_dtor = 0;
>>                 /*
>>                  * The last pfn of the mem_map cache must not be compound head
>>                  * page since all compound pages are aligned to its page order
>>                  * and PGMM_CACHED is a power of 2.
>>                  */
>> -               if ((index_pg < PGMM_CACHED - 1) && isCompoundHead(flags)) {
>> +               if ((index_pg < PGMM_CACHED - 1) && isCompoundHead(i.flags)) {
>>                         unsigned char *addr = pcache + SIZE(page);
>>
>>                         /*
>> @@ -6567,10 +6568,10 @@ __exclude_unnecessary_pages(unsigned long mem_map,
>>                         if (NUMBER(PAGE_HUGETLB_MAPCOUNT_VALUE) != NOT_FOUND_NUMBER) {
>>                                 unsigned long _flags_1 = ULONG(addr + OFFSET(page.flags));
>>
>> -                               compound_order = _flags_1 & 0xff;
>> +                               i.compound_order = _flags_1 & 0xff;
>>
>> -                               if (_mapcount == (int)NUMBER(PAGE_HUGETLB_MAPCOUNT_VALUE))
>> -                                       compound_dtor = IS_HUGETLB;
>> +                               if (i._mapcount == (int)NUMBER(PAGE_HUGETLB_MAPCOUNT_VALUE))
>> +                                       i.compound_dtor = IS_HUGETLB;
>>
>>                                 goto check_order;
>>                         }
>> @@ -6582,19 +6583,19 @@ __exclude_unnecessary_pages(unsigned long mem_map,
>>                         if (NUMBER(PG_hugetlb) != NOT_FOUND_NUMBER) {
>>                                 unsigned long _flags_1 = ULONG(addr + OFFSET(page.flags));
>>
>> -                               compound_order = _flags_1 & 0xff;
>> +                               i.compound_order = _flags_1 & 0xff;
>>
>>                                 if (_flags_1 & (1UL << NUMBER(PG_hugetlb)))
>> -                                       compound_dtor = IS_HUGETLB;
>> +                                       i.compound_dtor = IS_HUGETLB;
>>
>>                                 goto check_order;
>>                         }
>>
>>                         if (order_offset) {
>>                                 if (info->kernel_version >= KERNEL_VERSION(4, 16, 0))
>> -                                       compound_order = UCHAR(addr + order_offset);
>> +                                       i.compound_order = UCHAR(addr + order_offset);
>>                                 else
>> -                                       compound_order = USHORT(addr + order_offset);
>> +                                       i.compound_order = USHORT(addr + order_offset);
>>                         }
>>
>>                         if (dtor_offset) {
>> @@ -6603,40 +6604,40 @@ __exclude_unnecessary_pages(unsigned long mem_map,
>>                                  * to the ID of it since linux-4.4.
>>                                  */
>>                                 if (info->kernel_version >= KERNEL_VERSION(4, 16, 0))
>> -                                       compound_dtor = UCHAR(addr + dtor_offset);
>> +                                       i.compound_dtor = UCHAR(addr + dtor_offset);
>>                                 else if (info->kernel_version >= KERNEL_VERSION(4, 4, 0))
>> -                                       compound_dtor = USHORT(addr + dtor_offset);
>> +                                       i.compound_dtor = USHORT(addr + dtor_offset);
>>                                 else
>> -                                       compound_dtor = ULONG(addr + dtor_offset);
>> +                                       i.compound_dtor = ULONG(addr + dtor_offset);
>>                         }
>>  check_order:
>> -                       if ((compound_order >= sizeof(unsigned long) * 8)
>> -                           || ((pfn & ((1UL << compound_order) - 1)) != 0)) {
>> +                       if ((i.compound_order >= sizeof(unsigned long) * 8)
>> +                           || ((pfn & ((1UL << i.compound_order) - 1)) != 0)) {
>>                                 /* Invalid order */
>> -                               compound_order = 0;
>> +                               i.compound_order = 0;
>>                         }
>>                 }
>>                 if (OFFSET(page.compound_head) != NOT_FOUND_STRUCTURE)
>> -                       compound_head = ULONG(pcache + OFFSET(page.compound_head));
>> +                       i.compound_head = ULONG(pcache + OFFSET(page.compound_head));
>>
>>                 if (OFFSET(page.private) != NOT_FOUND_STRUCTURE)
>> -                       private = ULONG(pcache + OFFSET(page.private));
>> +                       i.private = ULONG(pcache + OFFSET(page.private));
>>
>> -               nr_pages = 1 << compound_order;
>> +               nr_pages = 1 << i.compound_order;
>>                 pfn_counter = NULL;
>>
>>                 /*
>>                  * Excludable compound tail pages must have already been excluded by
>>                  * exclude_range(), don't need to check them here.
>>                  */
>> -               if (compound_head & 1)
>> +               if (i.compound_head & 1)
>>                         continue;
>>
>>                 /*
>>                  * Include pages that specified by user via
>>                  * makedumpfile extensions
>>                  */
>> -               filter_pg = run_extension_callback(pfn, pcache);
>> +               filter_pg = run_extension_callback(pfn, pcache, &i);
>>                 if (filter_pg == PG_INCLUDE)
>>                         continue;
>>
>> @@ -6646,14 +6647,14 @@ check_order:
>>                  */
>>                 if ((info->dump_level & DL_EXCLUDE_FREE)
>>                     && info->page_is_buddy
>> -                   && info->page_is_buddy(flags, _mapcount, private, _count)) {
>> +                   && info->page_is_buddy(i.flags, i._mapcount, i.private, i._count)) {
>>                         if ((ARRAY_LENGTH(zone.free_area) != NOT_FOUND_STRUCTURE) &&
>> -                           (private >= ARRAY_LENGTH(zone.free_area))) {
>> +                           (i.private >= ARRAY_LENGTH(zone.free_area))) {
>>                                 MSG("WARNING: Invalid free page order: pfn=%llx, order=%lu, max order=%lu\n",
>> -                                   pfn, private, ARRAY_LENGTH(zone.free_area) - 1);
>> +                                   pfn, i.private, ARRAY_LENGTH(zone.free_area) - 1);
>>                                 continue;
>>                         }
>> -                       nr_pages = 1 << private;
>> +                       nr_pages = 1 << i.private;
>>                         pfn_counter = &pfn_free;
>>                 }
>>                 /*
>> @@ -6663,7 +6664,7 @@ check_order:
>>                  * accepted immediately without being on the list.
>>                  */
>>                 else if ((info->dump_level & DL_EXCLUDE_FREE)
>> -                       && isUnaccepted(_mapcount)) {
>> +                       && isUnaccepted(i._mapcount)) {
>>                         nr_pages = 1 << (ARRAY_LENGTH(zone.free_area) - 1);
>>                         pfn_counter = &pfn_free;
>>                 }
>> @@ -6671,17 +6672,17 @@ check_order:
>>                  * Exclude the non-private cache page.
>>                  */
>>                 else if ((info->dump_level & DL_EXCLUDE_CACHE)
>> -                   && is_cache_page(flags)
>> -                   && !isPrivate(flags) && !isAnon(mapping, flags, _mapcount)) {
>> +                   && is_cache_page(i.flags)
>> +                   && !isPrivate(i.flags) && !isAnon(i.mapping, i.flags, i._mapcount)) {
>>                         pfn_counter = &pfn_cache;
>>                 }
>>                 /*
>>                  * Exclude the cache page whether private or non-private.
>>                  */
>>                 else if ((info->dump_level & DL_EXCLUDE_CACHE_PRI)
>> -                   && is_cache_page(flags)
>> -                   && !isAnon(mapping, flags, _mapcount)) {
>> -                       if (isPrivate(flags))
>> +                   && is_cache_page(i.flags)
>> +                   && !isAnon(i.mapping, i.flags, i._mapcount)) {
>> +                       if (isPrivate(i.flags))
>>                                 pfn_counter = &pfn_cache_private;
>>                         else
>>                                 pfn_counter = &pfn_cache;
>> @@ -6692,19 +6693,19 @@ check_order:
>>                  *  - hugetlbfs pages
>>                  */
>>                 else if ((info->dump_level & DL_EXCLUDE_USER_DATA)
>> -                        && (isAnon(mapping, flags, _mapcount) || isHugetlb(compound_dtor))) {
>> +                        && (isAnon(i.mapping, i.flags, i._mapcount) || isHugetlb(i.compound_dtor))) {
>>                         pfn_counter = &pfn_user;
>>                 }
>>                 /*
>>                  * Exclude the hwpoison page.
>>                  */
>> -               else if (isHWPOISON(flags)) {
>> +               else if (isHWPOISON(i.flags)) {
>>                         pfn_counter = &pfn_hwpoison;
>>                 }
>>                 /*
>>                  * Exclude pages that are logically offline.
>>                  */
>> -               else if (isOffline(flags, _mapcount)) {
>> +               else if (isOffline(i.flags, i._mapcount)) {
>>                         pfn_counter = &pfn_offline;
>>                 }
>>                 /*
>> diff --git a/makedumpfile.h b/makedumpfile.h
>> index 4f707c7..87f973d 100644
>> --- a/makedumpfile.h
>> +++ b/makedumpfile.h
>> @@ -1507,6 +1507,22 @@ struct ppc64_vmemmap {
>>         unsigned long           virt;
>>  };
>>
>> +/* Per-page information determined during page filtering which may be useful
>> + * to extensions making their decisions */
>> +struct pginfo {
>> +       unsigned long flags;
>> +       unsigned long mapping;
>> +       /* Present whenever OFFSET(page.private) != NOT_FOUND_STRUCTURE */
>> +       unsigned long private;
>> +       unsigned long compound_dtor;
>> +       /* Present whenever OFFSET(page.compound_head) != NOT_FOUND_STRUCTURE */
>> +       unsigned long compound_head;
>> +       unsigned int _count;
>> +       /* Present whenever OFFSET(page._mapcount) != NOT_FOUND_STRUCTURE */
>> +       unsigned int _mapcount;
>> +       unsigned int compound_order;
>> +};
>> +
>>  struct DumpInfo {
>>         int32_t         kernel_version;      /* version of first kernel*/
>>         struct timeval  timestamp;
>> --
>> 2.47.3
>>