Re: [PATCH makedumpfile 4/9] Introduce a stat for pages retained by extension

Hi Kazu,

On Thu, Aug 13, 2026 at 4:54 PM HAGIO KAZUHITO(萩尾　一仁)
<[email protected]> wrote:
>
> On 2026/08/13 11:15, Tao Liu wrote:
> > Hi Stephen,
> >
> > On Mon, Jul 13, 2026 at 05:45:37PM -0700, Stephen Brennan wrote:
> >> Extensions can mark pages to be excluded, but those pages may already be
> >> excluded due to the dump level. We have a statistic to count pages
> >> excluded by extensions. It counts only pages which were excluded because
> >> no other criteria excluded them.
> >>
> >> Extensions can mark pages to be retained, but there is no statistic to
> >> count them. Adding a counter to the code as-is would not give us the
> >> value that we care about. Just as above, pages marked for inclusion may
> >> have been included anyway due to the dump-level configuration. The most
> >> useful statistic is the one that tells us how many pages were included
> >> by the extension, which would not have been included otherwise.
> >>
> >> Introduce a statistic that counts this amount.  To do so, we have to
> >> skip the short-circuit evaluation when PG_INCLUDE is returned. This
> >> seems like a worthwhile trade-off, since the dump-level checks are all
> >> reasonably efficient.
> >>
> >> Signed-off-by: Stephen Brennan <[email protected]>
> >> ---
> >>   makedumpfile.c | 13 +++++++++++--
> >>   1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/makedumpfile.c b/makedumpfile.c
> >> index a4c9bbf..cf6a38f 100644
> >> --- a/makedumpfile.c
> >> +++ b/makedumpfile.c
> >> @@ -106,6 +106,7 @@ mdf_pfn_t pfn_elf_excluded;
> >>   mdf_pfn_t pfn_extension;
> >>
> >>   mdf_pfn_t num_dumped;
> >> +mdf_pfn_t num_extension_retained;
> >>
> >>   int retcd = FAILED;        /* return code */
> >>
> >> @@ -6638,8 +6639,6 @@ check_order:
> >>               * makedumpfile extensions
> >>               */
> >>              filter_pg = run_extension_callback(pfn, pcache, &i);
> >> -            if (filter_pg == PG_INCLUDE)
> >> -                    continue;
> >>
> >>              /*
> >>               * Exclude the free page managed by a buddy
> >> @@ -6722,6 +6721,13 @@ check_order:
> >>              else
> >>                      continue;
> >>
> >> +            if (filter_pg == PG_INCLUDE) {
> >> +                    /* Account pages which would have been excluded, but were
> >> +                     * retained by an extension. */
> >> +                    num_extension_retained += nr_pages;
> >> +                    continue;
> >
> > Maybe I'm wrong, from the code we are trying to retain nr_pages, don't
> > we need to do
> >                       pfn += nr_pages
> > to update the pfn of the next for-loop?
>
> Thank you for your review, Tao.
>
> The current code below does not use nr_pages to skip tail pages to be
> retained, so I think this patch's code matches this.
>
>                  /*
>                   * Unexcludable page
>                   */
>                  else
>                          continue;
>
After rethinking, I guess both ways work:

1)
if (filter_pg == PG_INCLUDE) {
  num_extension_retained += nr_pages;
  continue;
}
Then in the next for-loop, all the tail pages(for-loop nr_pages times)
will hit continue:
if (i.compound_head & 1)
  continue;
so all pages are kept within vmcore.

2)
if (filter_pg == PG_INCLUDE) {
  num_extension_retained += nr_pages;
  pfn += nr_pages
  continue;
}
Then in the next for-loop, all tail pages are skiped(only for-loop 1 time).

The effect is the same, all the nr_pages are hit by continue, so all kept.

> (Retained tail pages will be skipped at compound_head check.  If we
> change this behavior, a separate patch would be preferable.)
>
> But on the other hand, if we set nr_pages to 1 when PG_EXCLUDE,
> extensions cannot exclude tail pages?
>
>                  else if (filter_pg == PG_EXCLUDE) {
>                          nr_pages = 1;
>                          pfn_counter = &pfn_extension;

In fact, I didn't consider the case of compond pages when implementing
makedumpfile extension. My original thought was, makedumpfile pass
information of page one-by-one to extensions, allowing the extension
to decide whether to keep or discard it. Thus the filtering logic for
makedumpfile would be much simpler.

Extension however, check page type(compound page or normal page) by
itself. E.g. if it want to exclude a compond page:

makedumpfile:

for (pfn;;pfn++) {
  run_extension_callback(pfn, pcache, &i);
  ...
}

extension_call_back:
if ((check_page_type(pfn, pcache, page_info)) == compond_page) {
    record_compond_range(pfn, pfn + nr_pages);
}
if (pfn within compond_range) {
  return PG_EXCLUDE;
}

In this way, I guess we even don't need to introduce PG_INCLUDE_HEAD
to makedumpfile, because it will be the same as PG_INCLUDE.

Thanks,
Tao Liu

>
> Thanks,
> Kazu
>
> >
> >> +            }
> >> +
> >>              /*
> >>               * Execute exclusion
> >>               */
> >> @@ -8265,6 +8271,7 @@ write_elf_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_page)
> >>      if (info->flag_cyclic) {
> >>              pfn_zero = pfn_cache = pfn_cache_private = 0;
> >>              pfn_user = pfn_free = pfn_hwpoison = pfn_offline = pfn_extension = 0;
> >> +            num_extension_retained = 0;
> >>              pfn_memhole = info->max_mapnr;
> >>      }
> >>
> >> @@ -9610,6 +9617,7 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
> >>               */
> >>              pfn_zero = pfn_cache = pfn_cache_private = 0;
> >>              pfn_user = pfn_free = pfn_hwpoison = pfn_offline = pfn_extension = 0;
> >> +            num_extension_retained = 0;
> >>              pfn_memhole = info->max_mapnr;
> >>
> >>              /*
> >> @@ -10575,6 +10583,7 @@ print_report(void)
> >>      REPORT_MSG("    Hwpoison pages          : 0x%016llx\n", pfn_hwpoison);
> >>      REPORT_MSG("    Offline pages           : 0x%016llx\n", pfn_offline);
> >>      REPORT_MSG("    Extension filter pages  : 0x%016llx\n", pfn_extension);
> >> +    REPORT_MSG("  Retained by extension     : 0x%016llx\n", num_extension_retained);
> >>      REPORT_MSG("  Remaining pages  : 0x%016llx\n",
> >>          pfn_original - pfn_excluded);
> >>
> > I suggest to reorder the "print_report" as follows:
> >
> > Original pages  :
> >    Excluded pages   :
> >      Pages filled with zero  :
> >      Non-private cache pages :
> >      Private cache pages     :
> >      User process data pages :
> >      Free pages              :
> >      Hwpoison pages          :
> >      Offline pages           :
> >      Extension filter pages  :
> >    Remaining pages  :
> >      Extension retain pages  :
> >
> > IMHO, this is clearer to represent "Original pages" == "Excluded pages" + "Remaining pages";
> > and "Extension retained pages" is a subset of "Remaining pages".
> >
> > Thanks,
> > Tao Liu
> >> --
> >> 2.47.3
> >>