Re: [PATCH v4 4/6] mm: Introduce a pageflag for partially mapped folios

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Aug 20, 2024 at 9:34 AM Barry Song <baohua@xxxxxxxxxx> wrote:
>
> On Tue, Aug 20, 2024 at 8:16 AM Usama Arif <usamaarif642@xxxxxxxxx> wrote:
> >
> >
> >
> > On 19/08/2024 20:00, Barry Song wrote:
> > > On Tue, Aug 20, 2024 at 2:17 AM Usama Arif <usamaarif642@xxxxxxxxx> wrote:
> > >>
> > >>
> > >>
> > >> On 19/08/2024 09:29, Barry Song wrote:
> > >>> Hi Usama,
> > >>>
> > >>> I feel it is much better now! thanks!
> > >>>
> > >>> On Mon, Aug 19, 2024 at 2:31 PM Usama Arif <usamaarif642@xxxxxxxxx> wrote:
> > >>>>
> > >>>> Currently folio->_deferred_list is used to keep track of
> > >>>> partially_mapped folios that are going to be split under memory
> > >>>> pressure. In the next patch, all THPs that are faulted in and collapsed
> > >>>> by khugepaged are also going to be tracked using _deferred_list.
> > >>>>
> > >>>> This patch introduces a pageflag to be able to distinguish between
> > >>>> partially mapped folios and others in the deferred_list at split time in
> > >>>> deferred_split_scan. Its needed as __folio_remove_rmap decrements
> > >>>> _mapcount, _large_mapcount and _entire_mapcount, hence it won't be
> > >>>> possible to distinguish between partially mapped folios and others in
> > >>>> deferred_split_scan.
> > >>>>
> > >>>> Eventhough it introduces an extra flag to track if the folio is
> > >>>> partially mapped, there is no functional change intended with this
> > >>>> patch and the flag is not useful in this patch itself, it will
> > >>>> become useful in the next patch when _deferred_list has non partially
> > >>>> mapped folios.
> > >>>>
> > >>>> Signed-off-by: Usama Arif <usamaarif642@xxxxxxxxx>
> > >>>> ---
> > >>>>  include/linux/huge_mm.h    |  4 ++--
> > >>>>  include/linux/page-flags.h | 11 +++++++++++
> > >>>>  mm/huge_memory.c           | 23 ++++++++++++++++-------
> > >>>>  mm/internal.h              |  4 +++-
> > >>>>  mm/memcontrol.c            |  3 ++-
> > >>>>  mm/migrate.c               |  3 ++-
> > >>>>  mm/page_alloc.c            |  5 +++--
> > >>>>  mm/rmap.c                  |  5 +++--
> > >>>>  mm/vmscan.c                |  3 ++-
> > >>>>  9 files changed, 44 insertions(+), 17 deletions(-)
> > >>>>
> > >>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > >>>> index 4c32058cacfe..969f11f360d2 100644
> > >>>> --- a/include/linux/huge_mm.h
> > >>>> +++ b/include/linux/huge_mm.h
> > >>>> @@ -321,7 +321,7 @@ static inline int split_huge_page(struct page *page)
> > >>>>  {
> > >>>>         return split_huge_page_to_list_to_order(page, NULL, 0);
> > >>>>  }
> > >>>> -void deferred_split_folio(struct folio *folio);
> > >>>> +void deferred_split_folio(struct folio *folio, bool partially_mapped);
> > >>>>
> > >>>>  void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> > >>>>                 unsigned long address, bool freeze, struct folio *folio);
> > >>>> @@ -495,7 +495,7 @@ static inline int split_huge_page(struct page *page)
> > >>>>  {
> > >>>>         return 0;
> > >>>>  }
> > >>>> -static inline void deferred_split_folio(struct folio *folio) {}
> > >>>> +static inline void deferred_split_folio(struct folio *folio, bool partially_mapped) {}
> > >>>>  #define split_huge_pmd(__vma, __pmd, __address)        \
> > >>>>         do { } while (0)
> > >>>>
> > >>>> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > >>>> index a0a29bd092f8..c3bb0e0da581 100644
> > >>>> --- a/include/linux/page-flags.h
> > >>>> +++ b/include/linux/page-flags.h
> > >>>> @@ -182,6 +182,7 @@ enum pageflags {
> > >>>>         /* At least one page in this folio has the hwpoison flag set */
> > >>>>         PG_has_hwpoisoned = PG_active,
> > >>>>         PG_large_rmappable = PG_workingset, /* anon or file-backed */
> > >>>> +       PG_partially_mapped = PG_reclaim, /* was identified to be partially mapped */
> > >>>>  };
> > >>>>
> > >>>>  #define PAGEFLAGS_MASK         ((1UL << NR_PAGEFLAGS) - 1)
> > >>>> @@ -861,8 +862,18 @@ static inline void ClearPageCompound(struct page *page)
> > >>>>         ClearPageHead(page);
> > >>>>  }
> > >>>>  FOLIO_FLAG(large_rmappable, FOLIO_SECOND_PAGE)
> > >>>> +FOLIO_TEST_FLAG(partially_mapped, FOLIO_SECOND_PAGE)
> > >>>> +/*
> > >>>> + * PG_partially_mapped is protected by deferred_split split_queue_lock,
> > >>>> + * so its safe to use non-atomic set/clear.
> > >>>> + */
> > >>>> +__FOLIO_SET_FLAG(partially_mapped, FOLIO_SECOND_PAGE)
> > >>>> +__FOLIO_CLEAR_FLAG(partially_mapped, FOLIO_SECOND_PAGE)
> > >>>>  #else
> > >>>>  FOLIO_FLAG_FALSE(large_rmappable)
> > >>>> +FOLIO_TEST_FLAG_FALSE(partially_mapped)
> > >>>> +__FOLIO_SET_FLAG_NOOP(partially_mapped)
> > >>>> +__FOLIO_CLEAR_FLAG_NOOP(partially_mapped)
> > >>>>  #endif
> > >>>>
> > >>>>  #define PG_head_mask ((1UL << PG_head))
> > >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > >>>> index 2d77b5d2291e..70ee49dfeaad 100644
> > >>>> --- a/mm/huge_memory.c
> > >>>> +++ b/mm/huge_memory.c
> > >>>> @@ -3398,6 +3398,7 @@ int split_huge_page_to_list_to_order(struct page *page, struct list_head *list,
> > >>>>                          * page_deferred_list.
> > >>>>                          */
> > >>>>                         list_del_init(&folio->_deferred_list);
> > >>>> +                       __folio_clear_partially_mapped(folio);
> > >>>>                 }
> > >>>>                 spin_unlock(&ds_queue->split_queue_lock);
> > >>>>                 if (mapping) {
> > >>>> @@ -3454,11 +3455,13 @@ void __folio_undo_large_rmappable(struct folio *folio)
> > >>>>         if (!list_empty(&folio->_deferred_list)) {
> > >>>>                 ds_queue->split_queue_len--;
> > >>>>                 list_del_init(&folio->_deferred_list);
> > >>>> +               __folio_clear_partially_mapped(folio);
> > >>>
> > >>> is it possible to make things clearer by
> > >>>
> > >>>  if (folio_clear_partially_mapped)
> > >>>     __folio_clear_partially_mapped(folio);
> > >>>
> > >>> While writing without conditions isn't necessarily wrong, adding a condition
> > >>> will improve the readability of the code and enhance the clarity of my mTHP
> > >>> counters series. also help decrease smp cache sync if we can avoid
> > >>> unnecessary writing?
> > >>>
> > >>
> > >> Do you mean if(folio_test_partially_mapped(folio))?
> > >>
> > >> I don't like this idea. I think it makes the readability worse? If I was looking at if (test) -> clear for the first time, I would become confused why its being tested if its going to be clear at the end anyways?
> > >
> > > In the pmd-order case, the majority of folios are not partially mapped.
> > > Unconditional writes will trigger cache synchronization across all
> > > CPUs (related to the MESI protocol), making them more costly. By
> > > using conditional writes, such as "if(test) write," we can avoid
> > > most unnecessary writes, which is much more efficient. Additionally,
> > > we only need to manage nr_split_deferred when the condition
> > > is met. We are carefully evaluating all scenarios to determine
> > > if modifications to the partially_mapped flag are necessary.
> > >
> >
> >
> > Hmm okay, as you said its needed for nr_split_deferred anyways. Something like below is ok to fold in?
> >
> > commit 4ae9e2067346effd902b342296987b97dee29018 (HEAD)
> > Author: Usama Arif <usamaarif642@xxxxxxxxx>
> > Date:   Mon Aug 19 21:07:16 2024 +0100
> >
> >     mm: Introduce a pageflag for partially mapped folios fix
> >
> >     Test partially_mapped flag before clearing it. This should
> >     avoid unnecessary writes and will be needed in the nr_split_deferred
> >     series.
> >
> >     Signed-off-by: Usama Arif <usamaarif642@xxxxxxxxx>
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 5d67d3b3c1b2..ccde60aaaa0f 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -3479,7 +3479,8 @@ void __folio_undo_large_rmappable(struct folio *folio)
> >         if (!list_empty(&folio->_deferred_list)) {
> >                 ds_queue->split_queue_len--;
> >                 list_del_init(&folio->_deferred_list);
> > -               __folio_clear_partially_mapped(folio);
> > +               if (folio_test_partially_mapped(folio))
> > +                       __folio_clear_partially_mapped(folio);
> >         }
> >         spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
> >  }
> > @@ -3610,7 +3611,8 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
> >                 } else {
> >                         /* We lost race with folio_put() */
> >                         list_del_init(&folio->_deferred_list);
> > -                       __folio_clear_partially_mapped(folio);
> > +                       if (folio_test_partially_mapped(folio))
> > +                               __folio_clear_partially_mapped(folio);
> >                         ds_queue->split_queue_len--;
> >                 }
> >                 if (!--sc->nr_to_scan)
> >
>
> Do we also need if (folio_test_partially_mapped(folio)) in
> split_huge_page_to_list_to_order()?
>
> I recall that in Yu Zhao's TAO, there’s a chance of splitting (shattering)
> non-partially-mapped folios. To be future-proof, we might want to handle
> both cases equally.

we recall we also have a real case which can split entirely_mapped
folio:

mm: huge_memory: enable debugfs to split huge pages to any order
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fc4d182316bd5309b4066fd9ef21529ea397a7d4

>
> By the way, we might not need to clear the flag for a new folio. This differs
> from the init_list, which is necessary. If a new folio has the partially_mapped
> flag, it indicates that we failed to clear it when freeing the folio to
> the buddy system, which is a bug we need to fix in the free path.
>
> Thanks
> Barry





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux