linux-kernel - Re: [PATCH v3 4/6] mm: Introduce a pageflag for partially mapped folios

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fe79d6ca-6005-46d9-b14a-b785dd0b0414@redhat.com>
Date: Thu, 15 Aug 2024 23:08:38 +0200
From: David Hildenbrand <david@...hat.com>
To: Usama Arif <usamaarif642@...il.com>, akpm@...ux-foundation.org,
 linux-mm@...ck.org
Cc: hannes@...xchg.org, riel@...riel.com, shakeel.butt@...ux.dev,
 roman.gushchin@...ux.dev, yuzhao@...gle.com, baohua@...nel.org,
 ryan.roberts@....com, rppt@...nel.org, willy@...radead.org,
 cerasuolodomenico@...il.com, corbet@....net, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org, kernel-team@...a.com
Subject: Re: [PATCH v3 4/6] mm: Introduce a pageflag for partially mapped
 folios


> 
> Was there some comment here? I just see ">" remove from the start of /* We lost race with folio_put() */
> 

Likely I wanted to comment something but decided otherwise, sorry :)

>>> +            folio_clear_partially_mapped(folio);
>>>                ds_queue->split_queue_len--;
>>>            }
>>>            if (!--sc->nr_to_scan)
>>> @@ -3558,7 +3564,6 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
>>>    next:
>>>            folio_put(folio);
>>>        }
>>> -
>>>        spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
>>>        list_splice_tail(&list, &ds_queue->split_queue);
>>>        spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>> index 1fdd9eab240c..2ae2d9a18e40 100644
>>> --- a/mm/hugetlb.c
>>> +++ b/mm/hugetlb.c
>>> @@ -1758,6 +1758,7 @@ static void __update_and_free_hugetlb_folio(struct hstate *h,
>>>            free_gigantic_folio(folio, huge_page_order(h));
>>>        } else {
>>>            INIT_LIST_HEAD(&folio->_deferred_list);
>>> +        folio_clear_partially_mapped(folio);
>>>            folio_put(folio);
>>>        }
>>>    }
>>> diff --git a/mm/internal.h b/mm/internal.h
>>> index 52f7fc4e8ac3..d64546b8d377 100644
>>> --- a/mm/internal.h
>>> +++ b/mm/internal.h
>>> @@ -662,8 +662,10 @@ static inline void prep_compound_head(struct page *page, unsigned int order)
>>>        atomic_set(&folio->_entire_mapcount, -1);
>>>        atomic_set(&folio->_nr_pages_mapped, 0);
>>>        atomic_set(&folio->_pincount, 0);
>>> -    if (order > 1)
>>> +    if (order > 1) {
>>>            INIT_LIST_HEAD(&folio->_deferred_list);
>>> +        folio_clear_partially_mapped(folio);
>>
>> Can we use the non-atomic version here?
>>
> 
> I believe we can use the non-atomic version in all places where set/clear is done as all set/clear are protected by ds_queue->split_queue_lock. So basically could replace all folio_set/clear_partially_mapped with __folio_set/clear_partially_mapped.
> 
> But I guess its likely not going to make much difference? I will do it anyways in the next revision, rather than sending a fix patch. There haven't been any reviews for patch 5 so will wait a few days for any comments on that.

If we can avoid atomics, please do! :)

-- 
Cheers,

David / dhildenb