(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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
