[PATCH] makedumpfile: add a new function update_cyclic_region_without_exclude for partial_bitmap1 setting

[PATCH] makedumpfile: add a new function update_cyclic_region_without_exclude for partial_bitmap1 setting

[Baoquan He]

When use "--message-level 31" to do a kdump compressed dump, printing
message is not correct any more. Free pages number is doubled as below.

Original pages  : 0x000000000007539c
  Excluded pages   : 0x00000000000d986a
    Pages filled with zero  : 0x0000000000001196
    Cache pages             : 0x0000000000025008
    Cache pages + private   : 0x0000000000004baa
    User process data pages : 0x00000000000127d0
    Free pages              : 0x000000000009c352
    Hwpoison pages          : 0x0000000000000000
  Remaining pages  : 0xfffffffffff9bb32
  (The number of pages is reduced to 38418230895101%.)
Memory Hole     : 0x000000000000ac62
--------------------------------------------------
Total pages     : 0x000000000007fffe

After review code, it's caused by update_cyclic_region. This function calls
exclude_unnecessary_pages_cyclic to set partial_bitmap2, then free pages are
counted. But when write the 1st bitmap, it's also called though it is
useless, and pfn_free add free pages number. Here add a new function
update_cyclic_region_without_exclude which is used to set partial_bitmap1
specifically.
---
 makedumpfile.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 3746cf6..367e8cf 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -4745,6 +4745,26 @@ update_cyclic_region(unsigned long long pfn)
 	return TRUE;
 }
 
