On 7/27/23 2:10 AM, Michał Mirosław wrote:
On Wed, 26 Jul 2023 at 10:34, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 7/25/23 11:05 PM, Michał Mirosław wrote:
On Tue, 25 Jul 2023 at 11:11, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
Michal please post your thoughts before I post this as v26.
[...]
Looks ok - minor things below.
- 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.
We shouldn't name the flags based on the use case but based on what they actually do. So if this checks UFFD registration for WP, then maybe PAGE_IS_WPALLOWED or something better describing the trait it matches?
PAGE_IS_WPALLOWED seems appropriate.
- 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.
The overwrite of `start` is making the ioctl restart (continuation) easier to handle. I prefer the current way, but it's not a strong opinion.
We shouldn't overwrite the start if we aren't gonna put the correct tag. So I've resorted to adding another variable `walk_end` to return the walk ending address.
- 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.
How is GetWriteWatch() working when passed a VirtualAlloc(..., MEM_LARGE_PAGES|MEM_WRITE_WATCH...)-allocated range? Does it still report 4K pages? This is only a problem when using max_pages: a hugetlb range might need counting and reporting huge pages and not 4K parts.
Best Regards Michał Mirosław
I'll send v26 in next hour.