[PATCH makedumpfile 4/9] Introduce a stat for pages retained by extension
Stephen Brennan
stephen.s.brennan at oracle.com
Thu Aug 20 12:35:21 PDT 2026
HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab at nec.com> writes:
> 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 <ltao at redhat.com> writes:
>>> Hi Kazu,
>>>
>>> On Thu, Aug 13, 2026 at 4:54 PM HAGIO KAZUHITO(萩尾 一仁)
>>> <k-hagio-ab at nec.com> 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 <stephen.s.brennan at oracle.com>
>>>>>> ---
>>>>>> 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 my own extensions: PG_INCLUDE_HEAD is enough. I do not actually have
an extension that would require processing individual tail pages.
Tao: do you have any extension that would require anything more
granular?
If nobody has a requirement for extensions processing individual tail
pages, then I think I have changed my mind: I now believe we should only
call extensions for tail pages, as this current series does.
Calling extensions for the tail pages may have a manageable cost in code
complexity. However, it also has a runtime cost. For large memory
systems which dedicate most of their memory to huge pages, makedumpfile
can skip over the tail pages, processing memory more efficiently.
With extensions, we would need to process each tail page in order to
allow extensions to handle them, so we cannot skip them in the loop. The
performance difference is definitely noticeable. I did a quick test on
my branch, without any extensions enabled. The test system has about 950
GiB (of 1 TiB) of memory dedicated to free 2M HugeTLB huge pages.
With this patch series (processing just compound heads).
(Similar results were found on the master branch).
No extensions enabled!
(core_collector makedumpfile -l --message-level 7 -d 31)
[ 13.017064] kdump[1146]: saving vmcore
[ 62.684150] kdump[1151]: saving vmcore complete
-> 49.66 second kdump operation
[ 13.124066] kdump[1150]: saving vmcore
[ 59.001598] kdump[1155]: saving vmcore complete
-> 45.88 second kdump operation
With the alternative approach (processing each sub-page)
No extensions enabled!
(core_collector makedumpfile -l --message-level 7 -d 31)
[ 12.888089] kdump[1148]: saving vmcore
[ 68.508773] kdump[1153]: saving vmcore complete
-> 55.62 second kdump operation
[ 12.755063] kdump[1144]: saving vmcore
[ 65.606558] kdump[1149]: saving vmcore complete
-> 52.85 second kdump operation
Looks like anywhere from 6-20% slowdown, dumping similar amounts of data
to a local filesystem with snappy compression. It is not huge, but it is
noticeable. Large memory systems really don't need anything further
slowing them down, and I know a lot of deployments care about the
benefit of huge pages for shrinking vmcores and reducing makedumpfile's
runtime (among other benefits, of course).
We could try to skip ahead whenever extensions are not loaded, but that
would mean that (1) the code must be even more complex, and (2) we would
still lose performance for extensions that don't want to handle tail
pages differently.
So given we haven't actually got a real use-case for anything other than
PG_INCLUDE_HEAD, let's stick to calling extensions for just the head
page. If we need another return status or another way for extensions to
return more specific decisions, we can handle that in the future.
I'll send the updated series with this approach and fixes from the code
review.
Thanks,
Stephen
> 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
>>>>>>
More information about the kexec
mailing list