On Wed, 26 Jul 2023 at 10:34, Muhammad Usama Anjum <usama.anjum@xxxxxxxxxxxxx> 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@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. 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? > > 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. 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. > > 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. 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
