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

[HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>]

(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