Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 21 Sep 2022 17:34:22 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Jason Gunthorpe <jgg@nvidia.com>,
        Dan Williams <dan.j.williams@intel.com>
CC:     Alistair Popple <apopple@nvidia.com>, <akpm@linux-foundation.org>,
        "Matthew Wilcox" <willy@infradead.org>, Jan Kara <jack@suse.cz>,
        "Darrick J. Wong" <djwong@kernel.org>,
        Christoph Hellwig <hch@lst.de>,
        John Hubbard <jhubbard@nvidia.com>,
        <linux-fsdevel@vger.kernel.org>, <nvdimm@lists.linux.dev>,
        <linux-xfs@vger.kernel.org>, <linux-mm@kvack.org>,
        <linux-ext4@vger.kernel.org>
Subject: Re: [PATCH v2 16/18] mm/memremap_pages: Support initializing pages
 to a zero reference count
Message-ID: <632bad8e685d5_349629438@dwillia2-xfh.jf.intel.com.notmuch>
References: <166329930818.2786261.6086109734008025807.stgit@dwillia2-xfh.jf.intel.com>
 <166329940343.2786261.6047770378829215962.stgit@dwillia2-xfh.jf.intel.com>
 <YyssywF6HmZrfqhD@nvidia.com>
 <632ba212e32bb_349629451@dwillia2-xfh.jf.intel.com.notmuch>
 <Yyuml1tSKPmvLS6P@nvidia.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Yyuml1tSKPmvLS6P@nvidia.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?NLPsHmjAExeu0mv/ONFYGsxRjgsM9DMG0CjApK/L6enidYGZrxcukERidyv4?=
 =?us-ascii?Q?evlXMjkHOGXq+AsnPBfIjJR2rIqB8fulySNoxlKKUqZQgpUb7w+sk0rBm00U?=
 =?us-ascii?Q?iTKQuPSzFYuiNrSrzQV/Bgvhqg+kUuHWpxXQKBGGvDEAJ4MBGsHCpN96Vnyy?=
 =?us-ascii?Q?RtcC/NOfwbGeQFwJdQnLDaNkFoZtk0ksBdtHYs0uDw6MH145XPdE11A1lqrB?=
 =?us-ascii?Q?ScffRZ8lcO2wGFEM8NXXnv9sNZ6rbPpWFNYfMAUQV6cIYw2VK2q87MYZ0Y0C?=
 =?us-ascii?Q?eqgK41goUJ0RTfwWgK+5pTq2tcPAnCVmoX6oq+P/RFBcJocyifXyz23BIULn?=
 =?us-ascii?Q?Bvj+NDV8Qux6edbPF/TbyY27gfysblwUZn+rXOboK4YqB9JLfVLMSLVpbzVW?=
 =?us-ascii?Q?kjRHiilKUCT0+G3+o+GWUYK6gG2NgMFTXRTN1Y4ZB+6zPHQ/6wJTuoeSsKaz?=
 =?us-ascii?Q?3+dZDeSwI9gDyBERz9XSqfDcxc6ktwAk68uN8zVSVJIejKnzNOSDxFPw+RCg?=
 =?us-ascii?Q?/Ey+fZ4SRJtXHnbw/DhyoqiaCy37nFJec0AyLVv2p5jsavaggYsgCm9KZtOH?=
 =?us-ascii?Q?vF0wJE9DkXS5hGZRSDdT3bhs+qYySeEw89ufz7+BxzIoX1Ua5vozYXyUcz92?=
 =?us-ascii?Q?bTOSa3UwMAHJaH59xw4JBFHt2T8KZa50aOJj0Gv1PMxwFAZF4YBVFjsj/xRM?=
 =?us-ascii?Q?6HARa76hraC+fvpRe7o6gpyytMsmQg3LlG9nMeXvVoiDxhiRwY3F9pRrerHi?=
 =?us-ascii?Q?lQ/lqd0f/v29NEDrJPem9AZuz8k4gi/zQVQh4tQjEMhIaFcxiHEpYwumhodQ?=
 =?us-ascii?Q?Evjkq6fPSnY66SHL4qK1bNak18M3voDrtZPg4ioNgAAKA6+0iecuI66txyAl?=
 =?us-ascii?Q?hcewVESwriLAv+rf+E/UwSSxgjd58Yeprd+cnHVaPpc1EaYPtU/l0buiz6IB?=
 =?us-ascii?Q?g+PnTP54yWxJ1Z+lxztHb5Nkr/HwUp9RnSuZ7Pcaqn00Jz7zTBVrAPhDqBwB?=
 =?us-ascii?Q?c8k7iT3O4ex2plePhZjXiCqeCEeFCpNn3GyGSTtEmcpd3mhfNRtpuuB/ibAp?=
 =?us-ascii?Q?f0PbWXsMeQkjI1I1hGhBLvyNYafMlwkXtKJ2IM63vYjLf3M5i8+z/z+Au9zd?=
 =?us-ascii?Q?iKIZov3rrR32tObafBi9V45R6/FWwNFUJYk5e4s3gS1AgWhvDnZUDFRAxDid?=
 =?us-ascii?Q?MNk3KelY3aFTYpZhOYKWiLkCIRctG5OBsBGdyF8xHKqJSeQnI5lWtnQ9+2iX?=
 =?us-ascii?Q?K+svA8RL3k9K68BUlLTwC7W83qWJ/4LAnh6xrzqQAuHiycXew3Oyh8iLoNWr?=
 =?us-ascii?Q?TrXp1H3sZWvS2dmuasx5kSs1N7N9IuSGzi0c4Szem8dgvVM1kvA6kHXpnyMl?=
 =?us-ascii?Q?lW5c+dvOZZzLmBlh5DtwfgT8kAVU/p/4EABToSTv5O94Gxbb+DnvJ2k1NKS/?=
 =?us-ascii?Q?UBFRtMej0U67qmKEF/NcF5D4zKuo1j6smKXWRiD+aOg/qexli24qogVRQIcW?=
 =?us-ascii?Q?IkpVEvUe56krCb733cahPWsHsorHGKLQs8c4oQvJg+MtSD7CVIg2POuC/b3C?=
 =?us-ascii?Q?02ArQbBxrAjxfNhjwJrKQU2EfbHN+XouhuUPg3NjD4Fe65bh0Ba/67fNZnDj?=
 =?us-ascii?Q?OA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 13b73711-d72a-4161-61b7-08da9c323368
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Sep 2022 00:34:25.2997
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: weFQmGvlASfUMRaWZf5GSn/pH5N44GRni/FqPTJB+/eJrLZw0eW9JeVSVzXxEVm6k3up+6Hq/EDRa4eCfXdwyUUigILDPrX4olLjoKzo6s0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5593
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org

Jason Gunthorpe wrote:
> On Wed, Sep 21, 2022 at 04:45:22PM -0700, Dan Williams wrote:
> > Jason Gunthorpe wrote:
> > > On Thu, Sep 15, 2022 at 08:36:43PM -0700, Dan Williams wrote:
> > > > The initial memremap_pages() implementation inherited the
> > > > __init_single_page() default of pages starting life with an elevated
> > > > reference count. This originally allowed for the page->pgmap pointer to
> > > > alias with the storage for page->lru since a page was only allowed to be
> > > > on an lru list when its reference count was zero.
> > > > 
> > > > Since then, 'struct page' definition cleanups have arranged for
> > > > dedicated space for the ZONE_DEVICE page metadata, and the
> > > > MEMORY_DEVICE_{PRIVATE,COHERENT} work has arranged for the 1 -> 0
> > > > page->_refcount transition to route the page to free_zone_device_page()
> > > > and not the core-mm page-free. With those cleanups in place and with
> > > > filesystem-dax and device-dax now converted to take and drop references
> > > > at map and truncate time, it is possible to start MEMORY_DEVICE_FS_DAX
> > > > and MEMORY_DEVICE_GENERIC reference counts at 0.
> > > > 
> > > > MEMORY_DEVICE_{PRIVATE,COHERENT} still expect that their ZONE_DEVICE
> > > > pages start life at _refcount 1, so make that the default if
> > > > pgmap->init_mode is left at zero.
> > > 
> > > I'm shocked to read this - how does it make any sense?
> > 
> > I think what happened is that since memremap_pages() historically
> > produced pages with an elevated reference count that GPU drivers skipped
> > taking a reference on first allocation and just passed along an elevated
> > reference count page to the first user.
> > 
> > So either we keep that assumption or update all users to be prepared for
> > idle pages coming out of memremap_pages().
> > 
> > This is all in reaction to the "set_page_count(page, 1);" in
> > free_zone_device_page(). Which I am happy to get rid of but need from
> > help from MEMORY_DEVICE_{PRIVATE,COHERENT} folks to react to
> > memremap_pages() starting all pages at reference count 0.
> 
> But, but this is all racy, it can't do this:
> 
> +	if (pgmap->ops && pgmap->ops->page_free)
> +		pgmap->ops->page_free(page);
>  
>  	/*
> +	 * Reset the page count to the @init_mode value to prepare for
> +	 * handing out the page again.
>  	 */
> +	if (pgmap->init_mode == INIT_PAGEMAP_BUSY)
> +		set_page_count(page, 1);
> 
> after the fact! Something like that hmm_test has already threaded the
> "freed" page into the free list via ops->page_free(), it can't have a
> 0 ref count and be on the free list, even temporarily :(
> 
> Maybe it nees to be re-ordered?
> 
> > > How on earth can a free'd page have both a 0 and 1 refcount??
> > 
> > This is residual wonkiness from memremap_pages() handing out pages with
> > elevated reference counts at the outset.
> 
> I think the answer to my question is the above troubled code where we
> still set the page refcount back to 1 even in the page_free path, so
> there is some consistency "a freed paged may have a refcount of 1" for
> the driver.
> 
> So, I guess this patch makes sense but I would put more noise around
> INIT_PAGEMAP_BUSY (eg annotate every driver that is using it with the
> explicit constant) and alert people that they need to fix their stuff
> to get rid of it.

Sounds reasonable.

> We should definately try to fix hmm_test as well so people have a good
> reference code to follow in fixing the other drivers :(

Oh, that's a good idea. I can probably fix that up and leave it to the
GPU driver folks to catch up with that example so we can kill off
INIT_PAGEMAP_BUSY.