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

[Tao Liu <ltao@redhat.com>]

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

> +		}
> +
>  		/*
>  		 * 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
>