[PATCH v3 0/2] Generic multi-page exclusion

[PATCH v3 0/2] Generic multi-page exclusion

[Petr Tesarik]

Kumagai-san,

this patch was inspired by this post of yours:

http://lists.infradead.org/pipermail/kexec/2013-November/010445.html

Unfortunately, this rewrite never happened, so I took the liberty of
overtaking the job. I hope you don't mind.

This is a preparatory series to add hugepage support without having
to care about the appropriate size of the cyclic buffer.

Changelog:
* v2:
  - Keep excluded regions per mem_map_data
  - Process excluded pages in chunks

* v3:
  - Keep excluded region extents in struct cycle

Petr Tesarik (2):
  Generic handling of multi-page exclusions
  Get rid of overrun adjustments

 makedumpfile.c | 108 +++++++++++++++++++--------------------------------------
 makedumpfile.h |   5 +++
 2 files changed, 40 insertions(+), 73 deletions(-)

-- 
1.8.4.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v3 1/2] Generic handling of multi-page exclusions

[Petr Tesarik]

When multiple pages are excluded from the dump, store the extents in
struct cycle and check if anything is still pending on the next invocation
of __exclude_unnecessary_pages. This assumes that:

  1. after __exclude_unnecessary_pages is called for a struct mem_map_data
     that extends beyond the current cycle, it is not called again during
     that cycle,
  2. in the next cycle, __exclude_unnecessary_pages is not called before
     this final struct mem_map_data.

Both assumptions are met if struct mem_map_data segments:

  1. do not overlap,
  2. are sorted by physical address in ascending order.

These two conditions are true for all supported memory models.

Note that the start PFN of the excluded extent is set to the end of the
current cycle (which is equal to the start of the next cycle, see
update_cycle), so only the part of the excluded region which falls beyond
current cycle buffer is valid. If the excluded region is completely
processed in the current cycle, the start PFN is bigger than the end PFN
and no work is done at the beginning of the next cycle.

After processing the leftover from last cycle, pfn_start and mem_map are
adjusted to skip the excluded pages. There is no check whether the
adjusted pfn_start is within the current cycle. Nothing bad happens if
it isn't, because pages outside the current cyclic region are ignored by
the subsequent loop, and the remainder is postponed to the next cycle by
exclude_range().

Signed-off-by: Petr Tesarik <ptesarik@suse.cz>
---
 makedumpfile.c | 49 +++++++++++++++++++++++++++++++++++--------------
 makedumpfile.h |  5 +++++
 2 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 16081a5..a3498e4 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -4667,6 +4667,26 @@ initialize_2nd_bitmap_cyclic(struct cycle *cycle)
 	return TRUE;
 }
 
+static void
+exclude_range(mdf_pfn_t *counter, mdf_pfn_t pfn, mdf_pfn_t endpfn,
+    struct cycle *cycle)
+{
+	if (cycle) {
+		cycle->exclude_pfn_start = cycle->end_pfn;
+		cycle->exclude_pfn_end = endpfn;
+		cycle->exclude_pfn_counter = counter;
+
+		if (cycle->end_pfn < endpfn)
+			endpfn = cycle->end_pfn;
+	}
+
+	while (pfn < endpfn) {
+		if (clear_bit_on_2nd_bitmap_for_kernel(pfn, cycle))
+			(*counter)++;
+		++pfn;
+	}
+}
+
 int
 __exclude_unnecessary_pages(unsigned long mem_map,
     mdf_pfn_t pfn_start, mdf_pfn_t pfn_end, struct cycle *cycle)
@@ -4681,6 +4701,18 @@ __exclude_unnecessary_pages(unsigned long mem_map,
 	unsigned long flags, mapping, private = 0;
 
 	/*
+	 * If a multi-page exclusion is pending, do it first
+	 */
+	if (cycle && cycle->exclude_pfn_start < cycle->exclude_pfn_end) {
+		exclude_range(cycle->exclude_pfn_counter,
+			cycle->exclude_pfn_start, cycle->exclude_pfn_end,
+			cycle);
+
+		mem_map += (cycle->exclude_pfn_end - pfn_start) * SIZE(page);
+		pfn_start = cycle->exclude_pfn_end;
+	}
+
+	/*
 	 * Refresh the buffer of struct page, when changing mem_map.
 	 */
 	pfn_read_start = ULONGLONG_MAX;
@@ -4744,21 +4776,10 @@ __exclude_unnecessary_pages(unsigned long mem_map,
 		if ((info->dump_level & DL_EXCLUDE_FREE)
 		    && info->page_is_buddy
 		    && info->page_is_buddy(flags, _mapcount, private, _count)) {
-			int i, nr_pages = 1 << private;
+			int nr_pages = 1 << private;
+
+			exclude_range(&pfn_free, pfn, pfn + nr_pages, cycle);
 
-			for (i = 0; i < nr_pages; ++i) {
-				/*
-				 * According to combination of
-				 * MAX_ORDER and size of cyclic
-				 * buffer, this clearing bit operation
-				 * can overrun the cyclic buffer.
-				 *
-				 * See check_cyclic_buffer_overrun()
-				 * for the detail.
-				 */
-				if (clear_bit_on_2nd_bitmap_for_kernel((pfn + i), cycle))
-					pfn_free++;
-			}
 			pfn += nr_pages - 1;
 			mem_map += (nr_pages - 1) * SIZE(page);
 		}
diff --git a/makedumpfile.h b/makedumpfile.h
index eb03688..43cf91d 100644
--- a/makedumpfile.h
+++ b/makedumpfile.h
@@ -1593,6 +1593,11 @@ int get_xen_info_ia64(void);
 struct cycle {
 	mdf_pfn_t start_pfn;
 	mdf_pfn_t end_pfn;
+
+	/* for excluding multi-page regions */
+	mdf_pfn_t exclude_pfn_start;
+	mdf_pfn_t exclude_pfn_end;
+	mdf_pfn_t *exclude_pfn_counter;
 };
 
 static inline int
-- 
1.8.4.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v3 2/2] Get rid of overrun adjustments

[Petr Tesarik]

Thanks to the previous commit, __exclude_unnecessary_pages does not
require any specific size of the cycle.

Signed-off-by: Petr Tesarik <ptesarik@suse.cz>
---
 makedumpfile.c | 59 ----------------------------------------------------------
 1 file changed, 59 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index a3498e4..9a36856 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -88,7 +88,6 @@ do { \
 		*ptr_long_table = value; \
 } while (0)
 
-static void check_cyclic_buffer_overrun(void);
 static void setup_page_is_buddy(void);
 
 void
@@ -3254,9 +3253,6 @@ out:
 		    !sadump_generate_elf_note_from_dumpfile())
 			return FALSE;
 
