(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
