On Fri, Mar 13, 2020 at 09:59:50AM -0700, Dave Hansen wrote:
> On 3/12/20 7:00 PM, Minchan Kim wrote:
> > On Thu, Mar 12, 2020 at 02:41:07PM -0700, Dave Hansen wrote:
> >> One other fun thing. I have a "victim" thread sitting in a loop doing:
> >>
> >> sleep(1)
> >> memcpy(&garbage, buffer, sz);
> >>
> >> The "attacker" is doing
> >>
> >> madvise(buffer, sz, MADV_PAGEOUT);
> >>
> >> in a loop. That, oddly enough doesn't cause the victim to page fault.
> >> But, if I do:
> >>
> >> memcpy(&garbage, buffer, sz);
> >> madvise(buffer, sz, MADV_PAGEOUT);
> >>
> >> It *does* cause the memory to get paged out. The MADV_PAGEOUT code
> >> actually has a !pte_present() check. It will punt on a PTE if it sees
> >> it. In other words, if a page is in the swap cache but not mapped by a
> >> pte_present() PTE, MADV_PAGEOUT won't touch it.
> >>
> >> Shouldn't MADV_PAGEOUT be able to find and reclaim those pages? Patch
> >> attached.
> >
> >>
> >>
> >> ---
> >>
> >> b/mm/madvise.c | 38 +++++++++++++++++++++++++++++++-------
> >> 1 file changed, 31 insertions(+), 7 deletions(-)
> >>
> >> diff -puN mm/madvise.c~madv-pageout-find-swap-cache mm/madvise.c
> >> --- a/mm/madvise.c~madv-pageout-find-swap-cache 2020-03-12 14:24:45.178775035 -0700
> >> +++ b/mm/madvise.c 2020-03-12 14:35:49.706773378 -0700
> >> @@ -248,6 +248,36 @@ static void force_shm_swapin_readahead(s
> >> #endif /* CONFIG_SWAP */
> >>
> >> /*
> >> + * Given a PTE, find the corresponding 'struct page'. Also handles
> >> + * non-present swap PTEs.
> >> + */
> >> +struct page *pte_to_reclaim_page(struct vm_area_struct *vma,
> >> + unsigned long addr, pte_t ptent)
> >> +{
> >> + swp_entry_t entry;
> >> +
> >> + /* Totally empty PTE: */
> >> + if (pte_none(ptent))
> >> + return NULL;
> >> +
> >> + /* A normal, present page is mapped: */
> >> + if (pte_present(ptent))
> >> + return vm_normal_page(vma, addr, ptent);
> >> +
> >
> > Please check is_swap_pte first.
>
> Why?
>
> is_swap_pte() duplicates the first two checks. But, I need an explicit
> pte_present() check somewhere because I need to call vm_normal_page()
> only on present PTEs.
>
> I guess the pte_present() check could be:
>
> if (!is_swap_pte(ptent))
> return vm_normal_page(...);
>
> *after* the pte_none() check.
Yub, I thought is_swap_pte looks more readable and maintainable for
the change of pte encoding in future. Anyway, I am not insisting.
>
> >> + entry = pte_to_swp_entry(vmf->orig_pte);
> >> + /* Is it one of the "swap PTEs" that's not really swap? */
> >> + if (non_swap_entry(entry))
> >> + return false;
> >> +
> >> + /*
> >> + * The PTE was a true swap entry. The page may be in the
> >> + * swap cache. If so, find it and return it so it may be
> >> + * reclaimed.
> >> + */
> >> + return lookup_swap_cache(entry, vma, addr);
> >
> > If we go with handling only exclusived owned page for anon,
> > I think we should apply the rule to swap cache, too.
>
> I'm going back and forth on it. If we're just trying to avoid causing
> faults in other processes, we could add a mapcount>0 check here in
> addition to the mapcount>1 checks that were added in the other patch.
>
> But, if we want a check for true exclusivity: no other swap entries or
> mappings, we need to check swap_count() too. It's getting quite a bit
> uglier as I add that it, but I guess we'll see how it looks in the end.
If we go to the map_count > 1 check here and follows the Daniel's suggestion
of MADV_PAGEOUT_ALL to make shared page paging out, that means it clearly
makes semantic change for MADV_PAGEOUT: "paging out only exclusive owned page"
so it would be rather weired if we reclaim swap_count() > 1 of swap cache.
>
> > Do you mind posting it as formal patch?
>
> Yeah, I'll send something out.
Thanks for bring up the issue, Dave!
