(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
