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

Hello Tao, Kazu,

I apologize for not responding on this patch's review sooner.
Thank you both for reviewing carefully.

Tao Liu <[email protected]> writes:
> 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.

Yes, as you noted in (1) the current design is that we detect compound
tail pages near the beginning of the loop, so we don't need to adjust
pfn. (For option (2), both mem_map and pfn must be adjusted, and they
must be incremented by (nr_pages - 1), not nr_pages. We must be really
careful when skipping the loop forward or else we corrupt the vmcore,
which is why I relied on option (1) in this patch.)

>> (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;

Hi Kazu,

Yes, I agree this is a problem. if PG_EXCLUDE sets nr_pages = 1, then
extensions cannot exclude tail pages.

We could drop this "nr_pages = 1" line here. But I think this could be
more completely fixed by Tao's approach below.

> 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.

Yes, I thought this was your intention. But the current code (prior to
this series) doesn't respect tail page decisions, as described in the
Patch 1 body. The current code makes decisions by compound page, not by
individual PFN.

It is possible to handle each PFN in the way you suggest. It makes the
extension API simpler and more flexible, but it makes
__exclude_unnecessary_pages() more complex:

1. We need a special case in the middle of the loop to handle
   excluded/included compound tails. In this special case, we need
   special bookkeeping to ensure we decrement the correct PFN counter
   for PG_INCLUDE pages which were previously excluded.

2. Pending multi-page exclusions at the beginning of a cycle must still
   be performed, but the loop must still process them to ensure that
   extensions have an opportunity to re-include them.

3. When extensions return PG_INCLUDE on a head page, yet makedumpfile
   would exclude the compound page, we need a special case to ensure
   that we exclude all the tail pages.

When I originally decided to implement it the way shown in this PR
(calling extensions only for the compound head), I thought that doing it
your way would be too complex. Today I tried to implement it and found
that it's not too bad. Here's a version of this patch series which calls
extensions for each PFN:

https://github.com/makedumpfile/makedumpfile/compare/master...brenns10:makedumpfile:userstack_alternative_api?expand=1

In particular, this commit is the one that matters:

https://github.com/makedumpfile/makedumpfile/commit/a6695155a58ff5a3078e38deeaf768adbc7cbdbf

The nice thing about this is that, with "struct pginfo", extensions can
now very easily test whether a page is a compound_head or not, without
relying on re-implementing makedumpfile's version-specific logic. So
this very nicely allows extensions to implement any exclusion they want:
they can either repeat their decision for each compound tail, or they
can do something more complex.

If Kazu is willing to accept this modest complexity for
__exclude_unnecessary_pages(), then I think it's worth it for the added
flexibility, and I would be happy to send that instead as v2 of this
series.

> 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".

I agree, thank you for that. I will change the order of the lines in v2.

Thank you,
Stephen

>> > Thanks,
>> > Tao Liu
>> >> --
>> >> 2.47.3
>> >>