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

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;

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

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