[PATCH 2/3] makedumpfile: Mark unstored ELF pages as filtered

[Atsushi Kumagai]

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