Messages in this thread | | | Date | Tue, 9 Apr 2024 21:35:08 +0200 | Subject | Re: [PATCH v3 1/7] mm: Add a bitmap into mmu_notifier_{clear,test}_young | From | David Hildenbrand <> |
| |
On 09.04.24 20:31, James Houghton wrote: > Ah, I didn't see this in my inbox, sorry David!
No worries :)
> > On Thu, Apr 4, 2024 at 11:52 AM David Hildenbrand <david@redhat.com> wrote: >> >> On 02.04.24 01:29, James Houghton wrote: >>> diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h >>> index f349e08a9dfe..daaa9db625d3 100644 >>> --- a/include/linux/mmu_notifier.h >>> +++ b/include/linux/mmu_notifier.h >>> @@ -61,6 +61,10 @@ enum mmu_notifier_event { >>> >>> #define MMU_NOTIFIER_RANGE_BLOCKABLE (1 << 0) >>> >>> +#define MMU_NOTIFIER_YOUNG (1 << 0) >>> +#define MMU_NOTIFIER_YOUNG_BITMAP_UNRELIABLE (1 << 1) >> >> Especially this one really deserves some documentation :) > > Yes, will do. Something like > > MMU_NOTIFIER_YOUNG_BITMAP_UNRELIABLE indicates that the passed-in > bitmap either (1) does not accurately represent the age of the pages > (in the case of test_young), or (2) was not able to be used to > completely clear the age/access bit (in the case of clear_young).
Make sense. I do wonder what the expected reaction from the caller is :)
> >> >>> +#define MMU_NOTIFIER_YOUNG_FAST (1 << 2) >> >> And that one as well. > > Something like > > Indicates that (1) passing a bitmap ({test,clear}_young_bitmap) > would have been supported for this address range. > > The name MMU_NOTIFIER_YOUNG_FAST really comes from the fact that KVM > is able to harvest the access bit "fast" (so for x86, locklessly, and > for arm64, with the KVM MMU read lock), "fast" enough that using a > bitmap to do look-around is probably a good idea.
Is that really the right way to communicate that ("would have been supported") -- wouldn't we want to sense support differently?
> >> >> Likely best to briefly document all of them, and how they are >> supposed to be used (return value for X). > > Right. Will do. > >> >>> + >>> struct mmu_notifier_ops { >>> /* >>> * Called either by mmu_notifier_unregister or when the mm is >>> @@ -106,21 +110,36 @@ struct mmu_notifier_ops { >>> * clear_young is a lightweight version of clear_flush_young. Like the >>> * latter, it is supposed to test-and-clear the young/accessed bitflag >>> * in the secondary pte, but it may omit flushing the secondary tlb. >>> + * >>> + * If @bitmap is given but is not supported, return >>> + * MMU_NOTIFIER_YOUNG_BITMAP_UNRELIABLE. >>> + * >>> + * If the walk is done "quickly" and there were young PTEs, >>> + * MMU_NOTIFIER_YOUNG_FAST is returned. >>> */ >>> int (*clear_young)(struct mmu_notifier *subscription, >>> struct mm_struct *mm, >>> unsigned long start, >>> - unsigned long end); >>> + unsigned long end, >>> + unsigned long *bitmap); >>> >>> /* >>> * test_young is called to check the young/accessed bitflag in >>> * the secondary pte. This is used to know if the page is >>> * frequently used without actually clearing the flag or tearing >>> * down the secondary mapping on the page. >>> + * >>> + * If @bitmap is given but is not supported, return >>> + * MMU_NOTIFIER_YOUNG_BITMAP_UNRELIABLE. >>> + * >>> + * If the walk is done "quickly" and there were young PTEs, >>> + * MMU_NOTIFIER_YOUNG_FAST is returned. >>> */ >>> int (*test_young)(struct mmu_notifier *subscription, >>> struct mm_struct *mm, >>> - unsigned long address); >>> + unsigned long start, >>> + unsigned long end, >>> + unsigned long *bitmap); >> >> What does "quickly" mean (why not use "fast")? What are the semantics, I >> don't find any existing usage of that in this file. > > "fast" means fast enough such that using a bitmap to scan adjacent > pages (e.g. with MGLRU) is likely to be beneficial. I'll write more in > this comment. Perhaps I should just rename it to > MMU_NOTIFIER_YOUNG_BITMAP_SUPPORTED and drop the whole "likely to be > beneficial" thing -- that's for MGLRU/etc. to decide really.
Yes!
> >> >> Further, what is MMU_NOTIFIER_YOUNG you introduce used for? > > MMU_NOTIFIER_YOUNG is the return value when the page was young, but we > (1) didn't use a bitmap, and (2) the "fast" access bit harvesting > wasn't possible. In this case we simply return 1, which is > MMU_NOTIFIER_YOUNG. I'll make kvm_mmu_notifier_test_clear_young() > properly return MMU_NOTIFIER_YOUNG instead of relying on the fact that > it will be 1.
Yes, that will clarify it!
-- Cheers,
David / dhildenb
| |