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