-		if (info->flag_cyclic && info->dump_level & DL_EXCLUDE_FREE)
-			check_cyclic_buffer_overrun();
-
 	} else {
 		if (!get_mem_map_without_mm())
 			return FALSE;
@@ -4280,61 +4276,6 @@ exclude_free_page(struct cycle *cycle)
 }
 
 /*
- * Let C be a cyclic buffer size and B a bitmap size used for
- * representing maximum block size managed by buddy allocator.
- *
- * For some combinations of C and B, clearing operation can overrun
- * the cyclic buffer. Let's consider three cases.
- *
- *   - If C == B, this is trivially safe.
- *
- *   - If B > C, overrun can easily happen.
- *
- *   - In case of C > B, if C mod B != 0, then there exist n > m > 0,
- *     B > b > 0 such that n x C = m x B + b. This means that clearing
- *     operation overruns cyclic buffer (B - b)-bytes in the
- *     combination of n-th cycle and m-th block.
- *
- *     Note that C mod B != 0 iff (m x C) mod B != 0 for some m.
- *
- * If C == B, C mod B == 0 always holds. Again, if B > C, C mod B != 0
- * always holds. Hence, it's always sufficient to check the condition
- * C mod B != 0 in order to determine whether overrun can happen or
- * not.
- *
- * The bitmap size used for maximum block size B is calculated from
- * MAX_ORDER as:
- *
- *   B := DIVIDE_UP((1 << (MAX_ORDER - 1)), BITS_PER_BYTE)
- *
- * Normally, MAX_ORDER is 11 at default. This is configurable through
- * CONFIG_FORCE_MAX_ZONEORDER.
- */
-static void
-check_cyclic_buffer_overrun(void)
-{
-	int max_order = ARRAY_LENGTH(zone.free_area);
-	int max_order_nr_pages = 1 << (max_order - 1);
-	unsigned long max_block_size = divideup(max_order_nr_pages, BITPERBYTE);
-
-	if (info->bufsize_cyclic % max_block_size) {
-		unsigned long bufsize;
-
-		if (max_block_size > info->bufsize_cyclic) {
-			MSG("WARNING: some free pages are not filtered.\n");
-			return;
-		}
-
-		bufsize = info->bufsize_cyclic;
-		info->bufsize_cyclic = round(bufsize, max_block_size);
-		info->pfn_cyclic = info->bufsize_cyclic * BITPERBYTE;
-
-		MSG("cyclic buffer size has been changed: %lu => %lu\n",
-		    bufsize, info->bufsize_cyclic);
-	}
-}
-
-/*
  * For the kernel versions from v2.6.17 to v2.6.37.
  */
 static int
-- 
1.8.4.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3 0/2] Generic multi-page exclusion

[Petr Tesarik]

On Wed,  7 May 2014 19:06:41 +0200
Petr Tesarik <ptesarik@suse.cz> wrote:

> Kumagai-san,
> 
> this patch was inspired by this post of yours:
> 
> http://lists.infradead.org/pipermail/kexec/2013-November/010445.html
> 
> Unfortunately, this rewrite never happened, so I took the liberty of
> overtaking the job. I hope you don't mind.
> 
> This is a preparatory series to add hugepage support without having
> to care about the appropriate size of the cyclic buffer.

Kumagai-san,

what is the status of this patch set now? Forgotten, or are you looking
at it?

Regards,
Petr T

> Changelog:
> * v2:
>   - Keep excluded regions per mem_map_data
>   - Process excluded pages in chunks
> 
> * v3:
>   - Keep excluded region extents in struct cycle
> 
> Petr Tesarik (2):
>   Generic handling of multi-page exclusions
>   Get rid of overrun adjustments
> 
>  makedumpfile.c | 108 +++++++++++++++++++--------------------------------------
>  makedumpfile.h |   5 +++
>  2 files changed, 40 insertions(+), 73 deletions(-)
> 


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH v3 1/2] Generic handling of multi-page exclusions

[Atsushi Kumagai]

Hello Petr,

