Hello Petr,
I'm happy to get your reply.
>On Tue, 7 Jun 2016 04:18:48 +0000
>Atsushi Kumagai <ats-kumagai at wm.jp.nec.com> wrote:
>
>> >>+static void
>> >>+exclude_nodata_pages(struct cycle *cycle)
>> >>+{
>> >>+ int i;
>> >>+ unsigned long long phys_start, phys_end;
>> >>+ off_t file_size;
>> >>+
>> >>+ i = 0;
>> >>+ while (get_pt_load_extents(i, &phys_start, &phys_end,
>> >>+ NULL, &file_size)) {
>> >>+ unsigned long long pfn, pfn_end;
>> >>+
>> >>+ pfn = paddr_to_pfn(phys_start + file_size);
>> >>+ pfn_end = paddr_to_pfn(phys_end);
>> >
>> >Does this code exclude the first pfn of out of PT_LOAD even if there is
>> >no unstored pages ? pfn and pfn_end will point at the next pfn to the
>> >last pfn of PT_LOAD.
>
>Indeed, phys_end is in fact the first physical address that is _not_
>contained in the segment.
>
>Good catch!
>
>> >This will be problem for the environments which have a overlapped PT_LOAD
>> >segment like ia64.
>
>I think it's quite the opposite. Partial pages on IA64 are the only
>ones that were handled correctly with the old code.
I didn't take care partial pages, just considered the case below:
pfn 1 2 3 4 5 6 7
------------------------------------------------
PT_LAOD(1) |---+---+---+---|
PT_LAOD(2) |---+---+---+---|
During checking PT_LOAD(1), the bit of pfn:5 will be dropped if
mem_size equals file_size, but practically the pfn is an used page
as PT_LAOD(2) shows.
If there is only PT_LOAD(1), pfn:5 must be on hole and the actual
harm doesn't occur.
>> >
>> >>+ if (pfn < cycle->start_pfn)
>> >>+ pfn = cycle->start_pfn;
>> >>+ if (pfn_end >= cycle->end_pfn)
>> >>+ pfn_end = cycle->end_pfn - 1;
>> >>+ while (pfn <= pfn_end) {
>> >
>> >Should we change this condition to "pfn < pfn_end" ?
>
>Better than nothing, but if the last page of a LOAD segment is not
>partial, it will not be excluded. Except for partial pages, end_pfn
>refers to the first PFN _after_ the current segment.
If mem_size equals file_size, the last page of a LOAD also shouldn't
be removed. However, the condition of "pfn <= pfn_end" can remove it
since end_pfn refers to the first PFN _after_ the current segment.
I just fixed it.
>> >
>> >>+ clear_bit_on_2nd_bitmap(pfn, cycle);
>> >>+ ++pfn;
>> >>+ }
>> >>+ ++i;
>> >>+ }
>> >>+}
>>
>> I fixed this as below for v1.6.0.
>> Of course your comment would still be helpful.
>
>It comes too late, I'm afraid.
>
>In all ia64 dumps with partial pages I have seen, the full content of
>the partial page is indeed stored in another segment, so the page would
>be marked incorrectly. OTOH I think the off-by-one bug you fixed
>affected all segments, even those without any partial pages; it would
>incorrectly mark the first PFN after the segment as filtered.
>
>The perfect solution is to round the starting PFN up instead of down to
>preserve partial pages, but I'll have to think about it some more
>before I can send a patch.
Again, I didn't consider partial page cases, I also should reconsider this.
Thank you for pointing it out.
Regards,
Atsushi Kumagai
