Received-SPF: softfail (google.com: domain of transitioning linux-ext4-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   Alistair Popple <apopple@nvidia.com>
To:     Felix Kuehling <felix.kuehling@amd.com>
Cc:     David Hildenbrand <david@redhat.com>,
        Alex Sierra <alex.sierra@amd.com>, jgg@nvidia.com,
        linux-mm@kvack.org, rcampbell@nvidia.com,
        linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org,
        amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        hch@lst.de, jglisse@redhat.com, willy@infradead.org,
        akpm@linux-foundation.org
Subject: Re: [PATCH v1 1/3] mm: split vm_normal_pages for LRU and non-LRU
 handling
Date:   Thu, 17 Mar 2022 13:54:32 +1100
References: <20220310172633.9151-1-alex.sierra@amd.com>
 <20220310172633.9151-2-alex.sierra@amd.com>
 <07401a0a-6878-6af2-f663-9f0c3c1d88e5@redhat.com>
 <1747447c-202d-9195-9d44-57f299be48c4@amd.com>
User-agent: mu4e 1.6.9; emacs 27.1
In-reply-to: <1747447c-202d-9195-9d44-57f299be48c4@amd.com>
Message-ID: <87lex98dtg.fsf@nvdebian.thelocal>
Content-Type: multipart/mixed; boundary="=-=-="
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bU1HL1I3dzRzSCtJQkpIU1F6Q3pDRWJ0Q0ttZDZ2em0ya20wM2R5MU41bzNS?=
 =?utf-8?B?TTUrNnZvS3BFSjNuTFF2STNnL1hkZ2ZuOXB1eVppckxTN3lDWGY5VCttQ1BW?=
 =?utf-8?B?Z3dBMVVRRWdqRzRPeUIzOElISUc4QU5SVUpLOTdxRkltRDlndmQvcVB0OURM?=
 =?utf-8?B?Y0lCeS9jU2NEYXNZQm5LaHNMZnVGQmQ5QTkzMHhTZnNhNk8vSzdyUHhXNjZZ?=
 =?utf-8?B?Wi9OdFd5QVdTZ3RaWXk1dVVWRVlSVEN3SzJBemNmMVhNWDlCbk9hWjg5QS81?=
 =?utf-8?B?WGhYM04yR2tOK2o4RUxwZTBFVFdQakVnTWtsMmZUTyt1R3pzNlVvUmJSdEsx?=
 =?utf-8?B?WkYyeDlVTU82a2tDWVM2Q2NDN1pvbGlxVkZxaXpkYzRvY2MwazFjd2hseHNq?=
 =?utf-8?B?QjF3V2g5ZGNUTkRmMm1FcGM4V25DQjhpaUxXcTgzd3ViVTUxaE82bVlvRmRO?=
 =?utf-8?B?Z2Y5K1d3bE9pRGJlam9sMDFKcUEvdHNscjN1WVlCdFpoM0FQVEljMEx2RkVW?=
 =?utf-8?B?SXcxaXVPUVRUSE1hQlZPM0tvTkQ3VUtrN2FQT1BGS3VEc0Fwc1BET2dsQkRm?=
 =?utf-8?B?ZXN0eVJNTFNnWDRWL0xxVmVqY25QQlM4Z0hCS01XOTNnNzV6dkVUdUlwOFhL?=
 =?utf-8?B?TlM3bUtLWlZwR0o5WlpmSmZsU0FSNUYyenBoMDRVTytCZC9kR0FGc2pJU0sy?=
 =?utf-8?B?N21WOExhMVJVWWNQYjVmZDFJSkJGQWdZbWFXSjBxb0hydmZ5L0plMDRGZVN2?=
 =?utf-8?B?OEw5dWN0dUVBNWUraHlmZ1hQcGxlRVNMUTVnRnlJcnhpamNMMkhWS1NaVytJ?=
 =?utf-8?B?QTZUbEp1UnlYbVNEZldIMytBdi9mVG45WVhVQ3pieW9UUTZEejNqSk5ha3ZJ?=
 =?utf-8?B?K3pFa1pGQm9rbnkzbGJYMzViOHBXdUhZYVR3ekxFc0JSK0ZCY0lyS1hoNS9a?=
 =?utf-8?B?cjBsM1ZzWmw4ekJCL3BXUGMvZ3hCK1R0MWxSR3JZTzBEalg4WmNXWUEzSHB2?=
 =?utf-8?B?a09UVnBXbjlpVGVrcEhnQ0k4dTdSdEJTeGkxQ0preDRtaU04WUNMbmUrcWFh?=
 =?utf-8?B?Z3RCUkhyNk8wWGVJTnVJeVRXSE9ZVUp0cHVOSnNuditxYnBzNVZhWUNZTm11?=
 =?utf-8?B?cS9JeW95ZVRSTERhNWowWCs3Zi94SmtGNVg5bUZYZmUwc3hnUGpUMVBDY05h?=
 =?utf-8?B?SEVOUThvanZoU3VIZVFmNTY0bXBmZDBjWkZRR2gvQUpSekt1VXYvOFQ0N253?=
 =?utf-8?B?ckwrZ1JTb3prWmtValN4MnRYcERZYXJMRXIvQThGWEFvNDhDUkxvUk52a3NB?=
 =?utf-8?B?dWZydWt1N0FkWnU0VVM4WTFITE1FdzlqbjJGOFI2ck1oZWcvcmZuZXA5T3dB?=
 =?utf-8?B?Wjl2TlRzeGtaM0xzN21ULzdhNTJRSG9IUTJ4bEJJRVZZWWRac3lUMmZ6bzJW?=
 =?utf-8?B?S095REk1OXVVd20rN3BXc3RWTUtGbTlqMFkzM3dlTVNkMjhrbFR6ZTVGb1FC?=
 =?utf-8?B?N2pTYkFKVFJQM0dOenBMUm9wRERwL2dJZzFkZ3daY0dKWXJGeTZXQUZWRnFp?=
 =?utf-8?B?T2dRamFCQnJQMG5CMFRmOTV6Y1A5aTJYa0N3KzhGK0VTbGMrREx5dkJzVUpx?=
 =?utf-8?B?WEpPbmFJaitjUVl6bE1kcWJVZHpQMkliZEJycWZua2tlNlVKQmg5QjdlWUdD?=
 =?utf-8?B?anVIeWF3a1Z2cW1BMjJKTktQUDJ5bTV2TFdCWUo1cmtoOGFzKzBxYm1lK2h2?=
 =?utf-8?B?bEpkak8zcnBlNUhnb0prMnk5bE5oeHZaQkxMM0xMa0lQanFuU0xsdW5TaFR5?=
 =?utf-8?B?OUlxemdVMTBZSzk5NmV2aTFrVEJzeHJaYmFZczRHemljWVlNY3l5azBWUUVG?=
 =?utf-8?B?MHJ1UjJselNOUE4vN2pXNTdpNy84N2hYdUkyZDhxK2VvRit2WGUzdG9LZmFB?=
 =?utf-8?Q?XTlAofVH35o3jxAcI42aUAceoHbvAuGR?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 792d664f-dca3-4051-ca5f-08da07c52196
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Mar 2022 03:20:48.1234
 (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: dqU/RXm16+RQUo3Xt06yKA5nAmBiBb3CrcZCjOk/Sjz79bMK5C2XjjQCl0Q/00JU7WG5VrZZ2iFO3hjRDF86Tw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR12MB1750
X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable 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

--=-=-=
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Felix Kuehling <felix.kuehling@amd.com> writes:

> On 2022-03-11 04:16, David Hildenbrand wrote:
>> On 10.03.22 18:26, Alex Sierra wrote:
>>> DEVICE_COHERENT pages introduce a subtle distinction in the way
>>> "normal" pages can be used by various callers throughout the kernel.
>>> They behave like normal pages for purposes of mapping in CPU page
>>> tables, and for COW. But they do not support LRU lists, NUMA
>>> migration or THP. Therefore we split vm_normal_page into two
>>> functions vm_normal_any_page and vm_normal_lru_page. The latter will
>>> only return pages that can be put on an LRU list and that support
>>> NUMA migration, KSM and THP.
>>>
>>> We also introduced a FOLL_LRU flag that adds the same behaviour to
>>> follow_page and related APIs, to allow callers to specify that they
>>> expect to put pages on an LRU list.
>>>
>> I still don't see the need for s/vm_normal_page/vm_normal_any_page/. And
>> as this patch is dominated by that change, I'd suggest (again) to just
>> drop it as I don't see any value of that renaming. No specifier implies =
any.
>
> OK. If nobody objects, we can adopts that naming convention.

I'd prefer we avoid the churn too, but I don't think we should make
vm_normal_page() the equivalent of vm_normal_any_page(). It would mean
vm_normal_page() would return non-LRU device coherent pages, but to me at l=
east
device coherent pages seem special and not what I'd expect from a function =
with
"normal" in the name.

So I think it would be better to s/vm_normal_lru_page/vm_normal_page/ and k=
eep
vm_normal_any_page() (or perhaps call it vm_any_page?). This is basically w=
hat
the previous incarnation of this feature did:

struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr=
,
                            pte_t pte, bool with_public_device);
#define vm_normal_page(vma, addr, pte) _vm_normal_page(vma, addr, pte, fals=
e)

