| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 3 Mar 2015 14:46:40 +0800
From: "Wang, Yalin" <Yalin.Wang@...ymobile.com>
To: 'Minchan Kim' <minchan@...nel.org>
CC: 'Michal Hocko' <mhocko@...e.cz>,
'Andrew Morton' <akpm@...ux-foundation.org>,
"'linux-kernel@...r.kernel.org'" <linux-kernel@...r.kernel.org>,
"'linux-mm@...ck.org'" <linux-mm@...ck.org>,
'Rik van Riel' <riel@...hat.com>,
'Johannes Weiner' <hannes@...xchg.org>,
'Mel Gorman' <mgorman@...e.de>, 'Shaohua Li' <shli@...nel.org>,
Hugh Dickins <hughd@...gle.com>,
Cyrill Gorcunov <gorcunov@...il.com>
Subject: RE: [RFC V3] mm: change mm_advise_free to clear page dirty
> -----Original Message-----
> From: Minchan Kim [mailto:minchan.kim@...il.com] On Behalf Of Minchan Kim
> Sent: Tuesday, March 03, 2015 12:15 PM
> To: Wang, Yalin
> Cc: 'Michal Hocko'; 'Andrew Morton'; 'linux-kernel@...r.kernel.org';
> 'linux-mm@...ck.org'; 'Rik van Riel'; 'Johannes Weiner'; 'Mel Gorman';
> 'Shaohua Li'; Hugh Dickins; Cyrill Gorcunov
> Subject: Re: [RFC V3] mm: change mm_advise_free to clear page dirty
>
> On Tue, Mar 03, 2015 at 11:59:17AM +0800, Wang, Yalin wrote:
> > > -----Original Message-----
> > > From: Minchan Kim [mailto:minchan.kim@...il.com] On Behalf Of Minchan
> Kim
> > > Sent: Tuesday, March 03, 2015 11:26 AM
> > > To: Wang, Yalin
> > > Cc: 'Michal Hocko'; 'Andrew Morton'; 'linux-kernel@...r.kernel.org';
> > > 'linux-mm@...ck.org'; 'Rik van Riel'; 'Johannes Weiner'; 'Mel Gorman';
> > > 'Shaohua Li'; Hugh Dickins; Cyrill Gorcunov
> > > Subject: Re: [RFC V3] mm: change mm_advise_free to clear page dirty
> > >
> > > Could you separte this patch in this patchset thread?
> > > It's tackling differnt problem.
> > >
> > > As well, I had a question to previous thread about why shared page
> > > has a problem now but you didn't answer and send a new patchset.
> > > It makes reviewers/maintainer time waste/confuse. Please, don't
> > > hurry to send a code. Before that, resolve reviewers's comments.
> > >
> > > On Tue, Mar 03, 2015 at 10:06:40AM +0800, Wang, Yalin wrote:
> > > > This patch add ClearPageDirty() to clear AnonPage dirty flag,
> > > > if not clear page dirty for this anon page, the page will never be
> > > > treated as freeable. We also make sure the shared AnonPage is not
> > > > freeable, we implement it by dirty all copyed AnonPage pte,
> > > > so that make sure the Anonpage will not become freeable, unless
> > > > all process which shared this page call madvise_free syscall.
> > >
> > > Please, spend more time to make description clear. I really doubt
> > > who understand this description without code inspection. :(
> > > Of course, I'm not a person to write description clear like native
> > > , either but just I'm sure I spend a more time to write description
> > > rather than coding, at least. :)
> > >
> > I see, I will send another mail for file private map pages.
> > Sorry for my English expressions.
> > I think your solution is ok,
> > Your patch will make sure the anonpage pte will always be dirty.
> > I add some comments for your patch:
> >
> > > ---
> > > mm/madvise.c | 1 -
> > > mm/memory.c | 9 +++++++--
> > > mm/rmap.c | 2 +-
> > > mm/vmscan.c | 3 +--
> > > 4 files changed, 9 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index 6d0fcb8..d64200e 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -309,7 +309,6 @@ static int madvise_free_pte_range(pmd_t *pmd,
> unsigned
> > > long addr,
> > > continue;
> > > }
> > >
> > > - ClearPageDirty(page);
> > > unlock_page(page);
> > > }
> > >
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index 8ae52c9..2f45e77 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -2460,9 +2460,14 @@ static int do_swap_page(struct mm_struct *mm,
> struct
> > > vm_area_struct *vma,
> > >
> > > inc_mm_counter_fast(mm, MM_ANONPAGES);
> > > dec_mm_counter_fast(mm, MM_SWAPENTS);
> > > - pte = mk_pte(page, vma->vm_page_prot);
> > > +
> > > + /*
> > > + * Every page swapped-out was pte_dirty so we makes pte dirty again.
> > > + * MADV_FREE relys on it.
> > > + */
> > > + pte = mk_pte(pte_mkdirty(page), vma->vm_page_prot);
> > pte_mkdirty() usage seems wrong here.
>
> Argh, it reveals I didn't test even build. My shame.
> But RFC tag might mitigate my shame. :)
> I will fix it if I send a formal version.
> Thanks for the review.
>
> >
> > > if ((flags & FAULT_FLAG_WRITE) && reuse_swap_page(page)) {
> > > - pte = maybe_mkwrite(pte_mkdirty(pte), vma);
> > > + pte = maybe_mkwrite(pte, vma);
> > > flags &= ~FAULT_FLAG_WRITE;
> > > ret |= VM_FAULT_WRITE;
> > > exclusive = 1;
> > > diff --git a/mm/rmap.c b/mm/rmap.c
> > > index 47b3ba8..34c1d66 100644
> > > --- a/mm/rmap.c
> > > +++ b/mm/rmap.c
> > > @@ -1268,7 +1268,7 @@ static int try_to_unmap_one(struct page *page,
> struct
> > > vm_area_struct *vma,
> > >
> > > if (flags & TTU_FREE) {
> > > VM_BUG_ON_PAGE(PageSwapCache(page), page);
> > > - if (!dirty && !PageDirty(page)) {
> > > + if (!dirty) {
> > > /* It's a freeable page by MADV_FREE */
> > > dec_mm_counter(mm, MM_ANONPAGES);
> > > goto discard;
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index 671e47e..7f520c9 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -805,8 +805,7 @@ static enum page_references
> > > page_check_references(struct page *page,
> > > return PAGEREF_KEEP;
> > > }
> > >
> > > - if (PageAnon(page) && !pte_dirty && !PageSwapCache(page) &&
> > > - !PageDirty(page))
> > > + if (PageAnon(page) && !pte_dirty && !PageSwapCache(page))
> > > *freeable = true;
> > >
> > > /* Reclaim if clean, defer dirty pages to writeback */
> > > --
> > > 1.9.3
> > Could we remove SetPageDirty(page); in try_to_free_swap() function based
> on this patch?
> > Because your patch will make sure the pte is always dirty,
> > We don't need setpagedirty(),
> > The try_to_unmap() path will re-dirty the page during reclaim path,
> > Isn't it?
>
> I dont't know what side-effect we will have if we removes SetPageDirty.
> It might regress on tmpfs which would page without pte.
> I don't want to have such risk in this patch.
> If you want it, you could suggest it separately if this patch lands.
>
Ok, Could you send out your change as a normal patch for more related maintainers to review /comment it?
Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists