Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 29 Sep 2022 18:49:20 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Alistair Popple <apopple@nvidia.com>,
        Dan Williams <dan.j.williams@intel.com>
CC:     Jason Gunthorpe <jgg@nvidia.com>, <linux-mm@kvack.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Michael Ellerman <mpe@ellerman.id.au>,
        "Nicholas Piggin" <npiggin@gmail.com>,
        Felix Kuehling <Felix.Kuehling@amd.com>,
        "Alex Deucher" <alexander.deucher@amd.com>,
        Christian =?iso-8859-1?Q?K=F6nig?= <christian.koenig@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Ben Skeggs <bskeggs@redhat.com>,
        Karol Herbst <kherbst@redhat.com>,
        Lyude Paul <lyude@redhat.com>,
        Ralph Campbell <rcampbell@nvidia.com>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Alex Sierra <alex.sierra@amd.com>,
        "John Hubbard" <jhubbard@nvidia.com>,
        <linuxppc-dev@lists.ozlabs.org>, <linux-kernel@vger.kernel.org>,
        <amd-gfx@lists.freedesktop.org>, <nouveau@lists.freedesktop.org>,
        <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 2/7] mm: Free device private pages have zero refcount
Message-ID: <63364b20ee66d_7390294a1@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <cover.f15b25597fc3afd45b144df863eeca3b2c13f9f4.1664171943.git-series.apopple@nvidia.com>
 <3d74bb439723c7e46cbe47d1711795308aee4ae3.1664171943.git-series.apopple@nvidia.com>
 <YzG42766BJSxro0R@nvidia.com>
 <877d1plfrm.fsf@nvdebian.thelocal>
 <6335fd87adc7f_f6c9d29474@dwillia2-xfh.jf.intel.com.notmuch>
 <87k05llly0.fsf@nvdebian.thelocal>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <87k05llly0.fsf@nvdebian.thelocal>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?0M0H5Ibl78YFwgxHjkTJVmCguTih14uLQrWTIWhrT+iGddLvNYacOj2JkTWI?=
 =?us-ascii?Q?XQ//g2fx+sqA5vG7kESi54ep66pTkSkiL5yX2lqME1x8UxbARTm4h3g8Ml/z?=
 =?us-ascii?Q?m9xjRsuZtGMaQ12A9mjG6l3oeMSQi54hzyMYfj+Isno8Wce0aM93crgbBoN6?=
 =?us-ascii?Q?Ct7YAjM79YE+E9yzxW3C+XRj2wPGxIaOCXXsbBjCcrUNurb3A65VpKfUZl/V?=
 =?us-ascii?Q?Vex4PmlCo41oG+oiza48m78Ut7jJaK2QUabFFSKw77qOgznq9Fs3a6pKoIIh?=
 =?us-ascii?Q?apvx7LZDIp3PPDzXacG+/XTd3/iU5WqnsPl0duHkE+JgeR69bk2cC5IpgG98?=
 =?us-ascii?Q?rS2BkDEhlKPOer6zO6ULUCBTDSrsySxGp7j/ITFnH+nUhWIJ6fuW45lgO/O/?=
 =?us-ascii?Q?bxYYNnAV9bLi2dKVueMOBsit+DJ3eeKsZcceMkQt/JKW/vLUNc4JprlQQs5B?=
 =?us-ascii?Q?PyTpbP38MsEA5s7+EBxExRpq45ZoX4AgXDU7ORgpYj25Vd2e4lGcDnYVWtPZ?=
 =?us-ascii?Q?G/aWCqspsOs7Di79c0zB5BR47rqLQ6vFr0Wryk3AIgk/aFhnv4bDU3KdvGDZ?=
 =?us-ascii?Q?ZMHknHJ0wRPlTIzaj05QNyrF5xlu/1768EbS4bUOAiJtcOcCO99ba40NjOTo?=
 =?us-ascii?Q?/HwVTNgSuzvVBl0Au3v65CuhTGZFU47rQePtIJt/STUxgsiyS843hft0IS3P?=
 =?us-ascii?Q?nRupraFXMMwr/zlEoYJYE6IjG9M+tC2SRqoJ9tq3luic/MMtSncDvrbwrx5H?=
 =?us-ascii?Q?b8Z8oL5FyGcQP+zdqAMJnf3Wbd71zZdiIBOaJP5Y3zXEtnwnzSZHqJCijePU?=
 =?us-ascii?Q?/K1DdFkEbD43zAzs+Evw6oPosmLFG/CQJ6nGNg6ppRpA3uglbo6+uUy61e9e?=
 =?us-ascii?Q?bhqIVL8/0L6pwIwBHSDtqzEuvHWAofgZdKgFYmBSOOjepH/spS1eoo1vFOqf?=
 =?us-ascii?Q?6EZhziPRDpTDTOmTUJH0K339qksyxsui/I8kp2EKLqVmhB+VYOocWHfJjYoT?=
 =?us-ascii?Q?PbDWVrpKYUQs6eYTRgHSw9W/7zyVT2Xfr1TKutt4B8PKd43Z0wS5jUHA/oHi?=
 =?us-ascii?Q?CykzPRauGiS5Z7T6K1969dKMGJmTl2BjoVGH60Z+LymrnQDTQdEQbc/9mzu/?=
 =?us-ascii?Q?O6MV3Q0KtTpJrt+FUmpuuGOyIpiL8KEHAiZpIsu0w8szCXEPdKS93CRaAQzF?=
 =?us-ascii?Q?S2L2w4+OYf4Isx8YGN/tXfeKO2l8PcCodAcUmo0SQodnWtR4BfuXXkcGSr5m?=
 =?us-ascii?Q?mjjdGfoHb9Pcsaki85xJCV9d+Oh4jHONC/WUv5gsIFnT1fkH6+cZaEal7/QC?=
 =?us-ascii?Q?Pbauxky39BxPsNIex8he8wVLxxABTdpYGd6nRmV6ouqBV8OyQmAGc2TUTKeO?=
 =?us-ascii?Q?EL5daB9PGXd8IwCJ6qGDOPbSkndTiBkhmg4uoFPMrJNrTrP8FlnVxubo18//?=
 =?us-ascii?Q?sm1Ayh5XRcRjaeGbQKAh3JMz1qL168WvDDHqg889eZYBwAV43nM/6tNzx6Do?=
 =?us-ascii?Q?DwhAgcVgCAVK7TrJqciEwLTtL+M0+PyubzCdJrUbB4xNGSKLg7/qhv6rQdHY?=
 =?us-ascii?Q?mZ8J/aEu09t53Eutuig9uaz/gkaqTI9yz4sTTzExE61exHPzEaA5R5RzvEVU?=
 =?us-ascii?Q?2g=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 200e443b-4afa-4160-d52f-08daa2860071
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Sep 2022 01:49:24.5155
 (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: Il8CfWe8Zs6X5xU0+zppk5yiuvhekafKsJzcDqSiC6QKpf3+uZGSygvAdf59wkFE28rVRzycSfIL473+ul+XPXz7RNcaVaTfCA904MPEUjc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5877
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 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-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Alistair Popple wrote:
> 
> Dan Williams <dan.j.williams@intel.com> writes:
> 
> > Alistair Popple wrote:
> >>
> >> Jason Gunthorpe <jgg@nvidia.com> writes:
> >>
> >> > On Mon, Sep 26, 2022 at 04:03:06PM +1000, Alistair Popple wrote:
> >> >> Since 27674ef6c73f ("mm: remove the extra ZONE_DEVICE struct page
> >> >> refcount") device private pages have no longer had an extra reference
> >> >> count when the page is in use. However before handing them back to the
> >> >> owning device driver we add an extra reference count such that free
> >> >> pages have a reference count of one.
> >> >>
> >> >> This makes it difficult to tell if a page is free or not because both
> >> >> free and in use pages will have a non-zero refcount. Instead we should
> >> >> return pages to the drivers page allocator with a zero reference count.
> >> >> Kernel code can then safely use kernel functions such as
> >> >> get_page_unless_zero().
> >> >>
> >> >> Signed-off-by: Alistair Popple <apopple@nvidia.com>
> >> >> ---
> >> >>  arch/powerpc/kvm/book3s_hv_uvmem.c       | 1 +
> >> >>  drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 1 +
> >> >>  drivers/gpu/drm/nouveau/nouveau_dmem.c   | 1 +
> >> >>  lib/test_hmm.c                           | 1 +
> >> >>  mm/memremap.c                            | 5 -----
> >> >>  mm/page_alloc.c                          | 6 ++++++
> >> >>  6 files changed, 10 insertions(+), 5 deletions(-)
> >> >
> >> > I think this is a great idea, but I'm surprised no dax stuff is
> >> > touched here?
> >>
> >> free_zone_device_page() shouldn't be called for pgmap->type ==
> >> MEMORY_DEVICE_FS_DAX so I don't think we should have to worry about DAX
> >> there. Except that the folio code looks like it might have introduced a
> >> bug. AFAICT put_page() always calls
> >> put_devmap_managed_page(&folio->page) but folio_put() does not (although
> >> folios_put() does!). So it seems folio_put() won't end up calling
> >> __put_devmap_managed_page_refs() as I think it should.
> >>
> >> I think you're right about the change to __init_zone_device_page() - I
> >> should limit it to DEVICE_PRIVATE/COHERENT pages only. But I need to
> >> look at Dan's patch series more closely as I suspect it might be better
> >> to rebase this patch on top of that.
> >
> > Apologies for the delay I was travelling the past few days. Yes, I think
> > this patch slots in nicely to avoid the introduction of an init_mode
> > [1]:
> >
> > https://lore.kernel.org/nvdimm/166329940343.2786261.6047770378829215962.stgit@dwillia2-xfh.jf.intel.com/
> >
> > Mind if I steal it into my series?
> 
> No problem, although I notice Andrew has already merged it into
> mm-unstable. If you end up rebasing your series on top of mine I think
> all that's needed is a patch somewhere in your series to drop the
> various `if (pgmap->type == MEMORY_DEVICE_*)` I added to (hopefully)
> avoid breaking DAX. Assuming DAX takes a pagemap reference on struct
> page allocation something like below.

Yeah, I'll go that route and rebase on top of -mm.

Thanks again.