On 7/27/23 2:10 AM, Michał Mirosław wrote: > 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? PAGE_IS_WPALLOWED seems appropriate. > >>> 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. 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. > >>> 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 I'll send v26 in next hour. -- BR, Muhammad Usama Anjum