>When multiple pages are excluded from the dump, store the extents in
>struct cycle and check if anything is still pending on the next invocation
>of __exclude_unnecessary_pages. This assumes that:
>
>  1. after __exclude_unnecessary_pages is called for a struct mem_map_data
>     that extends beyond the current cycle, it is not called again during
>     that cycle,
>  2. in the next cycle, __exclude_unnecessary_pages is not called before
>     this final struct mem_map_data.
>
>Both assumptions are met if struct mem_map_data segments:
>
>  1. do not overlap,
>  2. are sorted by physical address in ascending order.

In ELF case, write_elf_pages_cyclic() processes PT_LOAD entries from
PT_LOAD(0), this can break both assumptions unluckily.
Actually this patch doesn't work on my machine:

LOAD (0)
  phys_start : 1000000
  phys_end   : 182f000
  virt_start : ffffffff81000000
  virt_end   : ffffffff8182f000
LOAD (1)
  phys_start : 1000
  phys_end   : 9b400
  virt_start : ffff810000001000
  virt_end   : ffff81000009b400
LOAD (2)
  phys_start : 100000
  phys_end   : 27000000
  virt_start : ffff810000100000
  virt_end   : ffff810027000000
LOAD (3)
  phys_start : 37000000
  phys_end   : cff70000
  virt_start : ffff810037000000
  virt_end   : ffff8100cff70000
LOAD (4)
  phys_start : 100000000
  phys_end   : 170000000
  virt_start : ffff810100000000
  virt_end   : ffff810170000000


PT_LOAD(2) includes PT_LOAD(0) and there physical addresses aren't sorted.

If there is the only "sort issue", it may easy to fix it with a new iterator
like "for_each_pt_load()", it iterates PT_LOAD entries in ascending order
by physical address.
However, I don't have a good idea to solve the overlap issue now...


Thanks
Atsushi Kumagai

