lkml.org 
[lkml]   [2020]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH -V3] mm: Add PageLayzyFree() helper functions for MADV_FREE
    On Mon 09-03-20 09:55:38, David Hildenbrand wrote:
    > On 09.03.20 03:17, Huang, Ying wrote:
    [...]
    > > @@ -1235,7 +1234,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
    > > * Try to allocate it some swap space here.
    > > * Lazyfree page could be freed directly
    > > */
    > > - if (PageAnon(page) && PageSwapBacked(page)) {
    > > + if (PageAnon(page) && !__PageLazyFree(page)) {
    > > if (!PageSwapCache(page)) {
    > > if (!(sc->gfp_mask & __GFP_IO))
    > > goto keep_locked;
    > > @@ -1411,7 +1410,7 @@ static unsigned long shrink_page_list(struct list_head *page_list,
    > > }
    > > }
    > >
    > > - if (PageAnon(page) && !PageSwapBacked(page)) {
    > > + if (PageLazyFree(page)) {
    > > /* follow __remove_mapping for reference */
    > > if (!page_ref_freeze(page, 1))
    > > goto keep_locked;
    > >
    >
    > I still prefer something like
    >
    > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
    > index fd6d4670ccc3..7538501230bd 100644
    > --- a/include/linux/page-flags.h
    > +++ b/include/linux/page-flags.h
    > @@ -63,6 +63,10 @@
    > * page_waitqueue(page) is a wait queue of all tasks waiting for the page
    > * to become unlocked.
    > *
    > + * PG_swapbacked used with anonymous pages (PageAnon()) indicates that a
    > + * page is backed by swap. Anonymous pages without PG_swapbacked are
    > + * pages that can be lazily freed (e.g., MADV_FREE) on demand.
    > + *
    > * PG_uptodate tells whether the page's contents is valid. When a read
    > * completes, the page becomes uptodate, unless a disk I/O error happened.
    > *
    >
    > and really don't like the use of !__PageLazyFree() instead of PageSwapBacked().

    I have to say that I do not have a strong opinion about helper
    functions. In general I tend to be against adding them unless there is a
    very good reason for them. This particular patch is in a gray zone a bit.

    There are few places which are easier to follow but others sound like,
    we have a hammer let's use it. E.g. shrink_page_list path above. There
    is a clear comment explaining PageAnon && PageSwapBacked check being
    LazyFree related but do I have to know that this is LazyFree path? I
    believe that seeing PageSwapBacked has a more meaning to me because it
    tells me that anonymous pages without a backing store doesn't really
    need swap entry. This happens to be Lazy free related today but with a
    heavy overloading of our flags this might differ in the future. You have
    effectively made a more generic description more specific without a very
    good reason.

    On the other hand having PG_swapbacked description in page-flags.h above
    gives a very useful information which was previously hidden at the
    definition so this is a clear improvement.

    That being said I think that the patch is not helpful enough. I would
    much rather see a simply documentation update.

    --
    Michal Hocko
    SUSE Labs

    \
     
     \ /
      Last update: 2020-03-09 13:13    [W:3.558 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site