+
+int
+update_cyclic_region_without_exclude(unsigned long long pfn)
+{
+	if (is_cyclic_region(pfn))
+		return TRUE;
+
+	info->cyclic_start_pfn = round(pfn, info->pfn_cyclic);
+	info->cyclic_end_pfn = info->cyclic_start_pfn + info->pfn_cyclic;
+
+	if (info->cyclic_end_pfn > info->max_mapnr)
+		info->cyclic_end_pfn = info->max_mapnr;
+
+	if (info->flag_elf_dumpfile && !create_1st_bitmap_cyclic())
+		return FALSE;
+
+	return TRUE;
+}
+
+
 int
 copy_bitmap(void)
 {
@@ -6805,7 +6825,7 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
 	for (pfn = 0; pfn < info->max_mapnr; pfn++) {
 		if (is_cyclic_region(pfn))
 			continue;
-		if (!update_cyclic_region(pfn))
+		if (!update_cyclic_region_without_exclude(pfn))
 			return FALSE;
 		if (!create_1st_bitmap_cyclic())
 			return FALSE;
-- 
1.8.3.1


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

[PATCH v2] makedumpfile: add parameters to update_cyclic_region

[Baoquan He]

Function update_cyclic_region has a name which doesn't match its
functionality. it does 3 tings: update the cyclic region; if elf
dump, call create_1st_bitmap_cyclic in this region; call
exclude_unnecessary_pages_cyclic to exclude pages in this region.

However not all 3 things are needed to be done everywhere. Somewhere
unnecessary thing is done with much efforts, e.g when get dumpable
page numbers for elf cyclic dump 1st bigmap creating is not needed.
Somewhere thing is done, but it also cause errors, e.g when write
1st bitmap cyclic for compressed kdump it will add free page numbers
twice, namely pfn_free is doubled. Then dump information will be wrong
like below:

Original pages  : 0x000000000007539c
  Excluded pages   : 0x00000000000d986a
    Pages filled with zero  : 0x0000000000001196
    Cache pages             : 0x0000000000025008
    Cache pages + private   : 0x0000000000004baa
    User process data pages : 0x00000000000127d0
    Free pages              : 0x000000000009c352
    Hwpoison pages          : 0x0000000000000000
  Remaining pages  : 0xfffffffffff9bb32
  (The number of pages is reduced to 38418230895101%.)
Memory Hole     : 0x000000000000ac62
--------------------------------------------------
Total pages     : 0x000000000007fffe

So add 2 more parameters for update_cyclic_region. By these adding
update_cyclic_region can be called in a refined grain. Then effort
can be saved and above error can be fixed.

And in kdump compressed dump case, prepare_bitmap2_buffer_cyclic is
called, that means partial_bitmap2 is malloced twice. Here remove
the 2nd call.
---
 makedumpfile.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 3746cf6..8cfc2a5 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -4725,7 +4725,7 @@ exclude_unnecessary_pages_cyclic(void)
 }
 
 int
-update_cyclic_region(unsigned long long pfn)
+update_cyclic_region(unsigned long long pfn, int bmp1_set, int exclude)
 {
 	if (is_cyclic_region(pfn))
 		return TRUE;
@@ -4736,10 +4736,10 @@ update_cyclic_region(unsigned long long pfn)
 	if (info->cyclic_end_pfn > info->max_mapnr)
 		info->cyclic_end_pfn = info->max_mapnr;
 
-	if (info->flag_elf_dumpfile && !create_1st_bitmap_cyclic())
+	if (bmp1_set && info->flag_elf_dumpfile && !create_1st_bitmap_cyclic())
 		return FALSE;
 
-	if (!exclude_unnecessary_pages_cyclic())
+	if (exclude && !exclude_unnecessary_pages_cyclic())
 		return FALSE;
 
 	return TRUE;
@@ -5502,7 +5502,7 @@ get_num_dumpable_cyclic(void)
 	unsigned long long pfn, num_dumpable=0;
 
 	for (pfn = 0; pfn < info->max_mapnr; pfn++) {
-		if (!update_cyclic_region(pfn))
+		if (!update_cyclic_region(pfn, 0, 1))
 			return FALSE;
 
 		if (is_dumpable_cyclic(info->partial_bitmap2, pfn))
@@ -5803,7 +5803,7 @@ get_loads_dumpfile_cyclic(void)
 			 * Update target region and bitmap
 			 */
 			if (!is_cyclic_region(pfn)) {
-				if (!update_cyclic_region(pfn))
+				if (!update_cyclic_region(pfn, 0, 1))
 					return FALSE;
 			}
 
@@ -5872,7 +5872,7 @@ write_elf_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_page)
 
 	info->cyclic_start_pfn = 0;
 	info->cyclic_end_pfn = 0;
-	if (!update_cyclic_region(0))
+	if (!update_cyclic_region(0, 1, 1))
 		return FALSE;
 
 	if (!(phnum = get_phnum_memory()))
@@ -5910,7 +5910,7 @@ write_elf_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_page)
 			/*
 			 * Update target region and partial bitmap if necessary.
 			 */
-			if (!update_cyclic_region(pfn))
+			if (!update_cyclic_region(pfn, 1, 1))
 				return FALSE;
 
 			if (!is_dumpable_cyclic(info->partial_bitmap2, pfn)) {
@@ -6805,7 +6805,7 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
 	for (pfn = 0; pfn < info->max_mapnr; pfn++) {
 		if (is_cyclic_region(pfn))
 			continue;
-		if (!update_cyclic_region(pfn))
+		if (!update_cyclic_region(pfn, 0, 0))
 			return FALSE;
 		if (!create_1st_bitmap_cyclic())
 			return FALSE;
@@ -6815,9 +6815,6 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
 
 	free_bitmap1_buffer();
 
-	if (!prepare_bitmap2_buffer_cyclic())
-		return FALSE;
-
 	/*
 	 * Write pages and bitmap cyclically.
 	 */
@@ -6827,7 +6824,7 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
 		if (is_cyclic_region(pfn))
 			continue;
 
-		if (!update_cyclic_region(pfn))
+		if (!update_cyclic_region(pfn, 0, 1))
                         return FALSE;
 
 		if (!write_kdump_pages_cyclic(cd_header, cd_page, &pd_zero, &offset_data))
-- 
1.8.3.1


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

Re: [PATCH v2] makedumpfile: add parameters to update_cyclic_region

[Baoquan He]

Hi HATAYAMA and Atsushi,

I think v2 is better than v1, since update_cyclic_region can be used
with a more flexible calling.

What's your opinion about this?

On 11/23/13 at 05:29pm, Baoquan He wrote:
> Function update_cyclic_region has a name which doesn't match its
> functionality. it does 3 tings: update the cyclic region; if elf
> dump, call create_1st_bitmap_cyclic in this region; call
> exclude_unnecessary_pages_cyclic to exclude pages in this region.
> 
> However not all 3 things are needed to be done everywhere. Somewhere
> unnecessary thing is done with much efforts, e.g when get dumpable
> page numbers for elf cyclic dump 1st bigmap creating is not needed.
> Somewhere thing is done, but it also cause errors, e.g when write
> 1st bitmap cyclic for compressed kdump it will add free page numbers
> twice, namely pfn_free is doubled. Then dump information will be wrong
> like below:
> 
> Original pages  : 0x000000000007539c
>   Excluded pages   : 0x00000000000d986a
>     Pages filled with zero  : 0x0000000000001196
>     Cache pages             : 0x0000000000025008
>     Cache pages + private   : 0x0000000000004baa
>     User process data pages : 0x00000000000127d0
>     Free pages              : 0x000000000009c352
>     Hwpoison pages          : 0x0000000000000000
>   Remaining pages  : 0xfffffffffff9bb32
>   (The number of pages is reduced to 38418230895101%.)
> Memory Hole     : 0x000000000000ac62
> --------------------------------------------------
> Total pages     : 0x000000000007fffe
> 
> So add 2 more parameters for update_cyclic_region. By these adding
> update_cyclic_region can be called in a refined grain. Then effort
> can be saved and above error can be fixed.
> 
> And in kdump compressed dump case, prepare_bitmap2_buffer_cyclic is
> called, that means partial_bitmap2 is malloced twice. Here remove
> the 2nd call.
> ---
>  makedumpfile.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index 3746cf6..8cfc2a5 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -4725,7 +4725,7 @@ exclude_unnecessary_pages_cyclic(void)
>  }
>  
>  int
> -update_cyclic_region(unsigned long long pfn)
> +update_cyclic_region(unsigned long long pfn, int bmp1_set, int exclude)
>  {
>  	if (is_cyclic_region(pfn))
>  		return TRUE;
> @@ -4736,10 +4736,10 @@ update_cyclic_region(unsigned long long pfn)
>  	if (info->cyclic_end_pfn > info->max_mapnr)
>  		info->cyclic_end_pfn = info->max_mapnr;
>  
> -	if (info->flag_elf_dumpfile && !create_1st_bitmap_cyclic())
> +	if (bmp1_set && info->flag_elf_dumpfile && !create_1st_bitmap_cyclic())
>  		return FALSE;
>  
> -	if (!exclude_unnecessary_pages_cyclic())
> +	if (exclude && !exclude_unnecessary_pages_cyclic())
>  		return FALSE;
>  
>  	return TRUE;
> @@ -5502,7 +5502,7 @@ get_num_dumpable_cyclic(void)
>  	unsigned long long pfn, num_dumpable=0;
>  
>  	for (pfn = 0; pfn < info->max_mapnr; pfn++) {
> -		if (!update_cyclic_region(pfn))
> +		if (!update_cyclic_region(pfn, 0, 1))
>  			return FALSE;
>  
>  		if (is_dumpable_cyclic(info->partial_bitmap2, pfn))
> @@ -5803,7 +5803,7 @@ get_loads_dumpfile_cyclic(void)
>  			 * Update target region and bitmap
>  			 */
>  			if (!is_cyclic_region(pfn)) {
> -				if (!update_cyclic_region(pfn))
> +				if (!update_cyclic_region(pfn, 0, 1))
>  					return FALSE;
>  			}
>  
> @@ -5872,7 +5872,7 @@ write_elf_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_page)
>  
>  	info->cyclic_start_pfn = 0;
>  	info->cyclic_end_pfn = 0;
> -	if (!update_cyclic_region(0))
> +	if (!update_cyclic_region(0, 1, 1))
>  		return FALSE;
>  
>  	if (!(phnum = get_phnum_memory()))
> @@ -5910,7 +5910,7 @@ write_elf_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_page)
>  			/*
>  			 * Update target region and partial bitmap if necessary.
>  			 */
> -			if (!update_cyclic_region(pfn))
> +			if (!update_cyclic_region(pfn, 1, 1))
>  				return FALSE;
>  
>  			if (!is_dumpable_cyclic(info->partial_bitmap2, pfn)) {
> @@ -6805,7 +6805,7 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
>  	for (pfn = 0; pfn < info->max_mapnr; pfn++) {
>  		if (is_cyclic_region(pfn))
>  			continue;
> -		if (!update_cyclic_region(pfn))
> +		if (!update_cyclic_region(pfn, 0, 0))
>  			return FALSE;
>  		if (!create_1st_bitmap_cyclic())
>  			return FALSE;
> @@ -6815,9 +6815,6 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
>  
>  	free_bitmap1_buffer();
>  
> -	if (!prepare_bitmap2_buffer_cyclic())
> -		return FALSE;
> -
>  	/*
>  	 * Write pages and bitmap cyclically.
>  	 */
> @@ -6827,7 +6824,7 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
>  		if (is_cyclic_region(pfn))
>  			continue;
>  
> -		if (!update_cyclic_region(pfn))
> +		if (!update_cyclic_region(pfn, 0, 1))
>                          return FALSE;
>  
>  		if (!write_kdump_pages_cyclic(cd_header, cd_page, &pd_zero, &offset_data))
> -- 
> 1.8.3.1
> 
> 
> _______________________________________________
> 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 v2] makedumpfile: add parameters to update_cyclic_region

[HATAYAMA Daisuke]

(2013/11/25 11:31), Baoquan He wrote:
> Hi HATAYAMA and Atsushi,
>
> I think v2 is better than v1, since update_cyclic_region can be used
> with a more flexible calling.
>
> What's your opinion about this?
>
> On 11/23/13 at 05:29pm, Baoquan He wrote:

Thanks for your patch. The bug is caused by my patch set for creating a
whole part of 1st bitmap before entering cyclic process.

I think v1 is better than v2. The update_cyclic_range() call relevant
to this regression is somewhat special compared to other calls; it is
the almost only call that doesn't need to perform filtering processing.
To fix this bug, please make the patch so as not to affect the other calls,
in order to keep change as small as possible.

-- 
Thanks.
HATAYAMA, Daisuke


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

Re: [PATCH v2] makedumpfile: add parameters to update_cyclic_region

[Baoquan He]

On 11/25/13 at 01:33pm, HATAYAMA Daisuke wrote:
> (2013/11/25 11:31), Baoquan He wrote:
> >Hi HATAYAMA and Atsushi,
> >
> >I think v2 is better than v1, since update_cyclic_region can be used
> >with a more flexible calling.
> >
> >What's your opinion about this?
> >
> >On 11/23/13 at 05:29pm, Baoquan He wrote:
> 
> Thanks for your patch. The bug is caused by my patch set for creating a
> whole part of 1st bitmap before entering cyclic process.
> 
> I think v1 is better than v2. The update_cyclic_range() call relevant
> to this regression is somewhat special compared to other calls; it is
> the almost only call that doesn't need to perform filtering processing.
> To fix this bug, please make the patch so as not to affect the other calls,
> in order to keep change as small as possible.

OK, if you think so. But I still think update_cyclic_region is a little
weird, its name doesn't match its functionality, this confuses code
reviewers. And it does something unnecessary somewhere. If it's
possible, I would rather take out the create_1st_bitmap_cyclic and
exclude_unnecessary_pages_cyclic, and call them explicitly where they
are really needed. Surely we can make a little change in both of them,
E.g add a parameter pfn to them, then we can also judge like
update_cyclic_region has done:

        if (is_cyclic_region(pfn))
                return TRUE;

If you insist on v1 is a better idea, I will repost based on it, but keep
my personal opinion.

Baoquan
Thanks

> 
> -- 
> Thanks.
> HATAYAMA, Daisuke
> 
> 
> _______________________________________________
> 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 v2] makedumpfile: add parameters to update_cyclic_region

[Atsushi Kumagai]

Hello Baoquan,

On 2013/11/26 11:53:34, kexec <kexec-bounces@lists.infradead.org> wrote:
> On 11/25/13 at 01:33pm, HATAYAMA Daisuke wrote:
> > (2013/11/25 11:31), Baoquan He wrote:
> > >Hi HATAYAMA and Atsushi,
> > >
> > >I think v2 is better than v1, since update_cyclic_region can be used
> > >with a more flexible calling.
> > >
> > >What's your opinion about this?
> > >
> > >On 11/23/13 at 05:29pm, Baoquan He wrote:
> > 
> > Thanks for your patch. The bug is caused by my patch set for creating a
> > whole part of 1st bitmap before entering cyclic process.
> > 
> > I think v1 is better than v2. The update_cyclic_range() call relevant
> > to this regression is somewhat special compared to other calls; it is
> > the almost only call that doesn't need to perform filtering processing.
> > To fix this bug, please make the patch so as not to affect the other calls,
> > in order to keep change as small as possible.
> 
> OK, if you think so. But I still think update_cyclic_region is a little
> weird, its name doesn't match its functionality, this confuses code
> reviewers. And it does something unnecessary somewhere. If it's
> possible, I would rather take out the create_1st_bitmap_cyclic and
> exclude_unnecessary_pages_cyclic, and call them explicitly where they
> are really needed. Surely we can make a little change in both of them,
> E.g add a parameter pfn to them, then we can also judge like
> update_cyclic_region has done:
> 
>         if (is_cyclic_region(pfn))
>                 return TRUE;
> 
> If you insist on v1 is a better idea, I will repost based on it, but keep
> my personal opinion.

I also prefer v1 because the usage of update_cyclic_region_without_exclude() is
definite and understandable while v2's update_cyclic_region() is complicated.

On the other hand, I agree with your opinion, so could you post a cleanup patch
separately from v1 patch ?


Thanks
Atsushi Kumagai

> Baoquan
> Thanks
> 
> > 
> > -- 
> > Thanks.
> > HATAYAMA, Daisuke
> > 
> > 
> > _______________________________________________
> > 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
> 

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

Re: [PATCH v2] makedumpfile: add parameters to update_cyclic_region

[HATAYAMA Daisuke]

(2013/11/26 11:52), Baoquan He wrote:
> On 11/25/13 at 01:33pm, HATAYAMA Daisuke wrote:
>> (2013/11/25 11:31), Baoquan He wrote:
>>> Hi HATAYAMA and Atsushi,
>>>
>>> I think v2 is better than v1, since update_cyclic_region can be used
>>> with a more flexible calling.
>>>
>>> What's your opinion about this?
>>>
>>> On 11/23/13 at 05:29pm, Baoquan He wrote:
>>
>> Thanks for your patch. The bug is caused by my patch set for creating a
>> whole part of 1st bitmap before entering cyclic process.
>>
>> I think v1 is better than v2. The update_cyclic_range() call relevant
>> to this regression is somewhat special compared to other calls; it is
>> the almost only call that doesn't need to perform filtering processing.
>> To fix this bug, please make the patch so as not to affect the other calls,
>> in order to keep change as small as possible.
>
> OK, if you think so. But I still think update_cyclic_region is a little
> weird, its name doesn't match its functionality, this confuses code
> reviewers. And it does something unnecessary somewhere. If it's
> possible, I would rather take out the create_1st_bitmap_cyclic and
> exclude_unnecessary_pages_cyclic, and call them explicitly where they
> are really needed. Surely we can make a little change in both of them,
> E.g add a parameter pfn to them, then we can also judge like
> update_cyclic_region has done:
>
>          if (is_cyclic_region(pfn))
>                  return TRUE;
>
> If you insist on v1 is a better idea, I will repost based on it, but keep
> my personal opinion.
>

To be honest, I don't like current implementation of cyclic mode, too,
in particular part of update_cyclic_range() and is_cyclic_region() doing
much verbose processing.

I think update of cycle should appear topmost level only. For example,
current implementation in write_kdump_pages_and_bitmap_cyclic()is

         for (pfn = 0; pfn < info->max_mapnr; pfn++) {
                 if (is_cyclic_region(pfn))
                         continue;
                 if (!update_cyclic_region(pfn))
                         return FALSE;
                 if (!create_1st_bitmap_cyclic())
                         return FALSE;
                 if (!write_kdump_bitmap1_cyclic())
                         return FALSE;
         }

and the implementation like this needs dull sanity check in various
positions, for example, in:

int
set_bitmap_cyclic(char *bitmap, unsigned long long pfn, int val)
{
         int byte, bit;

         if (pfn < info->cyclic_start_pfn || info->cyclic_end_pfn <= pfn)
                 return FALSE;

This is due to the implementation above that doesn't satisfy the condition that
any pfn passed to inner function call always within the range of the current
cycle.

Instead, I think it better to change the implementation so the condition
that all the pfns passed to inner functions always within the range of
current cycle.

For example, I locally tried to introduce a kind of for_each_cycle()
statement. See the following. (please ignore details,
  please feel
the atmosphere from the above code.)

struct cycle {
   uint64_t start_pfn;
   uint64_t end_pfn;
};

#define for_each_cycle(C, MAX_MAPNR) \
   for (first_cycle((C), (MAX_MAPNR)); !end_cycle(C); \
        update_cycle(C))

for_each_cycle(&cycle, info->max_mapnr) {
   if (!create_1st_bitmap_cyclic(&cycle))
     return FALSE;
   if (!exclude_unnecessary_pages_cyclic(&cycle))
     return FALSE;
   if (!write_kdump_bitmap1_cyclic(&cycle))
     return FALSE;
}

where it's my preference that range of the current cycle is explicitly
passed to inner functions as a variable cycle.

Anyway, what I'd like to say is: is_cyclic_region(pfn) is unnecessary,
and the part of updating cycle should be done in a fixed one position
for code readability.

BTW, I could successfully clean up the code in this way in kdump-compressed code,
but I couldn't do that in the code from ELF to ELF... So I have yet to post
such clean up patch.

-- 
Thanks.
HATAYAMA, Daisuke


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

Re: [PATCH v2] makedumpfile: add parameters to update_cyclic_region

[Baoquan He]

On 11/26/13 at 02:50pm, HATAYAMA Daisuke wrote:
> (2013/11/26 11:52), Baoquan He wrote:
> >On 11/25/13 at 01:33pm, HATAYAMA Daisuke wrote:
> >>(2013/11/25 11:31), Baoquan He wrote:

> 
> To be honest, I don't like current implementation of cyclic mode, too,
> in particular part of update_cyclic_range() and is_cyclic_region() doing
> much verbose processing.
> 
> I think update of cycle should appear topmost level only. For example,
> current implementation in write_kdump_pages_and_bitmap_cyclic()is
> 
>         for (pfn = 0; pfn < info->max_mapnr; pfn++) {
>                 if (is_cyclic_region(pfn))
>                         continue;
>                 if (!update_cyclic_region(pfn))
>                         return FALSE;
>                 if (!create_1st_bitmap_cyclic())
>                         return FALSE;
>                 if (!write_kdump_bitmap1_cyclic())
>                         return FALSE;
>         }
> 
> and the implementation like this needs dull sanity check in various
> positions, for example, in:
> 
> int
> set_bitmap_cyclic(char *bitmap, unsigned long long pfn, int val)
> {
>         int byte, bit;
> 
>         if (pfn < info->cyclic_start_pfn || info->cyclic_end_pfn <= pfn)
>                 return FALSE;
> 
> This is due to the implementation above that doesn't satisfy the condition that
> any pfn passed to inner function call always within the range of the current
> cycle.
> 
> Instead, I think it better to change the implementation so the condition
> that all the pfns passed to inner functions always within the range of
> current cycle.
> 
> For example, I locally tried to introduce a kind of for_each_cycle()
> statement. See the following. (please ignore details,
>  please feel
> the atmosphere from the above code.)
> 
> struct cycle {
>   uint64_t start_pfn;
>   uint64_t end_pfn;
> };
> 
> #define for_each_cycle(C, MAX_MAPNR) \
>   for (first_cycle((C), (MAX_MAPNR)); !end_cycle(C); \
>        update_cycle(C))
> 
> for_each_cycle(&cycle, info->max_mapnr) {
>   if (!create_1st_bitmap_cyclic(&cycle))
>     return FALSE;
>   if (!exclude_unnecessary_pages_cyclic(&cycle))
>     return FALSE;
>   if (!write_kdump_bitmap1_cyclic(&cycle))
>     return FALSE;
> }
> 
> where it's my preference that range of the current cycle is explicitly
> passed to inner functions as a variable cycle.
> 
> Anyway, what I'd like to say is: is_cyclic_region(pfn) is unnecessary,
> and the part of updating cycle should be done in a fixed one position
> for code readability.
> 
> BTW, I could successfully clean up the code in this way in kdump-compressed code,
> but I couldn't do that in the code from ELF to ELF... So I have yet to post
> such clean up patch.

This is cool, cleanup like this would make code clearer. Let's wait your
clean up patch, I can help review and test.


> 
> -- 
> Thanks.
> HATAYAMA, Daisuke
> 

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

Re: [PATCH v2] makedumpfile: add parameters to update_cyclic_region

[HATAYAMA Daisuke]

(2013/11/26 16:57), Baoquan He wrote:
> On 11/26/13 at 02:50pm, HATAYAMA Daisuke wrote:
>> (2013/11/26 11:52), Baoquan He wrote:
>>> On 11/25/13 at 01:33pm, HATAYAMA Daisuke wrote:
>>>> (2013/11/25 11:31), Baoquan He wrote:
>
>>
>> To be honest, I don't like current implementation of cyclic mode, too,
>> in particular part of update_cyclic_range() and is_cyclic_region() doing
>> much verbose processing.
>>
>> I think update of cycle should appear topmost level only. For example,
>> current implementation in write_kdump_pages_and_bitmap_cyclic()is
>>
>>          for (pfn = 0; pfn < info->max_mapnr; pfn++) {
>>                  if (is_cyclic_region(pfn))
>>                          continue;
>>                  if (!update_cyclic_region(pfn))
>>                          return FALSE;
>>                  if (!create_1st_bitmap_cyclic())
>>                          return FALSE;
>>                  if (!write_kdump_bitmap1_cyclic())
>>                          return FALSE;
>>          }
>>
>> and the implementation like this needs dull sanity check in various
>> positions, for example, in:
>>
>> int
>> set_bitmap_cyclic(char *bitmap, unsigned long long pfn, int val)
>> {
>>          int byte, bit;
>>
>>          if (pfn < info->cyclic_start_pfn || info->cyclic_end_pfn <= pfn)
>>                  return FALSE;
>>
>> This is due to the implementation above that doesn't satisfy the condition that
>> any pfn passed to inner function call always within the range of the current
>> cycle.
>>
>> Instead, I think it better to change the implementation so the condition
>> that all the pfns passed to inner functions always within the range of
>> current cycle.
>>
>> For example, I locally tried to introduce a kind of for_each_cycle()
>> statement. See the following. (please ignore details,
>>   please feel
>> the atmosphere from the above code.)
>>
>> struct cycle {
>>    uint64_t start_pfn;
>>    uint64_t end_pfn;
>> };
>>
>> #define for_each_cycle(C, MAX_MAPNR) \
>>    for (first_cycle((C), (MAX_MAPNR)); !end_cycle(C); \
>>         update_cycle(C))
>>
>> for_each_cycle(&cycle, info->max_mapnr) {
>>    if (!create_1st_bitmap_cyclic(&cycle))
>>      return FALSE;
>>    if (!exclude_unnecessary_pages_cyclic(&cycle))
>>      return FALSE;
>>    if (!write_kdump_bitmap1_cyclic(&cycle))
>>      return FALSE;
>> }
>>
>> where it's my preference that range of the current cycle is explicitly
>> passed to inner functions as a variable cycle.
>>
>> Anyway, what I'd like to say is: is_cyclic_region(pfn) is unnecessary,
>> and the part of updating cycle should be done in a fixed one position
>> for code readability.
>>
>> BTW, I could successfully clean up the code in this way in kdump-compressed code,
>> but I couldn't do that in the code from ELF to ELF... So I have yet to post
>> such clean up patch.
>
> This is cool, cleanup like this would make code clearer. Let's wait your
> clean up patch, I can help review and test.
>

For that, you need to pass a part with the currnet cycle to __exclude_unnecessary_pages(),
not a whole (mmd->pfn_start, mmd->pfn_end). There might be similar part that needs fix,
but sorry I don't have good memory...

int
exclude_unnecessary_pages_cyclic(void)
{
<cut>
                         if (mmd->pfn_end >= info->cyclic_start_pfn &&
                             mmd->pfn_start <= info->cyclic_end_pfn) {
                                 if (!__exclude_unnecessary_pages(mmd->mem_map,
                                                                  mmd->pfn_start, mmd->pfn_end))
                                         return FALSE;
                         }

For ELF-to-ELF code, unfortunately, I gave up in the middle of source code reading.
At lesst, if I remember correctly, I think the code relied on the current
update_mmap_range() implementation. It might be hard to clean up there in a natural way.

-- 
Thanks.
HATAYAMA, Daisuke


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

Re: [PATCH v2] makedumpfile: add parameters to update_cyclic_region

[Baoquan He]

On 11/26/13 at 05:50pm, HATAYAMA Daisuke wrote:
> (2013/11/26 16:57), Baoquan He wrote:
> >On 11/26/13 at 02:50pm, HATAYAMA Daisuke wrote:
> >>(2013/11/26 11:52), Baoquan He wrote:
> >>>On 11/25/13 at 01:33pm, HATAYAMA Daisuke wrote:
> >>>>(2013/11/25 11:31), Baoquan He wrote:
> >
> >>
> >>
> >>BTW, I could successfully clean up the code in this way in kdump-compressed code,
> >>but I couldn't do that in the code from ELF to ELF... So I have yet to post
> >>such clean up patch.
> >
> >This is cool, cleanup like this would make code clearer. Let's wait your
> >clean up patch, I can help review and test.
> >
> 
> For that, you need to pass a part with the currnet cycle to __exclude_unnecessary_pages(),
> not a whole (mmd->pfn_start, mmd->pfn_end). There might be similar part that needs fix,
> but sorry I don't have good memory...
> 
> int
> exclude_unnecessary_pages_cyclic(void)
> {
> <cut>
>                         if (mmd->pfn_end >= info->cyclic_start_pfn &&
>                             mmd->pfn_start <= info->cyclic_end_pfn) {
>                                 if (!__exclude_unnecessary_pages(mmd->mem_map,
>                                                                  mmd->pfn_start, mmd->pfn_end))
>                                         return FALSE;
>                         }
> 
> For ELF-to-ELF code, unfortunately, I gave up in the middle of source code reading.
> At lesst, if I remember correctly, I think the code relied on the current
> update_mmap_range() implementation. It might be hard to clean up there in a natural way.

Yeah, understood. How about I post a patch based on my v1,
then you can do the cleanup work of elf part deliberately.


> 
> -- 
> Thanks.
> HATAYAMA, Daisuke
> 
> 
> _______________________________________________
> 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 v2] makedumpfile: add parameters to update_cyclic_region

[Baoquan He]

On 11/26/13 at 05:50pm, HATAYAMA Daisuke wrote:
> (2013/11/26 16:57), Baoquan He wrote:
> >On 11/26/13 at 02:50pm, HATAYAMA Daisuke wrote:
> >>(2013/11/26 11:52), Baoquan He wrote:
> >>>On 11/25/13 at 01:33pm, HATAYAMA Daisuke wrote:
> >>>>(2013/11/25 11:31), Baoquan He wrote:
> >
> >>

> 
> For that, you need to pass a part with the currnet cycle to __exclude_unnecessary_pages(),
> not a whole (mmd->pfn_start, mmd->pfn_end). There might be similar part that needs fix,
> but sorry I don't have good memory...
> 
> int
> exclude_unnecessary_pages_cyclic(void)
> {
> <cut>
>                         if (mmd->pfn_end >= info->cyclic_start_pfn &&
>                             mmd->pfn_start <= info->cyclic_end_pfn) {
>                                 if (!__exclude_unnecessary_pages(mmd->mem_map,
>                                                                  mmd->pfn_start, mmd->pfn_end))
>                                         return FALSE;
>                         }
> 
> For ELF-to-ELF code, unfortunately, I gave up in the middle of source code reading.
> At lesst, if I remember correctly, I think the code relied on the current
> update_mmap_range() implementation. It might be hard to clean up there in a natural way.

Hi,

I reviewed code again, and understand what kind of pain you meant. I
think the hard part is write_elf_pages_cyclic(), because it is not kind
of like kdump format which loops from 0 to info->max_mapnr, then you can
use the loop MACRO.

struct cycle {
  uint64_t start_pfn;
  uint64_t end_pfn;
};

#define for_each_cycle(C, MAX_MAPNR) \
  for (first_cycle((C), (MAX_MAPNR)); !end_cycle(C); \
       update_cycle(C))

for_each_cycle(&cycle, info->max_mapnr) {
  if (!create_1st_bitmap_cyclic(&cycle))
    return FALSE;
  if (!exclude_unnecessary_pages_cyclic(&cycle))
    return FALSE;
  if (!write_kdump_bitmap1_cyclic(&cycle))
    return FALSE;
}


In fact, don't worry. Please check create_1st_bitmap_cyclic() again, its
handling is very similar to write_elf_pages_cyclic(), because both of
them traverse each PT_LOAD segment, and then do some handling. So you
can call create_1st_bitmap_cyclic() in the big loop which is from 0 to
info->max_mapnr, if rethink about write_elf_pages_cyclic(), you will
find it can also be called like this. Then the cycle loop MACRO can be
used either for elf format handling.

for_each_cycle(&cycle, info->max_mapnr) {
  if (!create_1st_bitmap_cyclic(&cycle))
    return FALSE;
  if (!exclude_unnecessary_pages_cyclic(&cycle))
    return FALSE;
  if (!write_elf_pages_cyclic(&cycle))
    return FALSE;
}

As for mmap, I don't think it will be affected by this change, anyway,
cycle has been passed in, not as a function parameter, but a global
variable (it's stored in global varible info, should be the same).

Baoquan
Thanks

> 
> -- 
> Thanks.
> HATAYAMA, Daisuke
> 
> 
> _______________________________________________
> 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 v2] makedumpfile: add parameters to update_cyclic_region

[Baoquan He]

On 11/26/13 at 03:12am, Atsushi Kumagai wrote:
> Hello Baoquan,
> 
> On 2013/11/26 11:53:34, kexec <kexec-bounces@lists.infradead.org> wrote:
> > On 11/25/13 at 01:33pm, HATAYAMA Daisuke wrote:
> > > (2013/11/25 11:31), Baoquan He wrote:
> > > >Hi HATAYAMA and Atsushi,
> > > >
> > > >I think v2 is better than v1, since update_cyclic_region can be used
> > > >with a more flexible calling.
> > > >
> > > >What's your opinion about this?
> > > >
> > > >On 11/23/13 at 05:29pm, Baoquan He wrote:
> > > 
> > > Thanks for your patch. The bug is caused by my patch set for creating a
> > > whole part of 1st bitmap before entering cyclic process.
> > > 
> > > I think v1 is better than v2. The update_cyclic_range() call relevant
> > > to this regression is somewhat special compared to other calls; it is
> > > the almost only call that doesn't need to perform filtering processing.
> > > To fix this bug, please make the patch so as not to affect the other calls,
> > > in order to keep change as small as possible.
> > 
> > OK, if you think so. But I still think update_cyclic_region is a little
> > weird, its name doesn't match its functionality, this confuses code
> > reviewers. And it does something unnecessary somewhere. If it's
> > possible, I would rather take out the create_1st_bitmap_cyclic and
> > exclude_unnecessary_pages_cyclic, and call them explicitly where they
> > are really needed. Surely we can make a little change in both of them,
> > E.g add a parameter pfn to them, then we can also judge like
> > update_cyclic_region has done:
> > 
> >         if (is_cyclic_region(pfn))
> >                 return TRUE;
> > 
> > If you insist on v1 is a better idea, I will repost based on it, but keep
> > my personal opinion.
> 
> I also prefer v1 because the usage of update_cyclic_region_without_exclude() is
> definite and understandable while v2's update_cyclic_region() is complicated.
> 
> On the other hand, I agree with your opinion, so could you post a cleanup patch
> separately from v1 patch ?

Hi Atsushi,

Yeah, you are right, v1 is better. Then we can wait for HATAYAMA's cleanup
patch. I know you are goint to release v1.5.5.

Baoquan
Thanks


> 
> 
> Thanks
> Atsushi Kumagai
> 
> > Baoquan
> > Thanks
> > 
> > > 
> > > -- 
> > > Thanks.
> > > HATAYAMA, Daisuke
> > > 
> > > 
> > > _______________________________________________
> > > 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
> > 
> 
> _______________________________________________
> 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 v2] makedumpfile: add parameters to update_cyclic_region

[HATAYAMA Daisuke]

(2013/11/27 14:37), Baoquan He wrote:
> On 11/26/13 at 03:12am, Atsushi Kumagai wrote:
>> Hello Baoquan,
>>
>> On 2013/11/26 11:53:34, kexec <kexec-bounces@lists.infradead.org> wrote:
>>> On 11/25/13 at 01:33pm, HATAYAMA Daisuke wrote:
>>>> (2013/11/25 11:31), Baoquan He wrote:
>>>>> Hi HATAYAMA and Atsushi,
>>>>>
>>>>> I think v2 is better than v1, since update_cyclic_region can be used
>>>>> with a more flexible calling.
>>>>>
>>>>> What's your opinion about this?
>>>>>
>>>>> On 11/23/13 at 05:29pm, Baoquan He wrote:
>>>>
>>>> Thanks for your patch. The bug is caused by my patch set for creating a
>>>> whole part of 1st bitmap before entering cyclic process.
>>>>
>>>> I think v1 is better than v2. The update_cyclic_range() call relevant
>>>> to this regression is somewhat special compared to other calls; it is
>>>> the almost only call that doesn't need to perform filtering processing.
>>>> To fix this bug, please make the patch so as not to affect the other calls,
>>>> in order to keep change as small as possible.
>>>
>>> OK, if you think so. But I still think update_cyclic_region is a little
>>> weird, its name doesn't match its functionality, this confuses code
>>> reviewers. And it does something unnecessary somewhere. If it's
>>> possible, I would rather take out the create_1st_bitmap_cyclic and
>>> exclude_unnecessary_pages_cyclic, and call them explicitly where they
>>> are really needed. Surely we can make a little change in both of them,
>>> E.g add a parameter pfn to them, then we can also judge like
>>> update_cyclic_region has done:
>>>
>>>          if (is_cyclic_region(pfn))
>>>                  return TRUE;
>>>
>>> If you insist on v1 is a better idea, I will repost based on it, but keep
>>> my personal opinion.
>>
>> I also prefer v1 because the usage of update_cyclic_region_without_exclude() is
>> definite and understandable while v2's update_cyclic_region() is complicated.
>>
>> On the other hand, I agree with your opinion, so could you post a cleanup patch
>> separately from v1 patch ?
>
> Hi Atsushi,
>
> Yeah, you are right, v1 is better. Then we can wait for HATAYAMA's cleanup
> patch. I know you are goint to release v1.5.5.
>

I don't have a time to make a clean-up patch now, at least within this year.

-- 
Thanks.
HATAYAMA, Daisuke


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

Re: [PATCH v2] makedumpfile: add parameters to update_cyclic_region

[Baoquan He]

On 11/27/13 at 04:37pm, HATAYAMA Daisuke wrote:
> (2013/11/27 14:37), Baoquan He wrote:
> >On 11/26/13 at 03:12am, Atsushi Kumagai wrote:
> >>Hello Baoquan,
> >>

> >>I also prefer v1 because the usage of update_cyclic_region_without_exclude() is
> >>definite and understandable while v2's update_cyclic_region() is complicated.
> >>
> >>On the other hand, I agree with your opinion, so could you post a cleanup patch
> >>separately from v1 patch ?
> >
> >Hi Atsushi,
> >
> >Yeah, you are right, v1 is better. Then we can wait for HATAYAMA's cleanup
> >patch. I know you are goint to release v1.5.5.
> >
> 
> I don't have a time to make a clean-up patch now, at least within this year.

OK, I will try to post a clean-up patch based on your cycle loop method.
But I can not guarantee the completion date.

> 
> -- 
> Thanks.
> HATAYAMA, Daisuke
> 
> 
> _______________________________________________
> 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