Re: [PATCH -next resend v3] mm: hwposion: support recovery from ksm_might_need_to_copy()

[Kefeng Wang]

On 2022/12/17 10:24, Miaohe Lin wrote:
> On 2022/12/16 9:47, HORIGUCHI NAOYA(堀口 直也) wrote:
> > On Tue, Dec 13, 2022 at 08:05:23PM +0800, Kefeng Wang wrote:
> > > When the kernel copy a page from ksm_might_need_to_copy(), but runs
> > > into an uncorrectable error, it will crash since poisoned page is
> > > consumed by kernel, this is similar to Copy-on-write poison recovery,
> >
> > Maybe you mean "this is similar to the issue recently fixed by
> > Copy-on-write poison recovery."?  And if this sentence ends here,
> > please put "." instead of ",".
> >
> > > When an error is detected during the page copy, return VM_FAULT_HWPOISON
> > > in do_swap_page(), and install a hwpoison entry in unuse_pte() when
> > > swapoff, which help us to avoid system crash.  Note, memory failure on
> > > a KSM page will be skipped, but still call memory_failure_queue() to
> > > be consistent with general memory failure process.
> >
> > Thank you for the work.  I have a few comment below ...
>
> Thanks both.
>
> > > -	if (unlikely(!PageUptodate(page))) {
> > > -		pte_t pteval;
> > > +	if (hwposioned || !PageUptodate(page)) {
> > > +		swp_entry_t swp_entry;
> > >   
> > >   		dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
> > > -		pteval = swp_entry_to_pte(make_swapin_error_entry());
> > > -		set_pte_at(vma->vm_mm, addr, pte, pteval);
> > > -		swap_free(entry);
> > > +		if (hwposioned) {
> > > +			swp_entry = make_hwpoison_entry(swapcache);
> > > +			page = swapcache;
> >
> > This might work for the process accessing to the broken page, but ksm
> > pages are likely to be shared by multiple processes, so it would be
> > much nicer if you can convert all mapping entries for the error ksm page
> > into hwpoisoned ones.  Maybe in this thorough approach,
> > hwpoison_user_mappings() is updated to call try_to_unmap() for ksm pages.
> > But it's not necessary to do this together with applying mcsafe-memcpy,
> > because recovery action and mcsafe-memcpy can be done independently.
>
> I'm afraid leaving the ksm page in the cache will repeatedly trigger uncorrectable error for the
> same page if ksm pages are shared by multiple processes. This might reach the hardware threshold
> and result in fatal uncorrectable error (thus casuing system to panic). So IMHO it might be better
> to check if page is hwpoisoned before calling ksm_might_need_to_copy() if above thorough approach
> is not implemented. But I can easily be wrong.
Oh，missing this one，there are only two caller, one in do_swap_page(),
it has already check whether the page is hwpoisoned or not, another is 
in unuse_pte() which only called from swapoff, it is not a hot patch, so
I don't think it is an urgent requirement. Thanks.