From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 843ACC48BF6 for ; Thu, 7 Mar 2024 04:31:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F336C6B00FC; Wed, 6 Mar 2024 23:31:31 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id EE35C6B00FD; Wed, 6 Mar 2024 23:31:31 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DD2D46B00FE; Wed, 6 Mar 2024 23:31:31 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id CEC1E6B00FC for ; Wed, 6 Mar 2024 23:31:31 -0500 (EST) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 78A3E8067E for ; Thu, 7 Mar 2024 04:31:31 +0000 (UTC) X-FDA: 81868969182.16.A4ADDEB Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf28.hostedemail.com (Postfix) with ESMTP id 584EFC001A for ; Thu, 7 Mar 2024 04:31:29 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=FVre6TLR; spf=none (imf28.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1709785890; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=eEIcy2AuLFdeEvb7tZweH/HoBCe3v2yh7u2dvfEkbNs=; b=I7BCx27Hh9xa+rxtuRNjI3djSGMdhmZXfEUsjIAdrbPSBEeMf5IHQlvI6+F6CzzbhaAr8r UFawswzT69ybrjMY9FUc9or73DwOXo9NIQKIBLXsAXl63aX56rj4Uy+CPD/hn87pXlUhtt xf0hsaW7lZXqBSJcYqK8KAIJ70lYZTg= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1709785890; a=rsa-sha256; cv=none; b=skBxT6X5N/2dZGfUJo2gehoCoLRD4nfutpo/OdyIT/UV6AyffAKLertU/vVPsDoHcTTAXV cvqfPyER6DluqcJLUkLrMfg0GDB4vaDCdtDet7JE/c1lUcIteX0YYzSvglzGXY3zirInTy mPNT7uyntuSipyMA5GlHr+MF9rCT1c4= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=FVre6TLR; spf=none (imf28.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org; dmarc=none DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=eEIcy2AuLFdeEvb7tZweH/HoBCe3v2yh7u2dvfEkbNs=; b=FVre6TLROy/Ib5OIEMEsZ6NqTS DtIWl1YRhFuhPgeeDEy3vVorAy+AxH1WmhMWvQSPWncqvcjQTa7Fy1oqmEDuyMjGumGtFu9MjL/99 uwwDun6OAHypI2ktyxfvOLVy5K4/2Ck1TbhHU8u8G6wEDovtp9wxK4Ala5OklyueMAOnxUQ74imgq YZoOkzVVowt/ddZARhq7QOwJ5eZZTHJiDm6K0t4/z2OAFtefZ8hk+dnhjfUANlFnBu1tB39jEWAYY eNBn6aqPYUXkzHQjGQM8p/11MbosyznD3X1iVZ7OWjkpE+So1+oVZ8bNxroYP+MUKC+ydXT8Tsk/T QOgJJ39g==; Received: from willy by casper.infradead.org with local (Exim 4.97.1 #2 (Red Hat Linux)) id 1ri5Pj-00000008LiE-0F4v; Thu, 07 Mar 2024 04:31:27 +0000 Date: Thu, 7 Mar 2024 04:31:26 +0000 From: Matthew Wilcox To: David Hildenbrand Cc: linux-mm@kvack.org, Oscar Salvador Subject: Re: [PATCH 0/5] Remove some races around folio_test_hugetlb Message-ID: References: <20240301214712.2853147-1-willy@infradead.org> <52599fd8-76dc-4d8f-b9f2-78146fc7a518@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <52599fd8-76dc-4d8f-b9f2-78146fc7a518@redhat.com> X-Stat-Signature: cscim13s58pt4urjqswg3o51tfkm5t1d X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 584EFC001A X-Rspam-User: X-HE-Tag: 1709785889-83669 X-HE-Meta: U2FsdGVkX189YScVcRSITW109ByMYP4ircNGYxRbTHhXOMlfU+/5wh3I/U8tmUGvwipXRs9irk+muloJNFOVYJzX9qxV5jBCew4v/sF9Ru2NQ6BvigsAZDtGaQ8UgXgMOe9pAq+AZnz1InG1FkMN5u/5lheip03OJXthGKaMhyTyosF7YTlQszZS+nTiZS4lHpP0d44eoQHxn/BCDsVVJ/P3rhFi5CRhif9YO3yzwIbZvW2JmxyhTnpRnLVHvRWbPn4vhe1j+RAcLU6tr3xWicXONf56pdpe0RpsUk+JYIfPb0xQANOpmyenVgjTMX7g4vHUTagE4sh2LDFG7MQNYnHadkT1D0wMA2wMRUD/QWFGvH/fnlYA8/cAJly2VVIJdMCplmq8coktfY+H2Tw0TTNZLfKM/rcWTGauN2+OosuZyXw1sLNi+r/rsfsk+0JvyWfiP0Rcpb3Q4VbwEBKPwhqUVgXqqtD53IO80bCbQVttGM5hdHX9mFh5iawRq1BeLTZp34CbUEvnXw+t50pDhDA3v2zLkuLQekl3+4Ua9jc7C89LAW663q1eJkhg4yLMnKlFvJ0JkN11INwRoLagB/0xFHZEs2/vApSGaib7znkNdcvL/ldDC1jB/i/KE7jW3mh+0EeWJ2w2JnrOhQ7AAQId7lJlKBGkzvqBhFmDFeY7CAmOFNJX8XItA4QRSd0+Kbrreq1Gk9w58zFe7waD6Uoc7vXkzzNLT0IxZaFyKP0syrl4axj6Ap9WMkKF5rth6Rb4S0mbkTzQ649vfU1qtY/UtdlaLUs8OIC0xDF08dZMueMKlbppdr//hAEi9C7HGs01bEKsO7KfhDct9Zzpq6gp1/0wZw/8LLoCsxm2UV+Ov4XGTzJ+zH5uF6S0OBDCpACo652ldVVTZG6mltxRjgj0FEdwD3FITX1rI+AqqrCLiRejMDqHNMY682KVfCdTcSnBVyX+GMS1MSjzLGE BJKIi10v Jj3ZVXHOjNQn/tYERDd8YM6/cnKWt/TzmyYPyuql9EierhrGU0bVtFSUL+Dlsj6O3jddC1uVzSyQ0mNpZ9SBB0b3tkl0o+3Sf6wpsiDnqEYXc6YVTKZzpYO76GJKOs2iKU/KWBetIM0+YMKAEOvqFFcH2bY9cIdij08BJVxhGl4JZcKq2UdFg8IT/iw== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Wed, Mar 06, 2024 at 04:18:45PM +0100, David Hildenbrand wrote: > On 05.03.24 21:35, Matthew Wilcox wrote: > > On Tue, Mar 05, 2024 at 10:10:08AM +0100, David Hildenbrand wrote: > > > > The cost of this reliability is that we now consume the word I recently > > > > freed in folio->page[1]. I think this is acceptable; we've still gained > > > > a completely reliable folio_test_hugetlb() (which we didn't have before > > > > I started messing around with the folio dtors). Non-hugetlb users > > > > can use large_id as a pointer to something else entirely, or even as a > > > > non-pointer, as long as they can guarantee it can't conflict (ie don't > > > > use it as a bitfield). > > > > > > That probably means that we have to always set the lowest bit to use it for > > > something else, or use another bit. > > > > Yes, that would work. > > > > > I was wondering if > > > > > > a) We could move that to another subpage. In hugetlb folios we have plenty > > > of space for such things. I guess we'd have be able to detect the folio size > > > without holding a reference, to make sure we can touch another subpage. > > > > Yes, that was my concern. I wanted to put it in page[2] with all the > > other hugetlb goop, but I got to thinking about an order-1 compound > > page allocated at the end of memmap and got scared. We could make > > folio_test_hugetlb() look at ->flags for the head bit, then look at > > ->flags_1 for the order and finally at ->hugetlb_id, but now we've looked > > at three cachelines to answer a fairly frequent question. And then what > > if the folio got split between looking at ->flags and ->flags_1 and we > > get a bogus folio order that makes it look OK? We can't even look at > > ->flags, ->flags_1 and recheck ->flags because it might have got split, > > freed and reallocated in the meantime. > > > > > b) We could overload _nr_pages_mapped. We'd effectively have to steal one > > > bit from _nr_pages_mapped to make this work. > > > > > > Maybe what works is using the existing mechanism (hugetlb flag), and then > > > storing the pointer in __nr_pages_mapped. > > > > > > So depending on the hugetlb flag, we can interpret __nr_pages_mapped either > > > as the pointer or as the old variant. > > > > > > Mostly only folio_large_is_mapped() would need care for now, to ignore > > > _nr_pages_mapped if the hugetlb flag is set. > > > > I don't mind that at all. We wouldn't even need to steal a bit or use the > > existing flag; we could just say that -2 means this is a hugetlb folio. > > As long as it ends up at the same offset as page->mapping (because that's > > always NULL or a pointer possibly with a low bit set so can't ever be a > > number between -4095 and -1). > > Would hugetlb_id below be 32bit or 64bit on 64-bit? 64-bit, so when it's reused by page->mapping after a split, it isn't ambiguous. > > > > IOW: > > > > word page0 page1 > > 0 flags flags > > 1 lru.next head > > 2 lru.prev entire_mapcount + gap > > 3 mapping nr_pages_mapped + gap / hugetlb_id > > 4 index pincount + nr_pages > > 5 private unused > > 6 mapcount+refcount mapcount+refcount(0) > > 7 memcg_data - > > > > or on 32-bit > > > > word page0 page1 > > 0 flags flags > > 1 lru.next head > > 2 lru.prev entire_mapcount > > 3 mapping nr_pages_mapped / hugetlb_id > > ^ In the worst case, I think, nr_pages_mapped with a lot of entire mappings > could end up matching hugetlb_id. We add a large value to nr_pages_mapped > every time we add an entire mapping ... (not sure if that could currently be > a problem with many entire mappings of a large folio) My understanding was that nr_pages_mapped was incremented by one for each page which has a non-zero mapcount. It was also incremented by ENTIRELY_MAPPED the first time that we increment ->entire_mapcount. As such, I don't think entire_mapcount can get the top bit set. > > > 4 index pincount > > 5 private unused > > 6 mapcount mapcount > > 7 refcount refcount > > 8 memcg_data - > > 9+ virtual? last_cpupid? whatever > > > > Does this fit with your plans? > > For the total mapcount this would do (and it would be better), but the > layout gets a bit "sparse" on 64bit that way, which will end up being > problematic for some other stuff I might want to put in there. > > Not that we have to resolve that now, just bringing it up, that maybe we can > do better right away :) How about this layout? @@ -350,8 +350,13 @@ struct folio { unsigned long _head_1; unsigned long _folio_avail; /* public: */ - atomic_t _entire_mapcount; - atomic_t _nr_pages_mapped; + union { + unsigned long _hugetlb_id; + struct { + atomic_t _entire_mapcount; + atomic_t _nr_pages_mapped; + }; + }; atomic_t _pincount; #ifdef CONFIG_64BIT unsigned int _folio_nr_pages; That keeps _folio_avail as, well, available. It puts _hugetlb_id in the same bits as ->mapping. It continues to leave ->private unused on 64-bit. I think this does everything we want?