Re: [PATCH] mm: khugepaged: fix call hpage_collapse_scan_file() for anonymous vma

[Yang Shi <yang@os.amperecomputing.com>]

On 1/9/25 8:31 PM, Matthew Wilcox wrote:
> On Thu, Jan 09, 2025 at 09:00:24AM -0800, Yang Shi wrote:
>> Thanks for catching this. It sounds a little bit weird to have vm_file for
>> an anonymous VMA. I'm not sure why we should keep such special case. It
>> seems shared mapping is treated as shmem file mapping. So can we set vm_file
>> to NULL when mmap'ing /dev/zero for private mapping? Something like:
>>
>> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
>> index 169eed162a7f..fc332efc5c11 100644
>> --- a/drivers/char/mem.c
>> +++ b/drivers/char/mem.c
>> @@ -527,6 +527,7 @@ static int mmap_zero(struct file *file, struct
>> vm_area_struct *vma)
>>          if (vma->vm_flags & VM_SHARED)
>>                  return shmem_zero_setup(vma);
>>          vma_set_anonymous(vma);
>> +       vma->vm_file = NULL;
>>          return 0;
>>   }
> I'm wary this might cause other bugs somewhere.  rc6 is a bit late to be
> introducing such a subtle change.

Thanks for the extra caution. Applying the proposed fix in khugepaged 
code is fine to me either. We can try to kill the special case later.

Looking at the code further, I think we should do more to make private 
/dev/zero mapping an anonymous mapping:

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 169eed162a7f..98cfac2bb01f 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -527,6 +527,10 @@ static int mmap_zero(struct file *file, struct 
vm_area_struct *vma)
         if (vma->vm_flags & VM_SHARED)
                 return shmem_zero_setup(vma);
         vma_set_anonymous(vma);
+       fput(vma->vm_file);
+       vma->vm_file = NULL;
+       vma->vm_pgoff = vma->vm_start >> PAGE_SHIFT;
+
         return 0;
  }


AFAICT, the user visible effect is we will have different entry in 
smaps/maps.

Before the change:
ffffb7190000-ffffb7590000 rw-p 00001000 00:06 8                          
/dev/zero
Size:               4096 kB
KernelPageSize:        4 kB
MMUPageSize:           4 kB
Rss:                   4 kB
Pss:                   4 kB
Pss_Dirty:             4 kB
Shared_Clean:          0 kB
Shared_Dirty:          0 kB
Private_Clean:         0 kB
Private_Dirty:         4 kB
Referenced:            4 kB
Anonymous:             4 kB
KSM:                   0 kB
LazyFree:              0 kB
AnonHugePages:         0 kB
ShmemPmdMapped:        0 kB
FilePmdMapped:         0 kB
Shared_Hugetlb:        0 kB
Private_Hugetlb:       0 kB
Swap:                  0 kB
SwapPss:               0 kB
Locked:                0 kB
THPeligible:           0
VmFlags: rd wr mr mw me ac

After the change:
ffffb6130000-ffffb6530000 rw-p 00000000 00:00 0
Size:               4096 kB
KernelPageSize:        4 kB
MMUPageSize:           4 kB
Rss:                   4 kB
Pss:                   4 kB
Pss_Dirty:             4 kB
Shared_Clean:          0 kB
Shared_Dirty:          0 kB
Private_Clean:         0 kB
Private_Dirty:         4 kB
Referenced:            4 kB
Anonymous:             4 kB
KSM:                   0 kB
LazyFree:              0 kB
AnonHugePages:         0 kB
ShmemPmdMapped:        0 kB
FilePmdMapped:         0 kB
Shared_Hugetlb:        0 kB
Private_Hugetlb:       0 kB
Swap:                  0 kB
SwapPss:               0 kB
Locked:                0 kB
THPeligible:           0
VmFlags: rd wr mr mw me ac

I'm not sure who really cares about the difference.