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.
> >
> >>+ 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.
> >
> >>+ 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.
Thank you for your fix!
Petr T