>These two conditions are true for all supported memory models.
>
>Note that the start PFN of the excluded extent is set to the end of the
>current cycle (which is equal to the start of the next cycle, see
>update_cycle), so only the part of the excluded region which falls beyond
>current cycle buffer is valid. If the excluded region is completely
>processed in the current cycle, the start PFN is bigger than the end PFN
>and no work is done at the beginning of the next cycle.
>
>After processing the leftover from last cycle, pfn_start and mem_map are
>adjusted to skip the excluded pages. There is no check whether the
>adjusted pfn_start is within the current cycle. Nothing bad happens if
>it isn't, because pages outside the current cyclic region are ignored by
>the subsequent loop, and the remainder is postponed to the next cycle by
>exclude_range().
>
>Signed-off-by: Petr Tesarik <ptesarik@suse.cz>
>---
> makedumpfile.c | 49 +++++++++++++++++++++++++++++++++++--------------
> makedumpfile.h |  5 +++++
> 2 files changed, 40 insertions(+), 14 deletions(-)
>
>diff --git a/makedumpfile.c b/makedumpfile.c
>index 16081a5..a3498e4 100644
>--- a/makedumpfile.c
>+++ b/makedumpfile.c
>@@ -4667,6 +4667,26 @@ initialize_2nd_bitmap_cyclic(struct cycle *cycle)
> 	return TRUE;
> }
>
>+static void
>+exclude_range(mdf_pfn_t *counter, mdf_pfn_t pfn, mdf_pfn_t endpfn,
>+    struct cycle *cycle)
>+{
>+	if (cycle) {
>+		cycle->exclude_pfn_start = cycle->end_pfn;
>+		cycle->exclude_pfn_end = endpfn;
>+		cycle->exclude_pfn_counter = counter;
>+
>+		if (cycle->end_pfn < endpfn)
>+			endpfn = cycle->end_pfn;
>+	}
>+
>+	while (pfn < endpfn) {
>+		if (clear_bit_on_2nd_bitmap_for_kernel(pfn, cycle))
>+			(*counter)++;
>+		++pfn;
>+	}
>+}
>+
> int
> __exclude_unnecessary_pages(unsigned long mem_map,
>     mdf_pfn_t pfn_start, mdf_pfn_t pfn_end, struct cycle *cycle)
>@@ -4681,6 +4701,18 @@ __exclude_unnecessary_pages(unsigned long mem_map,
> 	unsigned long flags, mapping, private = 0;
>
> 	/*
>+	 * If a multi-page exclusion is pending, do it first
>+	 */
>+	if (cycle && cycle->exclude_pfn_start < cycle->exclude_pfn_end) {
>+		exclude_range(cycle->exclude_pfn_counter,
>+			cycle->exclude_pfn_start, cycle->exclude_pfn_end,
>+			cycle);
>+
>+		mem_map += (cycle->exclude_pfn_end - pfn_start) * SIZE(page);
>+		pfn_start = cycle->exclude_pfn_end;
>+	}
>+
>+	/*
> 	 * Refresh the buffer of struct page, when changing mem_map.
> 	 */
> 	pfn_read_start = ULONGLONG_MAX;
>@@ -4744,21 +4776,10 @@ __exclude_unnecessary_pages(unsigned long mem_map,
> 		if ((info->dump_level & DL_EXCLUDE_FREE)
> 		    && info->page_is_buddy
> 		    && info->page_is_buddy(flags, _mapcount, private, _count)) {
>-			int i, nr_pages = 1 << private;
>+			int nr_pages = 1 << private;
>+
>+			exclude_range(&pfn_free, pfn, pfn + nr_pages, cycle);
>
>-			for (i = 0; i < nr_pages; ++i) {
>-				/*
>-				 * According to combination of
>-				 * MAX_ORDER and size of cyclic
>-				 * buffer, this clearing bit operation
>-				 * can overrun the cyclic buffer.
>-				 *
>-				 * See check_cyclic_buffer_overrun()
>-				 * for the detail.
>-				 */
>-				if (clear_bit_on_2nd_bitmap_for_kernel((pfn + i), cycle))
>-					pfn_free++;
>-			}
> 			pfn += nr_pages - 1;
> 			mem_map += (nr_pages - 1) * SIZE(page);
> 		}
>diff --git a/makedumpfile.h b/makedumpfile.h
>index eb03688..43cf91d 100644
>--- a/makedumpfile.h
>+++ b/makedumpfile.h
>@@ -1593,6 +1593,11 @@ int get_xen_info_ia64(void);
> struct cycle {
> 	mdf_pfn_t start_pfn;
> 	mdf_pfn_t end_pfn;
>+
>+	/* for excluding multi-page regions */
>+	mdf_pfn_t exclude_pfn_start;
>+	mdf_pfn_t exclude_pfn_end;
>+	mdf_pfn_t *exclude_pfn_counter;
> };
>
> static inline int
>--
>1.8.4.5

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3 1/2] Generic handling of multi-page exclusions

[Petr Tesarik]

On Wed, 14 May 2014 07:54:17 +0000
Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp> wrote:

> Hello Petr,
> 
> >When multiple pages are excluded from the dump, store the extents in
> >struct cycle and check if anything is still pending on the next invocation
> >of __exclude_unnecessary_pages. This assumes that:
> >
> >  1. after __exclude_unnecessary_pages is called for a struct mem_map_data
> >     that extends beyond the current cycle, it is not called again during
> >     that cycle,
> >  2. in the next cycle, __exclude_unnecessary_pages is not called before
> >     this final struct mem_map_data.
> >
> >Both assumptions are met if struct mem_map_data segments:
> >
> >  1. do not overlap,
> >  2. are sorted by physical address in ascending order.
> 
> In ELF case, write_elf_pages_cyclic() processes PT_LOAD entries from
> PT_LOAD(0), this can break both assumptions unluckily.
> Actually this patch doesn't work on my machine:
> 
> LOAD (0)
>   phys_start : 1000000
>   phys_end   : 182f000
>   virt_start : ffffffff81000000
>   virt_end   : ffffffff8182f000
> LOAD (1)
>   phys_start : 1000
>   phys_end   : 9b400
>   virt_start : ffff810000001000
>   virt_end   : ffff81000009b400
> LOAD (2)
>   phys_start : 100000
>   phys_end   : 27000000
>   virt_start : ffff810000100000
>   virt_end   : ffff810027000000
> LOAD (3)
>   phys_start : 37000000
>   phys_end   : cff70000
>   virt_start : ffff810037000000
>   virt_end   : ffff8100cff70000
> LOAD (4)
>   phys_start : 100000000
>   phys_end   : 170000000
>   virt_start : ffff810100000000
>   virt_end   : ffff810170000000
> 
> 
> PT_LOAD(2) includes PT_LOAD(0) and there physical addresses aren't sorted.
> 
> If there is the only "sort issue", it may easy to fix it with a new iterator
> like "for_each_pt_load()", it iterates PT_LOAD entries in ascending order
> by physical address.
> However, I don't have a good idea to solve the overlap issue now...

I have. I can go back to my previous version and add those fields to
struct mem_map_data. I only changed it because of this feedback from
HATAYAMA Daisuke:

http://lists.infradead.org/pipermail/kexec/2014-April/011477.html

If I add the fields to struct mem_map_data, the code does not depend on
any specific call order.

OK, time for version 4.

Petr T

> Thanks
> Atsushi Kumagai
> 
> >These two conditions are true for all supported memory models.
> >
> >Note that the start PFN of the excluded extent is set to the end of the
> >current cycle (which is equal to the start of the next cycle, see
> >update_cycle), so only the part of the excluded region which falls beyond
> >current cycle buffer is valid. If the excluded region is completely
> >processed in the current cycle, the start PFN is bigger than the end PFN
> >and no work is done at the beginning of the next cycle.
> >
> >After processing the leftover from last cycle, pfn_start and mem_map are
> >adjusted to skip the excluded pages. There is no check whether the
> >adjusted pfn_start is within the current cycle. Nothing bad happens if
> >it isn't, because pages outside the current cyclic region are ignored by
> >the subsequent loop, and the remainder is postponed to the next cycle by
> >exclude_range().
> >
> >Signed-off-by: Petr Tesarik <ptesarik@suse.cz>
> >---
> > makedumpfile.c | 49 +++++++++++++++++++++++++++++++++++--------------
> > makedumpfile.h |  5 +++++
> > 2 files changed, 40 insertions(+), 14 deletions(-)
> >
> >diff --git a/makedumpfile.c b/makedumpfile.c
> >index 16081a5..a3498e4 100644
> >--- a/makedumpfile.c
> >+++ b/makedumpfile.c
> >@@ -4667,6 +4667,26 @@ initialize_2nd_bitmap_cyclic(struct cycle *cycle)
> > 	return TRUE;
> > }
> >
> >+static void
> >+exclude_range(mdf_pfn_t *counter, mdf_pfn_t pfn, mdf_pfn_t endpfn,
> >+    struct cycle *cycle)
> >+{
> >+	if (cycle) {
> >+		cycle->exclude_pfn_start = cycle->end_pfn;
> >+		cycle->exclude_pfn_end = endpfn;
> >+		cycle->exclude_pfn_counter = counter;
> >+
> >+		if (cycle->end_pfn < endpfn)
> >+			endpfn = cycle->end_pfn;
> >+	}
> >+
> >+	while (pfn < endpfn) {
> >+		if (clear_bit_on_2nd_bitmap_for_kernel(pfn, cycle))
> >+			(*counter)++;
> >+		++pfn;
> >+	}
> >+}
> >+
> > int
> > __exclude_unnecessary_pages(unsigned long mem_map,
> >     mdf_pfn_t pfn_start, mdf_pfn_t pfn_end, struct cycle *cycle)
> >@@ -4681,6 +4701,18 @@ __exclude_unnecessary_pages(unsigned long mem_map,
> > 	unsigned long flags, mapping, private = 0;
> >
> > 	/*
> >+	 * If a multi-page exclusion is pending, do it first
> >+	 */
> >+	if (cycle && cycle->exclude_pfn_start < cycle->exclude_pfn_end) {
> >+		exclude_range(cycle->exclude_pfn_counter,
> >+			cycle->exclude_pfn_start, cycle->exclude_pfn_end,
> >+			cycle);
> >+
> >+		mem_map += (cycle->exclude_pfn_end - pfn_start) * SIZE(page);
> >+		pfn_start = cycle->exclude_pfn_end;
> >+	}
> >+
> >+	/*
> > 	 * Refresh the buffer of struct page, when changing mem_map.
> > 	 */
> > 	pfn_read_start = ULONGLONG_MAX;
> >@@ -4744,21 +4776,10 @@ __exclude_unnecessary_pages(unsigned long mem_map,
> > 		if ((info->dump_level & DL_EXCLUDE_FREE)
> > 		    && info->page_is_buddy
> > 		    && info->page_is_buddy(flags, _mapcount, private, _count)) {
> >-			int i, nr_pages = 1 << private;
> >+			int nr_pages = 1 << private;
> >+
> >+			exclude_range(&pfn_free, pfn, pfn + nr_pages, cycle);
> >
> >-			for (i = 0; i < nr_pages; ++i) {
> >-				/*
> >-				 * According to combination of
> >-				 * MAX_ORDER and size of cyclic
> >-				 * buffer, this clearing bit operation
> >-				 * can overrun the cyclic buffer.
> >-				 *
> >-				 * See check_cyclic_buffer_overrun()
> >-				 * for the detail.
> >-				 */
> >-				if (clear_bit_on_2nd_bitmap_for_kernel((pfn + i), cycle))
> >-					pfn_free++;
> >-			}
> > 			pfn += nr_pages - 1;
> > 			mem_map += (nr_pages - 1) * SIZE(page);
> > 		}
> >diff --git a/makedumpfile.h b/makedumpfile.h
> >index eb03688..43cf91d 100644
> >--- a/makedumpfile.h
> >+++ b/makedumpfile.h
> >@@ -1593,6 +1593,11 @@ int get_xen_info_ia64(void);
> > struct cycle {
> > 	mdf_pfn_t start_pfn;
> > 	mdf_pfn_t end_pfn;
> >+
> >+	/* for excluding multi-page regions */
> >+	mdf_pfn_t exclude_pfn_start;
> >+	mdf_pfn_t exclude_pfn_end;
> >+	mdf_pfn_t *exclude_pfn_counter;
> > };
> >
> > static inline int
> >--
> >1.8.4.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3 1/2] Generic handling of multi-page exclusions

[HATAYAMA Daisuke]

From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
Subject: RE: [PATCH v3 1/2] Generic handling of multi-page exclusions
Date: Wed, 14 May 2014 07:54:17 +0000

> Hello Petr,
> 
>>When multiple pages are excluded from the dump, store the extents in
>>struct cycle and check if anything is still pending on the next invocation
>>of __exclude_unnecessary_pages. This assumes that:
>>
>>  1. after __exclude_unnecessary_pages is called for a struct mem_map_data
>>     that extends beyond the current cycle, it is not called again during
>>     that cycle,
>>  2. in the next cycle, __exclude_unnecessary_pages is not called before
>>     this final struct mem_map_data.
>>
>>Both assumptions are met if struct mem_map_data segments:
>>
>>  1. do not overlap,
>>  2. are sorted by physical address in ascending order.
> 
> In ELF case, write_elf_pages_cyclic() processes PT_LOAD entries from
> PT_LOAD(0), this can break both assumptions unluckily.
> Actually this patch doesn't work on my machine:
> 
> LOAD (0)
>   phys_start : 1000000
>   phys_end   : 182f000
>   virt_start : ffffffff81000000
>   virt_end   : ffffffff8182f000
> LOAD (1)
>   phys_start : 1000
>   phys_end   : 9b400
>   virt_start : ffff810000001000
>   virt_end   : ffff81000009b400
> LOAD (2)
>   phys_start : 100000
>   phys_end   : 27000000
>   virt_start : ffff810000100000
>   virt_end   : ffff810027000000
> LOAD (3)
>   phys_start : 37000000
>   phys_end   : cff70000
>   virt_start : ffff810037000000
>   virt_end   : ffff8100cff70000
> LOAD (4)
>   phys_start : 100000000
>   phys_end   : 170000000
>   virt_start : ffff810100000000
>   virt_end   : ffff810170000000
> 
> 
> PT_LOAD(2) includes PT_LOAD(0) and there physical addresses aren't sorted.
> 
> If there is the only "sort issue", it may easy to fix it with a new iterator
> like "for_each_pt_load()", it iterates PT_LOAD entries in ascending order
> by physical address.
> However, I don't have a good idea to solve the overlap issue now...
> 

Is it enough to merge them? Prepare a modified version of PTLOAD list
and refer to it in actual processing. I think this also leads to
cleaning up readpage_elf() that addresses some overapping memory map
issue on ia64.

--
Thanks.
HATAYAMA, Daisuke


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3 1/2] Generic handling of multi-page exclusions

