Re: [PATCH 3/4] Check whether pages are poisoned before copying

[K.Prasad]

On Thu, Mar 17, 2011 at 03:04:01PM +0100, Andrea Arcangeli wrote:
> Hello Hidetoshi,
> 
> On Thu, Mar 17, 2011 at 04:43:03PM +0900, Hidetoshi Seto wrote:
> > Still I think making the window smaller than now is worthwhile,
> > no matter it is change from 0.1% to 0.01%, or from 0.01% to 0.001%.
> > 
> > Or did you find the downside of the check here?
> 
> Well it makes the code more a little more complicated for something
> that looks impossible to trigger in the first place.
> 
> The slowdown of these changes is probably not significant because the
> 2m copy should dominate the collapse_huge_page cost, but it still add
> locked ops and loops that weren't there before so technically it is a
> microslowdown.
> 
> NOTE: if this closed the race window 100% I would not disagree with
> this. If there's still a 0.001% chance of hitting the race like Andi
> hints, then I don't find it very attractive. I think memory failure
> isn't 100% correct and probably it's impossible to make it 100%
> correct across the whole kernel (for example the compound_head is safe
> for THP but it's still unsafe for hugetlbfs while the page is being
> tear down), so it's probably ok that it tends to work in practice 100%
> reliably when the task is running in userland but we leave holes when
> kernel is mangling the page structures and moving stuff around,
> otherwise we litter the kernel code without much practical benefit, I
> guess KSM has the same issues of khugepaged for example.
> 

On an extended note, I don't understand why hwpoison code should not
handle Ksm pages the same way as other user-space pages. While it is
known that certain races between merging of pages by Ksm and poisoning
of code exist, the limitation posed for PageKsm() pages don't seem to
avoid it.

It appears that the restriction for PageKsm() can be removed from
memory-failure.c code without making the races any better or worse. Any
opinions on that?

Thanks,
K.Prasad

> So again, I'm not against making these changes, but I don't find them
> very attractive and I'm unsure if we should go down this route
> whenever the objective of the patch is only to reduce the race window
> (that is incredibly small and not reproducible to start with, and it's
> a theoretical race that hardly anybody could trigger) instead of
> actually closing the race completely. But thanks a lot for your
> effort, I see your point, I'm just not sure if it's worth it. I think
> I'll let other comments on this...
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/