Re: [v3] fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

[Muhammad Usama Anjum]

On 7/25/23 11:05 PM, Michał Mirosław wrote:
> On Tue, 25 Jul 2023 at 11:11, Muhammad Usama Anjum
> <usama.anjum@xxxxxxxxxxxxx> wrote:
>>
>> ----
>> Michal please post your thoughts before I post this as v26.
>> ----
> [...]
> 
> Looks ok - minor things below.
> 
> 1. I'd change the _WPASYNC things to something better, if this can
> also work with "normal" UFFD WP.
Yeah, but we don't have any use case where UFFD WP is required. It can be
easily added later when user case arrives. Also UFFD WP sends messages to
userspace. User can easily do the bookkeeping in userspace as performance
isn't a concern there.

> 
> 2. For the address tagging part I'd prefer someone who knows how this
> is used take a look. We're ignoring the tag (but clear it on return in
> ->start) - so it doesn't matter for the ioctl() itself.
I've added Kirill if he can give his thoughts about tagged memory.

Right now we are removing the tags from all 3 pointers (start, end, vec)
before using the pointers on kernel side. But we are overwriting and
writing the walk ending address in start which user can read/use.

I think we shouldn't over-write the start (and its tag) and instead return
the ending walk address in new variable, walk_end.

> 
> 3. BTW, One of the uses is the GetWriteWatch and I wonder how it
> behaves on HugeTLB (MEM_LARGE_PAGES allocation)? Shouldn't it return a
> list of huge pages and write *lpdwGranularity = HPAGE_SIZE?
Wine/Proton doesn't used hugetlb by default. Hugetlb isn't enabled by
default on Debian as well. For GetWriteWatch() we don't care about the
hugetlb at all. We have added hugetlb's implementation to complete the
feature and leave out something.

Also GetWriteWatch() implementation wouldn't require THP support as well
because you start to get 2MB of memory dirty even when only 4kB of memory
shouldn't have been dirty.

> 
> 4. The docs and commit messages need some rewording due to the changes
> in the API.
Yeah, I've updated the doc. I'll update the commit message as well.

> 
> Other than that:
> 
> Reviewed-by: Michał Mirosław <mirq-linux@xxxxxxxxxxxx>

-- 
BR,
Muhammad Usama Anjum