[PATCH 0/2] makedumpfile: improve write_kdump_pages_cyclic

[PATCH 0/2] makedumpfile: improve write_kdump_pages_cyclic

[Philipp Rudo]

Hi,

I noticed that on s390 makedumpfile sometimes floods the console with

Copying data       : [100.0 %] -           eta: 0s

causing the dump process to slow down dramatically. The patches attached are
two somewhat separate approaches to fix this issue. In my opinion both patches
make sense to be included even when one of them would be enough to fix the
problem described above.

Thanks
Philipp

Philipp Rudo (2):
  makedumpfile: omit unnecessary calls to print_progress
  makedumpfile: break loop after last dumpable page

 makedumpfile.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

-- 
2.35.1

[PATCH 1/2] makedumpfile: omit unnecessary calls to print_progress

[Philipp Rudo]

Check first if a page is dumpable before printing the process. Otherwise
there is the chance that num_dumped % per == 0 at the beginning of the
block of undampable pages. In that case num_dumped isn't updated before
the next dumpable page and thus print_process is called for every page
in that block.

This is especially annoying when the block is after the last dumpable
page and thus num_dumped == info->num_dumpable. In that case
print_process will bypass its check to only print the process once every
second and thus flood the console with unnecessary prints. This can lead
to a severe decrease in performance especially when the console is in
line mode.

Signed-off-by: Philipp Rudo <prudo@redhat.com>
---
 makedumpfile.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 14556db..2ef3458 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -8884,16 +8884,16 @@ write_kdump_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_pag
 
 	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
 
-		if ((num_dumped % per) == 0)
-			print_progress(PROGRESS_COPY, num_dumped, info->num_dumpable, &ts_start);
-
 		/*
 		 * Check the excluded page.
 		 */
 		if (!is_dumpable(info->bitmap2, pfn, cycle))
 			continue;
 
+		if ((num_dumped % per) == 0)
+			print_progress(PROGRESS_COPY, num_dumped, info->num_dumpable, &ts_start);
 		num_dumped++;
+
 		if (!read_pfn(pfn, buf))
 			goto out;
 
-- 
2.35.1

[PATCH 1/2] makedumpfile: omit unnecessary calls to print_progress

[HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?=]

Hi Philipp,

-----Original Message-----
> Check first if a page is dumpable before printing the process. Otherwise
> there is the chance that num_dumped % per == 0 at the beginning of the
> block of undampable pages. In that case num_dumped isn't updated before
> the next dumpable page and thus print_process is called for every page
> in that block.
> 
> This is especially annoying when the block is after the last dumpable
> page and thus num_dumped == info->num_dumpable. In that case
> print_process will bypass its check to only print the process once every
> second and thus flood the console with unnecessary prints. This can lead
> to a severe decrease in performance especially when the console is in
> line mode.

Good catch and improvement, will merge this.

Thanks,
Kazu

> 
> Signed-off-by: Philipp Rudo <prudo@redhat.com>
> ---
>  makedumpfile.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index 14556db..2ef3458 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -8884,16 +8884,16 @@ write_kdump_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_pag
> 
>  	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
> 
> -		if ((num_dumped % per) == 0)
> -			print_progress(PROGRESS_COPY, num_dumped, info->num_dumpable, &ts_start);
> -
>  		/*
>  		 * Check the excluded page.
>  		 */
>  		if (!is_dumpable(info->bitmap2, pfn, cycle))
>  			continue;
> 
> +		if ((num_dumped % per) == 0)
> +			print_progress(PROGRESS_COPY, num_dumped, info->num_dumpable, &ts_start);
>  		num_dumped++;
> +
>  		if (!read_pfn(pfn, buf))
>  			goto out;
> 
> --
> 2.35.1

[PATCH 2/2] makedumpfile: break loop after last dumpable page

[Philipp Rudo]

Once the last dumpable page was processed there is no need to finish the
loop to the last page. Thus exit early to improve performance.

Signed-off-by: Philipp Rudo <prudo@redhat.com>
---
 makedumpfile.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/makedumpfile.c b/makedumpfile.c
index 2ef3458..c944d0e 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -8884,6 +8884,12 @@ write_kdump_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_pag
 
 	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
 
+		/*
+		 * All dumpable pages have been processed. No need to continue.
+		 */
+		if (num_dumped == info->num_dumpable)
+			break;
+
 		/*
 		 * Check the excluded page.
 		 */
-- 
2.35.1

[PATCH 2/2] makedumpfile: break loop after last dumpable page

[HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?=]

-----Original Message-----
> Once the last dumpable page was processed there is no need to finish the
> loop to the last page. Thus exit early to improve performance.
> 
> Signed-off-by: Philipp Rudo <prudo@redhat.com>
> ---
>  makedumpfile.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index 2ef3458..c944d0e 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -8884,6 +8884,12 @@ write_kdump_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_pag
> 
>  	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
> 
> +		/*
> +		 * All dumpable pages have been processed. No need to continue.
> +		 */
> +		if (num_dumped == info->num_dumpable)
> +			break;

This patch is likely to increase the possibility of failure to capture
/proc/kcore, although this is an unofficial functionality...

  # makedumpfile -ld31 /proc/kcore kcore.snap
  # crash vmlinux kcore.snap
  ...
  crash: page incomplete: kernel virtual address: ffff916fbeffed00  type: "pglist node_id"

In cyclic mode, makedumpfile first calculates only info->num_dumpable [1] and
frees the used bitmap, and later creates 2nd bitmap again [2] at this time.

  create_dumpfile
    create_dump_bitmap
      info->num_dumpable = get_num_dumpable_cyclic()  <<-- [1]
    writeout_dumpfile
      write_kdump_pages_and_bitmap_cyclic
        foreach cycle
          create_2nd_bitmap  <<-- [2]
          write_kdump_pages_cyclic

So with live system, num_dumped can exceed info->num_dumpable.
If it stops at info->num_dumpable, some necessary data can be missed.

Capturing /proc/kcore is still fragile and not considered enough, but
sometimes useful... when I want to capture a snapshot of memory.

