Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 1 Sep 2021 13:16:13 -0300
From:   Jason Gunthorpe <jgg@nvidia.com>
To:     David Hildenbrand <david@redhat.com>
Cc:     Qi Zheng <zhengqi.arch@bytedance.com>, akpm@linux-foundation.org,
        tglx@linutronix.de, hannes@cmpxchg.org, mhocko@kernel.org,
        vdavydov.dev@gmail.com, kirill.shutemov@linux.intel.com,
        mika.penttila@nextfour.com, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        songmuchun@bytedance.com
Subject: Re: [PATCH v2 6/9] mm: free user PTE page table pages
Message-ID: <20210901161613.GN1721383@nvidia.com>
References: <20210819031858.98043-1-zhengqi.arch@bytedance.com>
 <20210819031858.98043-7-zhengqi.arch@bytedance.com>
 <20210901135314.GA1859446@nvidia.com>
 <0c9766c9-6e8b-5445-83dc-9f2b71a76b4f@redhat.com>
 <20210901153247.GJ1721383@nvidia.com>
 <7789261d-6a64-c47b-be6c-c9be680e5d33@redhat.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7789261d-6a64-c47b-be6c-c9be680e5d33@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?DGJNIS76YpNdAeyORJYa6MYth6x3U8pD9ISJuCXxcCElwp/098GRQ5BLarNa?=
 =?us-ascii?Q?P8qkYudMz676xAmwnaZMOmM/ebJPog0hunm/JK+4MYbYogLPa1lJ+1PkylPa?=
 =?us-ascii?Q?4nMSl1wE0bCJPFu0ZPny0a7oYMSWH8fZxqonVxXIXJkgl/J7yF5OIQgeZG7Q?=
 =?us-ascii?Q?EXMSbo6f7VJKXxHYmGXcSUbVHBULQONWPhYk5dFPQvztW5FlC5ArEg53uT1A?=
 =?us-ascii?Q?58qAE5FyyYD3/fhwZJGRhOcaH5T5IBRdGMqa6C3Icg1g0POKnvKWtscvQLMw?=
 =?us-ascii?Q?PDLPydz8WE/vXcjYKkktWaaJeLsAbpPAzb5iuy3nRjeiPoqvBNp5bsvA567Z?=
 =?us-ascii?Q?211VcmSPwN0yH3rC8zo2hI6WZ6YGdVHnkDQYHMbKSvwyFnw0+FLXyQpf9cmO?=
 =?us-ascii?Q?TPxslIMPs8KghOQ66Sfob0KDOzA5JkQ2qcnSh+cBHJ/cD1Srfay6R74A2FEv?=
 =?us-ascii?Q?W48a9Qjj3hn9cvvT6xWauqSX07fwP71n2CRr3u2vXjL/56WUcsuvtcXLAqkd?=
 =?us-ascii?Q?sZwmAM1wZngcWZ1EOPm/zWdimqP0mGJCQaaezojO6zxwWwxnvoBa5DRcj0xZ?=
 =?us-ascii?Q?UpuWs/BfLCqdIbEjI8g/gimI7wfkaCiObGFUdCXx5x8d+G3/SaHkt/FSSA9f?=
 =?us-ascii?Q?a9TaoIWizIjGk3c562MSygjYPGHKCjUPwbt6mV/IQeT9biwfzje+jCeYykDD?=
 =?us-ascii?Q?29KKUu++/iXWoJB2w2gL20/UQtDGYN0fF2Mj1frQaaJi38+tOYU/cUpvu66n?=
 =?us-ascii?Q?Dvn/BLPAi9GsLsm9Y93gyBRfSb/5DvS1k1HdQgIDvfEgUey7bqY0sh8Fu+L3?=
 =?us-ascii?Q?eIIpWuUXRU/kHoxZj239JvAGDtHD9vmgBa/uQjdqH6Y6Ng9bWZBj+n1cmez2?=
 =?us-ascii?Q?Diq7ONjdL/mKOb8gu/vcXsI1mfo2/WJSMf2MRdpuZlb65gR7uljLbUh8xo0J?=
 =?us-ascii?Q?ETKPl8e0+L5bcUfoBI5IFLbXvfC+/YNQzQonpocX2MpiMJ4JYc/XeOeH9va7?=
 =?us-ascii?Q?U19LD6QpdCfM6UycMjZ2+eET94EVyXodH9XKbraTJuhyX7VIuqi8x9GCEhoR?=
 =?us-ascii?Q?gZRTRP4OJMOko/dZRlJc/QBUTtIuXU4wyD/T6gLF4YuEDpFZ5sy2fQlKeaQn?=
 =?us-ascii?Q?V8OEkGm2fBla7LK84iSHj01AYFH4xtYe2KBHNX0OQk9PLL4lyeOIDa4zNaoA?=
 =?us-ascii?Q?ZNxO39v85v3KqGbXj1yoFYNK82gDD5a9MuoKjN3pvYMCbkSzHvusJJip4C1/?=
 =?us-ascii?Q?LL0CJM6GXpTMAEvCrSGiJhMEg6zyc3GZo9i5wAj6ncyg8O5cuH4nLrDiyehc?=
 =?us-ascii?Q?MUHak0hLc4z8xFMdZGgQC/s2?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0f40dc5f-c715-4460-4124-08d96d63d1f6
