[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
>