Except we should add:

#define vm_normal_any_page(vma, addr, pte) _vm_normal_page(vma, addr, pte, =
true)

>> The general idea of this change LGTM.
>>
>>
>> I wonder how this interacts with the actual DEVICE_COHERENT coherent
>> series. Is this a preparation? Should it be part of the DEVICE_COHERENT
>> series?
>
> Yes, it should be part of that series. Alex developed it on top of the se=
ries
> for now. But I think eventually it would need to be spliced into it.

Agreed, this needs to go at the start of the DEVICE_COHERENT series.

Thanks.

Alistair

> Patch1 would need to go somewhere before the other DEVICE_COHERENT patche=
s (with
> minor modifications). Patch 2 could be squashed into "tools: add hmm gup =
test
> for long term pinned device pages" or go next to it. Patch 3 doesn't have=
 a
> direct dependency on device-coherent pages. It only mentions them in comm=
ents.
>
>
>>
>> IOW, should this patch start with
>>
>> "With DEVICE_COHERENT, we'll soon have vm_normal_pages() return
>> device-managed anonymous pages that are not LRU pages. Although they
>> behave like normal pages for purposes of mapping in CPU page, and for
>> COW, they do not support LRU lists, NUMA migration or THP. [...]"
>
> Yes, that makes sense.
>
> Regards,
> =C2=A0 Felix
>
>
>>
>> But then, I'm confused by patch 2 and 3, because it feels more like we'd
>> already have DEVICE_COHERENT then ("hmm_is_coherent_type").
>>
>>

--=-=-=--