Re: [GIT PULL] Please pull hmm changes
On Wed, Dec 18, 2019 at 10:37 AM Jason Gunthorpe wrote: > > I think this is what you are looking for? I think that with these names, I would have had an easier time reading the original patch that made me go "Eww", yes. Of course, now that it's just a rename patch, it's not like the patch is all that legible, but yeah, I think the naming is saner. Linus ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [GIT PULL] Please pull hmm changes
On Wed, Dec 18, 2019 at 08:53:05AM -0800, Linus Torvalds wrote: > On Wed, Dec 18, 2019 at 6:59 AM Jason Gunthorpe wrote: > Yes, global function names need to be unique, and if they aren't > really core, they want some prefix that explains the context, because > global functions are called from _outside_ the context that explains > them. Yes, in this thread I have mostly talked about changing the global struct names, and for that mmn_ is the context explaining prefix. > But if it's a "struct mmu_interval_notifier" pointer, and it's inside > a file that is all about these pointers, it shouldn't be called > "mmn_xyz". That's not a name. That's line noise. > > So call it a "notifier". Maybe even an "interval_notifier" if you > don't mind the typing. Name it by something _descriptive_. And if you > want. > > And "subscriptions" is a lovely name. What does the "mmn" buy you? To be clear, I was proposing this as the struct name: 'struct mmu_notifier_mm' becomes 'struct mmn_subscriptions' (and similar for other mmu_notifier_x* and mmu_x_notifier patterns) >From there we now have a natural and readable local variable name like 'subscriptions' within mm/mmu_notifier.c I've just started looking at this in detail, but it seems sticking with 'mmu_notifier_' as the global prefix will avoid a fair amount of churn. So lets not try to shorten it to mmn_ as the global prefix. > Just to clarify: the names I really hated were the local variable > names (and the argument names) that were all entirely within the > context of mm/mmu_notifier.c. Calling something "mmn_mm" is a random > jumble of letters that looks more like you're humming than you're > speaking. Yes, I understood - I've approached trying to have good names for the variables via having good names for their struct's. Below is what I'm suggesting for the first patch. I am intending a patch series to make the naming better across mmu_notifier.h, this would be the first. Next patches would probably replace 'mn' with 'list_sub' (struct mmu_notifier_subscription) and 'mni' with 'range_sub' (struct mmu_notifier_range_subscription). Thus we have code working on a 'list_sub/range_sub' and storing it in a 'subscriptions'. I think this is what you are looking for? >From c656d0862dedcc2f5f4beda129a2ac51c892be7e Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Wed, 18 Dec 2019 13:40:35 -0400 Subject: [PATCH] mm/mmu_notifier: Rename struct mmu_notifier_mm to mmu_notifier_subscriptions The name mmu_notifier_mm implies that the thing is a mm_struct pointer, and is difficult to abbreviate. The struct is actually holding the interval tree and hlist containing the notifiers subscribed to a mm. Use 'subscriptions' as the variable name for this struct instead of the really terrible and misleading 'mmn_mm'. Signed-off-by: Jason Gunthorpe --- include/linux/mm_types.h | 2 +- include/linux/mmu_notifier.h | 18 +- kernel/fork.c| 4 +- mm/debug.c | 4 +- mm/mmu_notifier.c| 322 ++- 5 files changed, 182 insertions(+), 168 deletions(-) diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h index 270aa8fd2800b4..e87bb864bdb29a 100644 --- a/include/linux/mm_types.h +++ b/include/linux/mm_types.h @@ -490,7 +490,7 @@ struct mm_struct { /* store ref to file /proc//exe symlink points to */ struct file __rcu *exe_file; #ifdef CONFIG_MMU_NOTIFIER - struct mmu_notifier_mm *mmu_notifier_mm; + struct mmu_notifier_subscriptions *notifier_subscriptions; #endif #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && !USE_SPLIT_PMD_PTLOCKS pgtable_t pmd_huge_pte; /* protected by page_table_lock */ diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h index 9e6caa8ecd1938..a302925fbc6177 100644 --- a/include/linux/mmu_notifier.h +++ b/include/linux/mmu_notifier.h @@ -8,7 +8,7 @@ #include #include -struct mmu_notifier_mm; +struct mmu_notifier_subscriptions; struct mmu_notifier; struct mmu_notifier_range; struct mmu_interval_notifier; @@ -265,7 +265,7 @@ struct mmu_notifier_range { static inline int mm_has_notifiers(struct mm_struct *mm) { - return unlikely(mm->mmu_notifier_mm); + return unlikely(mm->notifier_subscriptions); } struct mmu_notifier *mmu_notifier_get_locked(const struct mmu_notifier_ops *ops, @@ -364,7 +364,7 @@ static inline bool mmu_interval_check_retry(struct mmu_interval_notifier *mni, return READ_ONCE(mni->invalidate_seq) != seq; } -extern void __mmu_notifier_mm_destroy(struct mm_struct *mm); +extern void __mmu_notifier_subscriptions_destroy(struct mm_struct *mm); extern void __mmu_notifier_release(struct mm_struct *mm); extern int __mmu_notifier_clear_flush_young(struct mm_struct *mm, unsigned long start, @@ -480,15 +480,15 @@ static inline void mmu_notifier_invalidate_range(struct
Re: [GIT PULL] Please pull hmm changes
On Wed, Dec 18, 2019 at 6:59 AM Jason Gunthorpe wrote: > > Do you think calling it 'mmn_subscriptions' is clear? Why do you want that "mmn"? Guys, the "mmn" part is clear from the _context_. The function name is When the function name is something like "mmu_interval_read_begin()", and the filename is "mm/mmu_notifier.c", you do NOT NEED silly prefixes like "mmn" for local variables. They add NOTHING. And they make the code an illegible mess. Yes, global function names need to be unique, and if they aren't really core, they want some prefix that explains the context, because global functions are called from _outside_ the context that explains them. But if it's a "struct mmu_interval_notifier" pointer, and it's inside a file that is all about these pointers, it shouldn't be called "mmn_xyz". That's not a name. That's line noise. So call it a "notifier". Maybe even an "interval_notifier" if you don't mind the typing. Name it by something _descriptive_. And if you want. And "subscriptions" is a lovely name. What does the "mmn" buy you? Just to clarify: the names I really hated were the local variable names (and the argument names) that were all entirely within the context of mm/mmu_notifier.c. Calling something "mmn_mm" is a random jumble of letters that looks more like you're humming than you're speaking. Don't mumble. Speak _clearly_. The other side of "short names" is that some non-local conventions exist because they are _so_ global. So if it's just a mm pointer, call it "mm". We do have some very core concepts in the kernel that permeate _everything_, and those core things we tend to have very short names for. So whenever you're working with VM code, you'll see lots of small names like "mm", "vma", "pte" etc. They aren't exactly clear, but they are _globally_ something you read and learn when you work on the Linux VM code. That's very diofferent from "mmn" - the "mmn" thing isn't some global shorthand, it is just a local abomination. So "notifier_mm" makes sense - it's the mm for a notifier. But "mmn_notifier" does not, because "mmn" only makes sense in a local context, and in that local context it's not any new information at all. See the difference? Two shorthands, but one makes sense and adds information, while the other is just unnecessary and pointless and doesn't add anything at all. Linus ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [GIT PULL] Please pull hmm changes
On Fri, Dec 13, 2019 at 11:19:16AM +0100, Daniel Vetter wrote: > On Wed, Dec 11, 2019 at 10:57:13PM +, Jason Gunthorpe wrote: > > On Thu, Dec 05, 2019 at 11:03:24AM -0500, Jerome Glisse wrote: > > > > > > struct mmu_notifier_mm (ie the mm->mmu_notifier_mm) > > > >-> mmn_mm > > > > struct mm_struct > > > >-> mm > > > > struct mmu_notifier (ie the user subscription to the mm_struct) > > > >-> mn > > > > struct mmu_interval_notifier (the other kind of user subscription) > > > >-> mni > > > > > > What about "interval" the context should already tell people > > > it is related to mmu notifier and thus a notifier. I would > > > just remove the notifier suffix, this would match the below > > > range. > > > > Interval could be a good replacement for mni in the mm/mmu_notififer > > file if we don't do the wholesale rename > > > > > > I think it would be overall nicer with better names for the original > > > > structs. Perhaps: > > > > > > > > mmn_* - MMU notifier prefix > > > > mmn_state <- struct mmu_notifier_mm > > > > mmn_subscription (mmn_sub) <- struct mmu_notifier > > > > mmn_range_subscription (mmn_range_sub) <- struct mmu_interval_notifier > > > > mmn_invalidate_desc <- struct mmu_notifier_range > > > > > > This looks good. > > > > Well, lets just bite the bullet then and switch it. Do you like > > 'state'? I thought that was the weakest one > > Since you're asking, here's my bikeshed. I kinda agree _state looks a bit > strange for this, what about a _link suffix in the spirit of Do you think calling it 'mmn_subscriptions' is clear? Ie a struct mmn_subscriptions holds the lists of struct mmn_subscription and struct mmn_range_subscription? Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [GIT PULL] Please pull hmm changes
On Wed, Dec 11, 2019 at 10:57:13PM +, Jason Gunthorpe wrote: > On Thu, Dec 05, 2019 at 11:03:24AM -0500, Jerome Glisse wrote: > > > > struct mmu_notifier_mm (ie the mm->mmu_notifier_mm) > > >-> mmn_mm > > > struct mm_struct > > >-> mm > > > struct mmu_notifier (ie the user subscription to the mm_struct) > > >-> mn > > > struct mmu_interval_notifier (the other kind of user subscription) > > >-> mni > > > > What about "interval" the context should already tell people > > it is related to mmu notifier and thus a notifier. I would > > just remove the notifier suffix, this would match the below > > range. > > Interval could be a good replacement for mni in the mm/mmu_notififer > file if we don't do the wholesale rename > > > > I think it would be overall nicer with better names for the original > > > structs. Perhaps: > > > > > > mmn_* - MMU notifier prefix > > > mmn_state <- struct mmu_notifier_mm > > > mmn_subscription (mmn_sub) <- struct mmu_notifier > > > mmn_range_subscription (mmn_range_sub) <- struct mmu_interval_notifier > > > mmn_invalidate_desc <- struct mmu_notifier_range > > > > This looks good. > > Well, lets just bite the bullet then and switch it. Do you like > 'state'? I thought that was the weakest one Since you're asking, here's my bikeshed. I kinda agree _state looks a bit strange for this, what about a _link suffix in the spirit of struct list_head link; The other common name is "node", but I think that's confusing in the context of mm code. The purpose of this struct is to link everything together (and yes it carries also some state, but the main job is to link a mm_struct to a mmu_notifier). At least for me a _state is configuration state for a specific object, not something that links a bunch of things together. But I'm biased on this, since we use that pattern in drm for all the display state tracking. Also feel free to ignore my bikeshed :-) Aside from this I think the proposed names are a solid improvement. -Daniel > > We could use mmnotif as the prefix, this makes the longest: > > struct mmnotif_range_subscription > > Which is reasonable enough > > > Maybe we can do a semantic patch to do convertion and then Linus > > can easily apply the patch by just re-running the coccinelle. > > I tried this last time I renamed everything, it was OK, but it missed > updating the comments. So it still needs some by-hand helping. > > I'll make some patches next week when I get back. > > Jason > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [GIT PULL] Please pull hmm changes
On Thu, Dec 05, 2019 at 11:03:24AM -0500, Jerome Glisse wrote: > > struct mmu_notifier_mm (ie the mm->mmu_notifier_mm) > >-> mmn_mm > > struct mm_struct > >-> mm > > struct mmu_notifier (ie the user subscription to the mm_struct) > >-> mn > > struct mmu_interval_notifier (the other kind of user subscription) > >-> mni > > What about "interval" the context should already tell people > it is related to mmu notifier and thus a notifier. I would > just remove the notifier suffix, this would match the below > range. Interval could be a good replacement for mni in the mm/mmu_notififer file if we don't do the wholesale rename > > I think it would be overall nicer with better names for the original > > structs. Perhaps: > > > > mmn_* - MMU notifier prefix > > mmn_state <- struct mmu_notifier_mm > > mmn_subscription (mmn_sub) <- struct mmu_notifier > > mmn_range_subscription (mmn_range_sub) <- struct mmu_interval_notifier > > mmn_invalidate_desc <- struct mmu_notifier_range > > This looks good. Well, lets just bite the bullet then and switch it. Do you like 'state'? I thought that was the weakest one We could use mmnotif as the prefix, this makes the longest: struct mmnotif_range_subscription Which is reasonable enough > Maybe we can do a semantic patch to do convertion and then Linus > can easily apply the patch by just re-running the coccinelle. I tried this last time I renamed everything, it was OK, but it missed updating the comments. So it still needs some by-hand helping. I'll make some patches next week when I get back. Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [GIT PULL] Please pull hmm changes
On Thu, Dec 05, 2019 at 03:03:56PM -0800, John Hubbard wrote: > No advice, just a naming idea similar in spirit to Jerome's suggestion > (use a longer descriptive word, and don't try to capture the entire phrase): > use "notif" in place of the unloved "mmn". So partially, approximately like > this: > > notif_*<- MMU notifier prefix > notif_state<- struct mmu_notifier_mm > notif_subscription (notif_sub) <- struct mmu_notifier > notif_invalidate_desc <- struct mmu_notifier_range* > notif_range_subscription (notif_range_sub) <- struct mmu_interval_notifier To me 'notif' suggests this belongs to the stuff in notifier.h - ie the naked word notififer is already taken Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [GIT PULL] Please pull hmm changes
On 12/2/19 6:42 PM, Jason Gunthorpe wrote: ... > Regarding the ugly names.. Naming has been really hard here because > currently everything is a 'mmu notifier' and the natural abberviations > from there are crummy. Here is the basic summary: > > struct mmu_notifier_mm (ie the mm->mmu_notifier_mm) >-> mmn_mm > struct mm_struct >-> mm > struct mmu_notifier (ie the user subscription to the mm_struct) >-> mn > struct mmu_interval_notifier (the other kind of user subscription) >-> mni > struct mmu_notifier_range (ie the args to invalidate_range) >-> range > > I can send a patch to switch mmn_mm to mmu_notifier_mm, which is the > only pre-existing name for this value. But IIRC, it is a somewhat ugly > with long line wrapping. 'mni' is a pain, I have to reflect on that. > (honesly, I dislike mmu_notififer_mm quite a lot too) > > I think it would be overall nicer with better names for the original > structs. Perhaps: > > mmn_* - MMU notifier prefix > mmn_state <- struct mmu_notifier_mm > mmn_subscription (mmn_sub) <- struct mmu_notifier > mmn_range_subscription (mmn_range_sub) <- struct mmu_interval_notifier > mmn_invalidate_desc <- struct mmu_notifier_range > > At least this is how I describe them in my mind.. This is a lot of > churn, and spreads through many drivers. This is why I kept the names > as-is and we ended up with the also quite bad 'mmu_interval_notifier' > > Maybe just switch mmu_notifier_mm for mmn_state and leave the drivers > alone? > > Anyone on the CC list have advice? > > Jason No advice, just a naming idea similar in spirit to Jerome's suggestion (use a longer descriptive word, and don't try to capture the entire phrase): use "notif" in place of the unloved "mmn". So partially, approximately like this: notif_*<- MMU notifier prefix notif_state<- struct mmu_notifier_mm notif_subscription (notif_sub) <- struct mmu_notifier notif_invalidate_desc <- struct mmu_notifier_range* notif_range_subscription (notif_range_sub) <- struct mmu_interval_notifier thanks, -- John Hubbard NVIDIA ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [GIT PULL] Please pull hmm changes
On Tue, Dec 03, 2019 at 02:42:12AM +, Jason Gunthorpe wrote: > On Sat, Nov 30, 2019 at 10:23:31AM -0800, Linus Torvalds wrote: > > On Sat, Nov 30, 2019 at 10:03 AM Linus Torvalds > > wrote: > > > > > > I'll try to figure the code out, but my initial reaction was "yeah, > > > not in my VM". > > > > Why is it ok to sometimes do > > > > WRITE_ONCE(mni->invalidate_seq, cur_seq); > > > > (to pair with the unlocked READ_ONCE), and sometimes then do > > > > mni->invalidate_seq = mmn_mm->invalidate_seq; > > > > My initial guess was that latter is only done at initialization time, > > but at least in one case it's done *after* the mni has been added to > > the mmn_mm (oh, how I despise those names - I can only repeat: WTF?). > > Yes, the only occurrences are in the notifier_insert, under the > spinlock. The one case where it is out of the natural order was to > make the manipulation of seq a bit saner, but in all cases since the > spinlock is held there is no way for another thread to get the pointer > to the 'mmu_interval_notifier *' to do the unlocked read. > > Regarding the ugly names.. Naming has been really hard here because > currently everything is a 'mmu notifier' and the natural abberviations > from there are crummy. Here is the basic summary: > > struct mmu_notifier_mm (ie the mm->mmu_notifier_mm) >-> mmn_mm > struct mm_struct >-> mm > struct mmu_notifier (ie the user subscription to the mm_struct) >-> mn > struct mmu_interval_notifier (the other kind of user subscription) >-> mni What about "interval" the context should already tell people it is related to mmu notifier and thus a notifier. I would just remove the notifier suffix, this would match the below range. > struct mmu_notifier_range (ie the args to invalidate_range) >-> range Yeah range as context should tell you it is related to mmu notifier. > > I can send a patch to switch mmn_mm to mmu_notifier_mm, which is the > only pre-existing name for this value. But IIRC, it is a somewhat ugly > with long line wrapping. 'mni' is a pain, I have to reflect on that. > (honesly, I dislike mmu_notififer_mm quite a lot too) > > I think it would be overall nicer with better names for the original > structs. Perhaps: > > mmn_* - MMU notifier prefix > mmn_state <- struct mmu_notifier_mm > mmn_subscription (mmn_sub) <- struct mmu_notifier > mmn_range_subscription (mmn_range_sub) <- struct mmu_interval_notifier > mmn_invalidate_desc <- struct mmu_notifier_range This looks good. > > At least this is how I describe them in my mind.. This is a lot of > churn, and spreads through many drivers. This is why I kept the names > as-is and we ended up with the also quite bad 'mmu_interval_notifier' > > Maybe just switch mmu_notifier_mm for mmn_state and leave the drivers > alone? > > Anyone on the CC list have advice? Maybe we can do a semantic patch to do convertion and then Linus can easily apply the patch by just re-running the coccinelle. Cheers, Jérôme ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [GIT PULL] Please pull hmm changes
On Sat, Nov 30, 2019 at 10:23:31AM -0800, Linus Torvalds wrote: > On Sat, Nov 30, 2019 at 10:03 AM Linus Torvalds > wrote: > > > > I'll try to figure the code out, but my initial reaction was "yeah, > > not in my VM". > > Why is it ok to sometimes do > > WRITE_ONCE(mni->invalidate_seq, cur_seq); > > (to pair with the unlocked READ_ONCE), and sometimes then do > > mni->invalidate_seq = mmn_mm->invalidate_seq; > > My initial guess was that latter is only done at initialization time, > but at least in one case it's done *after* the mni has been added to > the mmn_mm (oh, how I despise those names - I can only repeat: WTF?). Yes, the only occurrences are in the notifier_insert, under the spinlock. The one case where it is out of the natural order was to make the manipulation of seq a bit saner, but in all cases since the spinlock is held there is no way for another thread to get the pointer to the 'mmu_interval_notifier *' to do the unlocked read. Regarding the ugly names.. Naming has been really hard here because currently everything is a 'mmu notifier' and the natural abberviations from there are crummy. Here is the basic summary: struct mmu_notifier_mm (ie the mm->mmu_notifier_mm) -> mmn_mm struct mm_struct -> mm struct mmu_notifier (ie the user subscription to the mm_struct) -> mn struct mmu_interval_notifier (the other kind of user subscription) -> mni struct mmu_notifier_range (ie the args to invalidate_range) -> range I can send a patch to switch mmn_mm to mmu_notifier_mm, which is the only pre-existing name for this value. But IIRC, it is a somewhat ugly with long line wrapping. 'mni' is a pain, I have to reflect on that. (honesly, I dislike mmu_notififer_mm quite a lot too) I think it would be overall nicer with better names for the original structs. Perhaps: mmn_* - MMU notifier prefix mmn_state <- struct mmu_notifier_mm mmn_subscription (mmn_sub) <- struct mmu_notifier mmn_range_subscription (mmn_range_sub) <- struct mmu_interval_notifier mmn_invalidate_desc <- struct mmu_notifier_range At least this is how I describe them in my mind.. This is a lot of churn, and spreads through many drivers. This is why I kept the names as-is and we ended up with the also quite bad 'mmu_interval_notifier' Maybe just switch mmu_notifier_mm for mmn_state and leave the drivers alone? Anyone on the CC list have advice? Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [GIT PULL] Please pull hmm changes
On Sat, Nov 30, 2019 at 10:03 AM Linus Torvalds wrote: > > I'll try to figure the code out, but my initial reaction was "yeah, > not in my VM". Why is it ok to sometimes do WRITE_ONCE(mni->invalidate_seq, cur_seq); (to pair with the unlocked READ_ONCE), and sometimes then do mni->invalidate_seq = mmn_mm->invalidate_seq; My initial guess was that latter is only done at initialization time, but at least in one case it's done *after* the mni has been added to the mmn_mm (oh, how I despise those names - I can only repeat: WTF?). See __mmu_interval_notifier_insert() in the mmn_mm->active_invalidate_ranges case. I'm guessing that it doesn't matter, because when inserting the notifier the sequence number is presumably not used until after the insertion (and any use though mmn_mm is protected by the mmn_mm->lock), but it still looks odd to me. Linus ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [GIT PULL] Please pull hmm changes
On Mon, Nov 25, 2019 at 12:42 PM Jason Gunthorpe wrote: > > Here is this batch of hmm updates, I think we are nearing the end of this > project for now, although I suspect there will be some more patches related to > hmm_range_fault() in the next cycle. I've ended up pulling this, but I'm not entirely happy with the code. You've already seen the comments on it in the earlier replies. Linus ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [GIT PULL] Please pull hmm changes
On Mon, Nov 25, 2019 at 12:42 PM Jason Gunthorpe wrote: > > You will probably be most interested in the patch "mm/mmu_notifier: add an > interval tree notifier". I'm trying to read that patch, and I'm completely unable to by the absolutely *horrid* variable names. There are zero excuses for names like "mmn_mm". WTF? I'll try to figure the code out, but my initial reaction was "yeah, not in my VM". Linus ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[GIT PULL] Please pull hmm changes
Hi Linus, Here is this batch of hmm updates, I think we are nearing the end of this project for now, although I suspect there will be some more patches related to hmm_range_fault() in the next cycle. You will probably be most interested in the patch "mm/mmu_notifier: add an interval tree notifier". The approach here largely pre-exists in the various drivers, but is honestly kind of complex/ugly. No better idea was found, I'm hoping putting it all in one place will help improve this over the long term. At least many bugs were squashed and lines of code eliminated while consolidating. Already i915 GPU has posted a series for the next window that also needs this same approach. There are two small conflicts I know of, the first is RDMA related with -rc, the second is a one liner updating a deleted comment in GPU. Both can be solved by using the hmm.git side of the conflict. All the big driver changes have been acked and/or tested by their respective maintainers. Regards, Jason The following changes since commit d6d5df1db6e9d7f8f76d2911707f7d5877251b02: Linux 5.4-rc5 (2019-10-27 13:19:19 -0400) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git tags/for-linus-hmm for you to fetch changes up to 93f4e735b6d98ee4b7a1252d81e815a983e359f2: mm/hmm: remove hmm_range_dma_map and hmm_range_dma_unmap (2019-11-23 19:56:45 -0400) hmm related patches for 5.5 This is another round of bug fixing and cleanup. This time the focus is on the driver pattern to use mmu notifiers to monitor a VA range. This code is lifted out of many drivers and hmm_mirror directly into the mmu_notifier core and written using the best ideas from all the driver implementations. This removes many bugs from the drivers and has a very pleasing diffstat. More drivers can still be converted, but that is for another cycle. - A shared branch with RDMA reworking the RDMA ODP implementation - New mmu_interval_notifier API. This is focused on the use case of monitoring a VA and simplifies the process for drivers - A common seq-count locking scheme built into the mmu_interval_notifier API usable by drivers that call get_user_pages() or hmm_range_fault() with the VA range - Conversion of mlx5 ODP, hfi1, radeon, nouveau, AMD GPU, and Xen GntDev drivers to the new API. This deletes a lot of wonky driver code. - Two improvements for hmm_range_fault(), from testing done by Ralph Christoph Hellwig (1): mm/hmm: remove hmm_range_dma_map and hmm_range_dma_unmap Jason Gunthorpe (30): RDMA/mlx5: Use SRCU properly in ODP prefetch RDMA/mlx5: Split sig_err MR data into its own xarray RDMA/mlx5: Use a dedicated mkey xarray for ODP RDMA/mlx5: Delete struct mlx5_priv->mkey_table RDMA/mlx5: Rework implicit_mr_get_data RDMA/mlx5: Lift implicit_mr_alloc() into the two routines that call it RDMA/mlx5: Set the HW IOVA of the child MRs to their place in the tree RDMA/mlx5: Split implicit handling from pagefault_mr RDMA/mlx5: Use an xarray for the children of an implicit ODP RDMA/mlx5: Reduce locking in implicit_mr_get_data() RDMA/mlx5: Avoid double lookups on the pagefault path RDMA/mlx5: Rework implicit ODP destroy RDMA/mlx5: Do not store implicit children in the odp_mkeys xarray RDMA/mlx5: Do not race with mlx5_ib_invalidate_range during create and destroy RDMA/odp: Remove broken debugging call to invalidate_range Merge branch 'odp_rework' into hmm.git mm/mmu_notifier: define the header pre-processor parts even if disabled mm/mmu_notifier: add an interval tree notifier mm/hmm: allow hmm_range to be used with a mmu_interval_notifier or hmm_mirror mm/hmm: define the pre-processor related parts of hmm.h even if disabled RDMA/odp: Use mmu_interval_notifier_insert() RDMA/hfi1: Use mmu_interval_notifier_insert for user_exp_rcv drm/radeon: use mmu_interval_notifier_insert nouveau: use mmu_notifier directly for invalidate_range_start nouveau: use mmu_interval_notifier instead of hmm_mirror drm/amdgpu: Call find_vma under mmap_sem drm/amdgpu: Use mmu_interval_insert instead of hmm_mirror drm/amdgpu: Use mmu_interval_notifier instead of hmm_mirror mm/hmm: remove hmm_mirror and related xen/gntdev: use mmu_interval_notifier_insert Ralph Campbell (2): mm/hmm: allow snapshot of the special zero page mm/hmm: make full use of walk_page_range() Documentation/vm/hmm.rst | 105 +-- drivers/gpu/drm/amd/amdgpu/amdgpu.h |2 + drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c |9 +- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 14 +- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |1 + drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
[GIT PULL] Please pull hmm changes
Hi Linus, Locking fix for nouveau's use of HMM This small series was posted by Christoph before the merge window, but didn't make it in time for the PR. It fixes various locking errors in the nouveau driver's use of the hmm_range_* functions. The diffstat is a bit big as Christoph did a comprehensive job to move the obsolete API from the core header and into the driver before fixing its flow, but the risk of regression from this code motion is low. I don't intend to often send -rc patches for hmm, but this is entangled with other changes already, so it is simpler to keep it on the hmm git branch. Thanks, Jason The following changes since commit 5f9e832c137075045d15cd6899ab0505cfb2ca4b: Linus 5.3-rc1 (2019-07-21 14:05:38 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git tags/for-linus-hmm for you to fetch changes up to de4ee728465f7c0c29241550e083139b2ce9159c: nouveau: unlock mmap_sem on all errors from nouveau_range_fault (2019-07-25 16:14:40 -0300) HMM patches for 5.3-rc Fix the locking around nouveau's use of the hmm_range_* APIs. It works correctly in the success case, but many of the the edge cases have missing unlocks or double unlocks. Christoph Hellwig (4): mm/hmm: always return EBUSY for invalid ranges in hmm_range_{fault,snapshot} mm/hmm: move hmm_vma_range_done and hmm_vma_fault to nouveau nouveau: remove the block parameter to nouveau_range_fault nouveau: unlock mmap_sem on all errors from nouveau_range_fault Documentation/vm/hmm.rst | 2 +- drivers/gpu/drm/nouveau/nouveau_svm.c | 47 -- include/linux/hmm.h | 54 --- mm/hmm.c | 10 +++ 4 files changed, 49 insertions(+), 64 deletions(-) signature.asc Description: PGP signature
Re: [GIT PULL] Please pull hmm changes
The pull request you sent on Tue, 9 Jul 2019 19:24:21 +: > git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git tags/for-linus-hmm has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/fec88ab0af9706b2201e5daf377c5031c62d11f7 Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [GIT PULL] Please pull hmm changes
On Tue, Jul 9, 2019 at 12:24 PM Jason Gunthorpe wrote: > > I'm sending it early as it is now a dependency for several patches in > mm's quilt. .. but I waited to merge it until I had time to review it more closely, because I expected the review to be painful. I'm happy to say that I was overly pessimistic, and that instead of finding things to hate, I found it all looking good. Particularly the whole "use reference counts properly, so that lifetimes make sense and all those nasty cases can't happen" parts. It's all merged, just waiting for the test-build to verify that I didn't miss anything (well, at least nothing obvious). Linus
[GIT PULL] Please pull hmm changes
Hi Linus, As was discussed some time ago here are the mostly -mm patches related to hmm functions. In agreement with Andrew we split this out from quilt into a git topic branch so it can be shared between the DRM and RDMA git trees. However, this cycle did not see dependencies with work in DRM or RDMA that required a topic merge. I expect that work will start to get ready next cycle and we will see a need for a cross-tree topic merge then. I'm sending it early as it is now a dependency for several patches in mm's quilt. This has been an exciting topic branch for conflicts, you'll need the below simple resolution in the merge commit to make it compile (lockdep_assert_held_exclusive() was renamed to lockdep_assert_held_write()) Otherwise, for reference to all parties, here is how the conflicts were handled: - Several small patches from -mm quilt were moved to this tree to simplify conflict management, only Ira's 'fix release_pages()' patch was not hmm related. - DRM introduced a new users of the hmm_range_register() API. We worked with AMDGPU to ensure that their new user could use the revised API via the below trivial merge fixup with DRM: diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -783,7 +783,7 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages) 0 : range->flags[HMM_PFN_WRITE]; range->pfn_flags_mask = 0; range->pfns = pfns; - hmm_range_register(range, mm, start, + hmm_range_register(range, mirror, start, start + ttm->num_pages * PAGE_SIZE, PAGE_SHIFT); retry: - ARM64 has a patch series going through -mm with a trivial conflict ("Devmap cleanups + arm64 support"), Andrew has re-applied this in quilt onto linux-next and will send it - The memreap sub-section changes in -mm has 5 hunk conflict with the memremap changes here. Andrew reapplied Dan's series ontop of Christoph's series in linux-next and will send it. The tag for-linus-hmm-merged with my merge resolution to your tree is also available to pull. Thanks, Jason diff --cc mm/hmm.c index d48b9283725a90,f702a3895d05d8..e1eedef129cf5c --- a/mm/hmm.c +++ b/mm/hmm.c @@@ -42,16 -54,11 +42,16 @@@ static const struct mmu_notifier_ops hm */ static struct hmm *hmm_get_or_create(struct mm_struct *mm) { - struct hmm *hmm = mm_get_hmm(mm); - bool cleanup = false; + struct hmm *hmm; - lockdep_assert_held_exclusive(>mmap_sem); - if (hmm) - return hmm; ++ lockdep_assert_held_write(>mmap_sem); + + /* Abuse the page_table_lock to also protect mm->hmm. */ + spin_lock(>page_table_lock); + hmm = mm->hmm; + if (mm->hmm && kref_get_unless_zero(>hmm->kref)) + goto out_unlock; + spin_unlock(>page_table_lock); hmm = kmalloc(sizeof(*hmm), GFP_KERNEL); if (!hmm) @@@ -245,8 -277,8 +245,8 @@@ static const struct mmu_notifier_ops hm */ int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm) { - lockdep_assert_held_exclusive(>mmap_sem); ++ lockdep_assert_held_write(>mmap_sem); + /* Sanity check */ if (!mm || !mirror || !mirror->ops) return -EINVAL; The following changes since commit 6fbc7275c7a9ba97877050335f290341a1fd8dbf: Linux 5.2-rc7 (2019-06-30 11:25:36 +0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git tags/for-linus-hmm for you to fetch changes up to cc5dfd59e375f4d0f2b64643723d16b38b2f2d78: Merge branch 'hmm-devmem-cleanup.4' into rdma.git hmm (2019-07-02 15:10:45 -0300) HMM patches for 5.3 Improvements and bug fixes for the hmm interface in the kernel: - Improve clarity, locking and APIs related to the 'hmm mirror' feature merged last cycle. In linux-next we now see AMDGPU and nouveau to be using this API. - Remove old or transitional hmm APIs. These are hold overs from the past with no users, or APIs that existed only to manage cross tree conflicts. There are still a few more of these cleanups that didn't make the merge window cut off. - Improve some core mm APIs: * export alloc_pages_vma() for driver use * refactor into devm_request_free_mem_region() to manage DEVICE_PRIVATE resource reservations * refactor duplicative driver code into the core dev_pagemap struct - Remove hmm wrappers of improved core mm APIs, instead have drivers use the simplified API directly - Remove DEVICE_PUBLIC - Simplify the kconfig flow for the hmm users and core code Christoph Hellwig (24): mm: remove the unused ARCH_HAS_HMM_DEVICE Kconfig option mm: remove the struct hmm_device infrastructure mm: remove MEMORY_DEVICE_PUBLIC support mm: don't clear ->mapping in