On Thu, Mar 17, 2022 at 09:13:50AM +0100, David Hildenbrand wrote:
On 17.03.22 03:54, Alistair Popple wrote:
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 least 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 keep vm_normal_any_page() (or perhaps call it vm_any_page?). This is basically what 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, false)
Except we should add:
#define vm_normal_any_page(vma, addr, pte) _vm_normal_page(vma, addr, pte, true)
"normal" simply tells us that this is not a special mapping -- IOW, we want the VM to take a look at the memmap and not treat it like a PFN map. What we're changing is that we're now also returning non-lru pages. Fair enough, that's why we introduce vm_normal_lru_page() as a replacement where we really can only deal with lru pages.
vm_normal_page vs vm_normal_lru_page is good enough. "lru" further limits what we get via vm_normal_page, that's even how it's implemented.
This naming makes sense to me.
Jason