| 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
| ||
|
Message-ID: <9a58e794-2156-4a9f-a383-1cdfc07eee5e@gmail.com>
Date: Mon, 19 Aug 2024 16:16:07 -0400
From: Usama Arif <usamaarif642@...il.com>
To: Barry Song <baohua@...nel.org>
Cc: akpm@...ux-foundation.org, linux-mm@...ck.org, hannes@...xchg.org,
riel@...riel.com, shakeel.butt@...ux.dev, roman.gushchin@...ux.dev,
yuzhao@...gle.com, david@...hat.com, ryan.roberts@....com, rppt@...nel.org,
willy@...radead.org, cerasuolodomenico@...il.com, ryncsn@...il.com,
corbet@....net, linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
kernel-team@...a.com
Subject: Re: [PATCH v4 4/6] mm: Introduce a pageflag for partially mapped
folios
On 19/08/2024 20:00, Barry Song wrote:
> On Tue, Aug 20, 2024 at 2:17 AM Usama Arif <usamaarif642@...il.com> 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@...il.com> 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@...il.com>
>>>> ---
>>>> 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@...il.com>
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@...il.com>
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)
Powered by blists - more mailing lists