[HATAYAMA Daisuke]

From: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
Subject: Re: [PATCH v3 1/2] Generic handling of multi-page exclusions
Date: Wed, 14 May 2014 19:37:23 +0900

> From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
> Subject: RE: [PATCH v3 1/2] Generic handling of multi-page exclusions
> Date: Wed, 14 May 2014 07:54:17 +0000
> 
>> Hello Petr,
>> 
>>>When multiple pages are excluded from the dump, store the extents in
>>>struct cycle and check if anything is still pending on the next invocation
>>>of __exclude_unnecessary_pages. This assumes that:
>>>
>>>  1. after __exclude_unnecessary_pages is called for a struct mem_map_data
>>>     that extends beyond the current cycle, it is not called again during
>>>     that cycle,
>>>  2. in the next cycle, __exclude_unnecessary_pages is not called before
>>>     this final struct mem_map_data.
>>>
>>>Both assumptions are met if struct mem_map_data segments:
>>>
>>>  1. do not overlap,
>>>  2. are sorted by physical address in ascending order.
>> 
>> In ELF case, write_elf_pages_cyclic() processes PT_LOAD entries from
>> PT_LOAD(0), this can break both assumptions unluckily.
>> Actually this patch doesn't work on my machine:
>> 
>> LOAD (0)
>>   phys_start : 1000000
>>   phys_end   : 182f000
>>   virt_start : ffffffff81000000
>>   virt_end   : ffffffff8182f000
>> LOAD (1)
>>   phys_start : 1000
>>   phys_end   : 9b400
>>   virt_start : ffff810000001000
>>   virt_end   : ffff81000009b400
>> LOAD (2)
>>   phys_start : 100000
>>   phys_end   : 27000000
>>   virt_start : ffff810000100000
>>   virt_end   : ffff810027000000
>> LOAD (3)
>>   phys_start : 37000000
>>   phys_end   : cff70000
>>   virt_start : ffff810037000000
>>   virt_end   : ffff8100cff70000
>> LOAD (4)
>>   phys_start : 100000000
>>   phys_end   : 170000000
>>   virt_start : ffff810100000000
>>   virt_end   : ffff810170000000
>> 
>> 
>> PT_LOAD(2) includes PT_LOAD(0) and there physical addresses aren't sorted.
>> 
>> If there is the only "sort issue", it may easy to fix it with a new iterator
>> like "for_each_pt_load()", it iterates PT_LOAD entries in ascending order
>> by physical address.
>> However, I don't have a good idea to solve the overlap issue now...
>> 
> 
> Is it enough to merge them? Prepare a modified version of PTLOAD list
> and refer to it in actual processing. I think this also leads to
> cleaning up readpage_elf() that addresses some overapping memory map
> issue on ia64.
> 

I'm saying this because I don't find anywhere virt_start or virt_end
is used. We look up page table to convert virtual address to physical
address, not PT_LOAD entries.

--
Thanks.
HATAYAMA, Daisuke


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3 1/2] Generic handling of multi-page exclusions

[Petr Tesarik]

On Wed, 14 May 2014 19:54:28 +0900 (JST)
HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:

> From: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
> Subject: Re: [PATCH v3 1/2] Generic handling of multi-page exclusions
> Date: Wed, 14 May 2014 19:37:23 +0900
> 
> > From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
> > Subject: RE: [PATCH v3 1/2] Generic handling of multi-page exclusions
> > Date: Wed, 14 May 2014 07:54:17 +0000
> > 
> >> Hello Petr,
> >> 
> >>>When multiple pages are excluded from the dump, store the extents in
> >>>struct cycle and check if anything is still pending on the next invocation
> >>>of __exclude_unnecessary_pages. This assumes that:
> >>>
> >>>  1. after __exclude_unnecessary_pages is called for a struct mem_map_data
> >>>     that extends beyond the current cycle, it is not called again during
> >>>     that cycle,
> >>>  2. in the next cycle, __exclude_unnecessary_pages is not called before
> >>>     this final struct mem_map_data.
> >>>
> >>>Both assumptions are met if struct mem_map_data segments:
> >>>
> >>>  1. do not overlap,
> >>>  2. are sorted by physical address in ascending order.
> >> 
> >> In ELF case, write_elf_pages_cyclic() processes PT_LOAD entries from
> >> PT_LOAD(0), this can break both assumptions unluckily.
> >> Actually this patch doesn't work on my machine:
> >> 
> >> LOAD (0)
> >>   phys_start : 1000000
> >>   phys_end   : 182f000
> >>   virt_start : ffffffff81000000
> >>   virt_end   : ffffffff8182f000
> >> LOAD (1)
> >>   phys_start : 1000
> >>   phys_end   : 9b400
> >>   virt_start : ffff810000001000
> >>   virt_end   : ffff81000009b400
> >> LOAD (2)
> >>   phys_start : 100000
> >>   phys_end   : 27000000
> >>   virt_start : ffff810000100000
> >>   virt_end   : ffff810027000000
> >> LOAD (3)
> >>   phys_start : 37000000
> >>   phys_end   : cff70000
> >>   virt_start : ffff810037000000
> >>   virt_end   : ffff8100cff70000
> >> LOAD (4)
> >>   phys_start : 100000000
> >>   phys_end   : 170000000
> >>   virt_start : ffff810100000000
> >>   virt_end   : ffff810170000000
> >> 
> >> 
> >> PT_LOAD(2) includes PT_LOAD(0) and there physical addresses aren't sorted.
> >> 
> >> If there is the only "sort issue", it may easy to fix it with a new iterator
> >> like "for_each_pt_load()", it iterates PT_LOAD entries in ascending order
> >> by physical address.
> >> However, I don't have a good idea to solve the overlap issue now...
> >> 
> > 
> > Is it enough to merge them? Prepare a modified version of PTLOAD list
> > and refer to it in actual processing. I think this also leads to
> > cleaning up readpage_elf() that addresses some overapping memory map
> > issue on ia64.
> > 
> 
> I'm saying this because I don't find anywhere virt_start or virt_end
> is used. We look up page table to convert virtual address to physical
> address, not PT_LOAD entries.

Oh, you're right! Why does the ordering of PT_LOAD segments matter here?
If makedumpfile fails on your machine after applying my patches, then
it's quite likely because of something else.

FWIW I verified on a few dumpfiles that makedumpfile produced exactly
the same output before and after applying the patches.

OTOH I can see a warning when writing an ELF file. Before the patch:

Excluding unnecessary pages        : [100.0 %] \WARNING: PFN out of cycle range. (pfn:c00, cycle:[3fc00-3ffd0])

After the patch:

Excluding unnecessary pages        : [100.0 %] \WARNING: PFN out of cycle range. (pfn:26c00, cycle:[0-1ff6])

I'm unsure why there are out-of-cycle PFNs. Researching...

Petr T

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH v3 1/2] Generic handling of multi-page exclusions

[Atsushi Kumagai]

>On Wed, 14 May 2014 19:54:28 +0900 (JST)
>HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:
>
>> From: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
>> Subject: Re: [PATCH v3 1/2] Generic handling of multi-page exclusions
>> Date: Wed, 14 May 2014 19:37:23 +0900
>>
>> > From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
>> > Subject: RE: [PATCH v3 1/2] Generic handling of multi-page exclusions
>> > Date: Wed, 14 May 2014 07:54:17 +0000
>> >
>> >> Hello Petr,
>> >>
>> >>>When multiple pages are excluded from the dump, store the extents in
>> >>>struct cycle and check if anything is still pending on the next invocation
>> >>>of __exclude_unnecessary_pages. This assumes that:
>> >>>
>> >>>  1. after __exclude_unnecessary_pages is called for a struct mem_map_data
>> >>>     that extends beyond the current cycle, it is not called again during
>> >>>     that cycle,
>> >>>  2. in the next cycle, __exclude_unnecessary_pages is not called before
>> >>>     this final struct mem_map_data.
>> >>>
>> >>>Both assumptions are met if struct mem_map_data segments:
>> >>>
>> >>>  1. do not overlap,
>> >>>  2. are sorted by physical address in ascending order.
>> >>
>> >> In ELF case, write_elf_pages_cyclic() processes PT_LOAD entries from
>> >> PT_LOAD(0), this can break both assumptions unluckily.
>> >> Actually this patch doesn't work on my machine:
>> >>
>> >> LOAD (0)
>> >>   phys_start : 1000000
>> >>   phys_end   : 182f000
>> >>   virt_start : ffffffff81000000
>> >>   virt_end   : ffffffff8182f000
>> >> LOAD (1)
>> >>   phys_start : 1000
>> >>   phys_end   : 9b400
>> >>   virt_start : ffff810000001000
>> >>   virt_end   : ffff81000009b400
>> >> LOAD (2)
>> >>   phys_start : 100000
>> >>   phys_end   : 27000000
>> >>   virt_start : ffff810000100000
>> >>   virt_end   : ffff810027000000
>> >> LOAD (3)
>> >>   phys_start : 37000000
>> >>   phys_end   : cff70000
>> >>   virt_start : ffff810037000000
>> >>   virt_end   : ffff8100cff70000
>> >> LOAD (4)
>> >>   phys_start : 100000000
>> >>   phys_end   : 170000000
>> >>   virt_start : ffff810100000000
>> >>   virt_end   : ffff810170000000
>> >>
>> >>
>> >> PT_LOAD(2) includes PT_LOAD(0) and there physical addresses aren't sorted.
>> >>
>> >> If there is the only "sort issue", it may easy to fix it with a new iterator
>> >> like "for_each_pt_load()", it iterates PT_LOAD entries in ascending order
>> >> by physical address.
>> >> However, I don't have a good idea to solve the overlap issue now...
>> >>
>> >
>> > Is it enough to merge them? Prepare a modified version of PTLOAD list
>> > and refer to it in actual processing. I think this also leads to
>> > cleaning up readpage_elf() that addresses some overapping memory map
>> > issue on ia64.
>> >
>>
>> I'm saying this because I don't find anywhere virt_start or virt_end
>> is used. We look up page table to convert virtual address to physical
>> address, not PT_LOAD entries.

I thought it's better to keep the original PT_LOAD list at first, but the
current code can split it already. So I think we shouldn't worry about
modification to PT_LOAD entries now.
If crash doesn't use virt_start and virt_end too, your idea sounds good to me.

>Oh, you're right! Why does the ordering of PT_LOAD segments matter here?
>If makedumpfile fails on your machine after applying my patches, then
>it's quite likely because of something else.

