Re: [PATCH v2] makedumpfile: add parameters to update_cyclic_region

[HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>]

(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
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec