On Fri, Oct 03, 2025 at 09:41:32PM +0000, Michael Kelley wrote: > From: Stanislav Kinsburskii <[email protected]> Sent: Friday, > October 3, 2025 9:52 AM > > > > > On Fri, Oct 03, 2025 at 12:27:13AM +0000, Michael Kelley wrote: > > > From: Stanislav Kinsburskii <[email protected]> Sent: > > > Thursday, October 2, 2025 9:36 AM > > > > > > > > Reduce overhead when unmapping large memory regions by batching GPA > > > > unmap > > > > operations in 2MB-aligned chunks. > > > > > > > > Use a dedicated constant for batch size to improve code clarity and > > > > maintainability. > > > > > > > > Signed-off-by: Stanislav Kinsburskii <[email protected]>
<snip> > > > > + MSHV_MAX_UNMAP_GPA_PAGES, > > > > unmap_flags); > > > > + > > > > + page_offset += MSHV_MAX_UNMAP_GPA_PAGES - 1; > > > > > > This update to the page_offset doesn't take into account the effect of the > > > ALIGN (or ALIGN_DOWN) call. With a change to ALIGN_DOWN(), it may > > > increment too far and perhaps cause the "for" loop to be exited > > > prematurely, > > > which would fail to unmap some of the pages. > > > > > > > I’m not sure I see the problem here. If we align the offset by > > MSHV_MAX_UNMAP_GPA_PAGES and unmap the same number of pages, then we > > should increment the offset by that very same number, shouldn’t we? > > Here's an example showing the problem I see (assuming ALIGN_DOWN > instead of ALIGN): > > 1) For simplicity in the example, assume region->start_gfn is zero. > 2) Entries 0 thru 3 (i.e., 4 entries) in region->pages[] are zero. > 3) Entries 4 thru 515 (the next 512 entries) are non-zero. > 4) Entries 516 thru 1023 (the next 508 entries) are zero. > 5) Entries 1024 thru 1535 (the last 512 entries) are non-zero. > > Upon entering the "for" loop for the first time, page_offset gets > incremented to 4 because of skipping entries 0 thru 3 that are zero. > On the next iteration where page_offset is 4, the hypercall is made, > passing 0 for the gfn (because of ALIGN_DOWN), with a count of > 512, so entries 0 thru 511 are unmapped. Entries 0 thru 3 are valid > entries, and the fact that they aren't mapped is presumably ignored > by the hypercall, so everything works. > > Then page_offset is incremented by 511, so it will be 515. Continuing > the "for" loop increments page_offset to 516. The zero entries 516 > thru 1023 increment page_offset to 1024. Finally the hypercall is made > again covering entries 1024 thru 1535, none of which are zero. > > But notice that entries 512 thru 515 (which are non-zero) got skipped. > That's because the first invocation of the hypercall covered only through > entry 511, while page_offset was incremented to 515. page_offset > should have been set to 511, since that's the last entry processed by > the first invocation of the hypercall. > > Michael I see the problem now. Thank you. I'll fix it in the next revision. Thanks, Stanislav