(the bitmap is allocated as block, so probably it's working as some buffer?)

So I will merge the 1/2 patch, but personally would not like to merge
this patch.  How necessary is this?

Thanks,
Kazu

[PATCH 2/2] makedumpfile: break loop after last dumpable page

[HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?=]

-----Original Message-----
> -----Original Message-----
> > Once the last dumpable page was processed there is no need to finish the
> > loop to the last page. Thus exit early to improve performance.
> >
> > Signed-off-by: Philipp Rudo <prudo@redhat.com>
> > ---
> >  makedumpfile.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/makedumpfile.c b/makedumpfile.c
> > index 2ef3458..c944d0e 100644
> > --- a/makedumpfile.c
> > +++ b/makedumpfile.c
> > @@ -8884,6 +8884,12 @@ write_kdump_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_pag
> >
> >  	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
> >
> > +		/*
> > +		 * All dumpable pages have been processed. No need to continue.
> > +		 */
> > +		if (num_dumped == info->num_dumpable)
> > +			break;
> 
> This patch is likely to increase the possibility of failure to capture
> /proc/kcore, although this is an unofficial functionality...
> 
>   # makedumpfile -ld31 /proc/kcore kcore.snap
>   # crash vmlinux kcore.snap
>   ...
>   crash: page incomplete: kernel virtual address: ffff916fbeffed00  type: "pglist node_id"
> 
> In cyclic mode, makedumpfile first calculates only info->num_dumpable [1] and
> frees the used bitmap, and later creates 2nd bitmap again [2] at this time.
> 
>   create_dumpfile
>     create_dump_bitmap
>       info->num_dumpable = get_num_dumpable_cyclic()  <<-- [1]
>     writeout_dumpfile
>       write_kdump_pages_and_bitmap_cyclic
>         foreach cycle
>           create_2nd_bitmap  <<-- [2]
>           write_kdump_pages_cyclic
> 
> So with live system, num_dumped can exceed info->num_dumpable.
> If it stops at info->num_dumpable, some necessary data can be missed.
> 
> Capturing /proc/kcore is still fragile and not considered enough, but
> sometimes useful... when I want to capture a snapshot of memory.
> 
> (the bitmap is allocated as block, so probably it's working as some buffer?)

sorry, mistake.
(page descriptors are aligned by block, ...)

> 
> So I will merge the 1/2 patch, but personally would not like to merge
> this patch.  How necessary is this?
> 
> Thanks,
> Kazu
> 
> 
> _______________________________________________
> kexec mailing list
> kexec at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 2/2] makedumpfile: break loop after last dumpable page

[Philipp Rudo]

Hi Kazu,

On Thu, 7 Apr 2022 06:43:00 +0000
HAGIO KAZUHITO(?????) <k-hagio-ab@nec.com> wrote:

> -----Original Message-----
> > Once the last dumpable page was processed there is no need to finish the
> > loop to the last page. Thus exit early to improve performance.
> > 
> > Signed-off-by: Philipp Rudo <prudo@redhat.com>
> > ---
> >  makedumpfile.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/makedumpfile.c b/makedumpfile.c
> > index 2ef3458..c944d0e 100644
> > --- a/makedumpfile.c
> > +++ b/makedumpfile.c
> > @@ -8884,6 +8884,12 @@ write_kdump_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_pag
> > 
> >  	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
> > 
> > +		/*
> > +		 * All dumpable pages have been processed. No need to continue.
> > +		 */
> > +		if (num_dumped == info->num_dumpable)
> > +			break;  
> 
> This patch is likely to increase the possibility of failure to capture
> /proc/kcore, although this is an unofficial functionality...
> 
>   # makedumpfile -ld31 /proc/kcore kcore.snap
>   # crash vmlinux kcore.snap
>   ...
>   crash: page incomplete: kernel virtual address: ffff916fbeffed00  type: "pglist node_id"
> 
> In cyclic mode, makedumpfile first calculates only info->num_dumpable [1] and
> frees the used bitmap, and later creates 2nd bitmap again [2] at this time.
> 
>   create_dumpfile
>     create_dump_bitmap
>       info->num_dumpable = get_num_dumpable_cyclic()  <<-- [1]
>     writeout_dumpfile
>       write_kdump_pages_and_bitmap_cyclic
>         foreach cycle
>           create_2nd_bitmap  <<-- [2]
>           write_kdump_pages_cyclic
> 
> So with live system, num_dumped can exceed info->num_dumpable.
> If it stops at info->num_dumpable, some necessary data can be missed.

thanks for the explanation! I haven't considered that case and assumed
info->num_dumpable is constant.

> Capturing /proc/kcore is still fragile and not considered enough, but
> sometimes useful... when I want to capture a snapshot of memory.
> 
> (the bitmap is allocated as block, so probably it's working as some buffer?)
> 
> So I will merge the 1/2 patch, but personally would not like to merge
> this patch.  How necessary is this?

I don't think this patch is absolutely necessary. It's only a small
performance improvement but shouldn't have a huge impact. If you like
you can drop the patch.

Thanks
Philipp

[PATCH 2/2] makedumpfile: break loop after last dumpable page

[HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?=]

-----Original Message-----
> Hi Kazu,
> 
> On Thu, 7 Apr 2022 06:43:00 +0000
> HAGIO KAZUHITO(?????) <k-hagio-ab@nec.com> wrote:
> 
> > -----Original Message-----
> > > Once the last dumpable page was processed there is no need to finish the
> > > loop to the last page. Thus exit early to improve performance.
> > >
> > > Signed-off-by: Philipp Rudo <prudo@redhat.com>
> > > ---
> > >  makedumpfile.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/makedumpfile.c b/makedumpfile.c
> > > index 2ef3458..c944d0e 100644
> > > --- a/makedumpfile.c
> > > +++ b/makedumpfile.c
> > > @@ -8884,6 +8884,12 @@ write_kdump_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_pag
> > >
> > >  	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
> > >
> > > +		/*
> > > +		 * All dumpable pages have been processed. No need to continue.
> > > +		 */
> > > +		if (num_dumped == info->num_dumpable)
> > > +			break;
> >
> > This patch is likely to increase the possibility of failure to capture
> > /proc/kcore, although this is an unofficial functionality...
> >
> >   # makedumpfile -ld31 /proc/kcore kcore.snap
> >   # crash vmlinux kcore.snap
> >   ...
> >   crash: page incomplete: kernel virtual address: ffff916fbeffed00  type: "pglist node_id"
> >
> > In cyclic mode, makedumpfile first calculates only info->num_dumpable [1] and
> > frees the used bitmap, and later creates 2nd bitmap again [2] at this time.
> >
> >   create_dumpfile
> >     create_dump_bitmap
> >       info->num_dumpable = get_num_dumpable_cyclic()  <<-- [1]
> >     writeout_dumpfile
> >       write_kdump_pages_and_bitmap_cyclic
> >         foreach cycle
> >           create_2nd_bitmap  <<-- [2]
> >           write_kdump_pages_cyclic
> >
> > So with live system, num_dumped can exceed info->num_dumpable.
> > If it stops at info->num_dumpable, some necessary data can be missed.
> 
> thanks for the explanation! I haven't considered that case and assumed
> info->num_dumpable is constant.
> 
> > Capturing /proc/kcore is still fragile and not considered enough, but
> > sometimes useful... when I want to capture a snapshot of memory.
> >
> > (the bitmap is allocated as block, so probably it's working as some buffer?)
> >
> > So I will merge the 1/2 patch, but personally would not like to merge
> > this patch.  How necessary is this?
> 
> I don't think this patch is absolutely necessary. It's only a small
> performance improvement but shouldn't have a huge impact. If you like
> you can drop the patch.

ok, drop it at present.

Applied the 1/2 patch.
https://github.com/makedumpfile/makedumpfile/commit/3318c51c3c60ed192519cef6f6e72178151f88a8

Thanks!
Kazu