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

On 2026/08/18 4:58, Stephen Brennan wrote:
> 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.

hmm, personally I was thinking that it would be better to process compound
pages as compound page also for extensions, because that's the current
makedumpfile process model and simple, like

   PG_EXCLUDE -> exclude a (compound) page
   PG_INCLUDE -> include a (compound) page
   PG_INCLUDE_HEAD -> include only a head page

But looking at your (Stephen and Tao) extensions, apparently there are
cases where more flexibility is required for size efficiency or accuracy
in excluding/including pages.  So I understood that we have to accept a
certain complexity.

For now, I don't think of another way, please proceed with it.

Thanks,
Kazu

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