X-MS-Exchange-CrossTenant-AuthSource: BL0PR12MB5506.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Sep 2021 16:16:14.3204
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: RKulUH2w9iOZlTQecMoNswxCEWaKJCIwXe8bMjglPXW6FWLHtavBcjFGYpids0ok
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR12MB5352
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Sep 01, 2021 at 06:13:07PM +0200, David Hildenbrand wrote:
> On 01.09.21 17:32, Jason Gunthorpe wrote:
> > On Wed, Sep 01, 2021 at 03:57:09PM +0200, David Hildenbrand wrote:
> > > On 01.09.21 15:53, Jason Gunthorpe wrote:
> > > > On Thu, Aug 19, 2021 at 11:18:55AM +0800, Qi Zheng wrote:
> > > > 
> > > > > diff --git a/mm/gup.c b/mm/gup.c
> > > > > index 2630ed1bb4f4..30757f3b176c 100644
> > > > > +++ b/mm/gup.c
> > > > > @@ -500,6 +500,9 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
> > > > >    	if (unlikely(pmd_bad(*pmd)))
> > > > >    		return no_page_table(vma, flags);
> > > > > +	if (!pte_try_get(mm, pmd))
> > > > > +		return no_page_table(vma, flags);
> > > > > +
> > > > >    	ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
> > > > 
> > > > This is not good on a performance path, the pte_try_get() is
> > > > locking/locking the same lock that pte_offset_map_lock() is getting.
> > > 
> > > Yes, and we really need patch #8, anything else is just confusing reviewers.
> > 
> > It is a bit better with patch 8, but it is still not optimal, we don't
> > need to do the atomic work at all if the entire ptep is accessed while
> > locked. So the above is stil not what I would expect here, even with
> > RCU.
> > 
> > eg I would expect that this kind of change would work first with the
> > existing paired acessors, ie
> > 
> > 	pte = pte_offset_map(pmd, address);
> > 	pte_unmap(pte);
> > 
> > Should handle the refcount under the covers, and same kind of idea for
> > the _locked/_unlocked varient.
> 
> See my other mail.

Do you have a reference?

> > Only places that don't already use that pairing should get modified.
> > 
> > To do this we have to extend the API so that pte_offset_map() can
> > fail, or very cleverly return some kind of global non-present pte page
> > (I wonder if the zero page would work?)
> 
> I explored both ideas (returning NULL, return a specially prepared page) and
> it didn't work in some cases where we unmap+remap etc.

I wouldn't think it works everywhere, bit it works in a lot of places,
and it is a heck of a lot better than what is proposed here. I'd
rather see the places that can use it be moved, and the few places
that can't be opencoded.

Jason