Hatayama-san must said a VtoP mapping included in a PT_LOAD will be lost
by merging PT_LOADs and it looks no problem for makedumpfile in practice.

I meant the ELF path calls for_each_cycle(PT_LOAD[i].pfn_start, PT_LOAD[i].pfn_end)
from i=0, so the PFNs aren't sorted and they are overlapping in some cases
like mine. OTOH, the kdump path calls for_each_cycle(0, info->max_mapnr) always,
so there is no problem.

I explain why the problem I met happen below, each paragraph means the flow
of for_each_cycle():

1. PT_LOAD(0): pfn [0x00001000 - 0x0000182f]
  There are free pages [0x1820-0x1840] and exclude_range(0x1820, 0x1840)
  was called. Then, exclude_pfn_start was set as 0x182f and exclude_pfn_end
  was set as 0x1840. (I'll express this like "exclude_pfn [0x182f-0x1840]")

2. PT_LOAD(1): pfn [0x00000001 - 0x0000009c]
  At the top of __exclude_unnecessary_pages(), exclude_range(0x182f, 0x1840) 
  was called since exclude_pfn was [0x182f-0x1840]. This exclude_range() didn't
  any operations for bitmaps because the PFNs are out of the cycle. However,
  exclude_pfn was set as [0x9c-0x1840] in the function.

3. PT_LOAD(2): pfn [0x00000100 - 0x00027000]
  Here, exclude_pfn was [0x9c-0x1840], so exclude_range(0x9c, 0x1840) was called.
  A part of the PFNs are on the cycle unluckily, they are excluded wrongly.
  The PFNs that should be excluded are only [0x182f-0x1840], this is the problem.

4. PT_LAOD(3): pfn [0x00037000 - 0x000cff70]
  There is no problem here.

5. PT_LOAD(4): pfn [0x00100000 - 0x00170000]
  There is no problem here too.

Like this, the unsorted issue causes wrong setting of 
exclude_pfn_(start|end) and the combination of the overlapping issue and
wrong exclude_pfn_(start|end) causes wrong bitmap operations.


Thanks
Atsushi Kumagai

>FWIW I verified on a few dumpfiles that makedumpfile produced exactly
>the same output before and after applying the patches.
>
>OTOH I can see a warning when writing an ELF file. Before the patch:
>
>Excluding unnecessary pages        : [100.0 %] \WARNING: PFN out of cycle range. (pfn:c00, cycle:[3fc00-3ffd0])
>
>After the patch:
>
>Excluding unnecessary pages        : [100.0 %] \WARNING: PFN out of cycle range. (pfn:26c00, cycle:[0-1ff6])
>
>I'm unsure why there are out-of-cycle PFNs. Researching...
>
>Petr T
>
>_______________________________________________
>kexec mailing list
>kexec@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3 1/2] Generic handling of multi-page exclusions

[Petr Tesarik]

On Fri, 16 May 2014 07:24:02 +0000
Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp> wrote:

> >On Wed, 14 May 2014 19:54:28 +0900 (JST)
> >HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:
> >
> >> From: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
> >> Subject: Re: [PATCH v3 1/2] Generic handling of multi-page exclusions
> >> Date: Wed, 14 May 2014 19:37:23 +0900
> >>
> >> > From: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
> >> > Subject: RE: [PATCH v3 1/2] Generic handling of multi-page exclusions
> >> > Date: Wed, 14 May 2014 07:54:17 +0000
> >> >
> >> >> Hello Petr,
> >> >>
> >> >>>When multiple pages are excluded from the dump, store the extents in
> >> >>>struct cycle and check if anything is still pending on the next invocation
> >> >>>of __exclude_unnecessary_pages. This assumes that:
> >> >>>
> >> >>>  1. after __exclude_unnecessary_pages is called for a struct mem_map_data
> >> >>>     that extends beyond the current cycle, it is not called again during
> >> >>>     that cycle,
> >> >>>  2. in the next cycle, __exclude_unnecessary_pages is not called before
> >> >>>     this final struct mem_map_data.
> >> >>>
> >> >>>Both assumptions are met if struct mem_map_data segments:
> >> >>>
> >> >>>  1. do not overlap,
> >> >>>  2. are sorted by physical address in ascending order.
> >> >>
> >> >> In ELF case, write_elf_pages_cyclic() processes PT_LOAD entries from
> >> >> PT_LOAD(0), this can break both assumptions unluckily.
>[...]
> 
> I thought it's better to keep the original PT_LOAD list at first, but the
> current code can split it already. So I think we shouldn't worry about
> modification to PT_LOAD entries now.
> If crash doesn't use virt_start and virt_end too, your idea sounds good to me.

I'm not sure how to continue here.

First, crash does not use virt_start and virt_end from the ELF headers.
It uses the same algorithm as makedumpfile to convert virtual addresses
to physical addresses (in fact, I believe makedumpfile initially copied
the code from crash).

Second, I understand that this change has a high risk of introducing
regressions, so I would rather keep the well-tested code path. It is,
of course, possible to fix the multi-page exclusions in ELF files.

Let me explain:

Every PT_LOAD segment behaves as an isolated run through this portion
of physical RAM. I mean, makedumpfile has never coped with page
exclusions that start in one segment and continue in another segment.
So, if we accept this limitation, then exclude_pfn_start and
exclude_pfn_end can be reset in each iteration of the PT_LOAD main
cycle. This adds only two lines to my previous patch.

Petr T

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec