Re: [PATCH 01/36] mmu_notifier: add event information to address invalidation v7
On Wed, 3 Jun 2015, Jerome Glisse wrote: > On Mon, Jun 01, 2015 at 04:10:46PM -0700, John Hubbard wrote: > > On Mon, 1 Jun 2015, Jerome Glisse wrote: > > > On Fri, May 29, 2015 at 08:43:59PM -0700, John Hubbard wrote: > > > > On Thu, 21 May 2015, j.gli...@gmail.com wrote: > > > > > From: Jérôme Glisse > > [...] > > > > We may have to add MMU_READ_WRITE (and maybe another one, I haven't > > > > bottomed out on that), if you agree with the above approach of > > > > always sending a precise event, instead of "protection changed". > > > > > > I think Linus point made sense last time, but i would need to read > > > again the thread. The idea of that patch is really to provide context > > > information on what kind of CPU page table changes is happening and > > > why. > > > > > > > Shoot, I tried to find that conversation, but my search foo is too weak. > > If you have a link to that thread, I'd appreciate it, so I can refresh my > > memory. > > > > I was hoping to re-read it and see if anything has changed. It's not > > really a huge problem to call find_vma() again, but I do want to be sure > > that there's a good reason for doing so. > > > > Otherwise, I'll just rely on your memory that Linus preferred your current > > approach, and call it good, then. > > http://lkml.iu.edu/hypermail/linux/kernel/1406.3/04880.html > > I am working on doing some of the changes discussed so far, i will push my > tree to git://people.freedesktop.org/~glisse/linux hmm branch once i am done. Aha, OK, that was back when you were passing around the vma. But now, you're not doing that anymore. It's just: mm*, range* (start, end, event_type), and sometimes page* and exclude*). So I think it's still reasonable to either pass down pure vma flags, or else add in new event types, in order to avoid having to lookup the vma later. We could still get NAK'd for adding ugly new event types, but if you're going to add the event types at all, let's make them complete, so that we really *earn* the NAK. :) > > Cheers, > Jérôme >
Re: [PATCH 01/36] mmu_notifier: add event information to address invalidation v7
On Mon, Jun 01, 2015 at 04:10:46PM -0700, John Hubbard wrote: > On Mon, 1 Jun 2015, Jerome Glisse wrote: > > On Fri, May 29, 2015 at 08:43:59PM -0700, John Hubbard wrote: > > > On Thu, 21 May 2015, j.gli...@gmail.com wrote: > > > > From: Jérôme Glisse [...] > > > > + MMU_ISDIRTY, > > > > > > This MMU_ISDIRTY seems like a problem to me. First of all, it looks > > > backwards: the only place that invokes it is the clear_refs_write() > > > routine, for the soft-dirty tracking feature. And in that case, the pages > > > are *not* being made dirty! Rather, the kernel is actually making the > > > pages non-writable, in order to be able to trap the subsequent page fault > > > and figure out if the page is in active use. > > > > > > So, given that there is only one call site, and that call site should > > > actually be setting MMU_WRITE_PROTECT instead (I think), let's just > > > delete > > > MMU_ISDIRTY. > > > > > > Come to think about it, there is no callback possible for "a page became > > > dirty", anyway. Because the dirty and accessed bits are actually set by > > > the hardware, and software is generally unable to know the current state. > > > So MMU_ISDIRTY just seems inappropriate to me, across the board. > > > > > > I'll take a look at the corresponding HMM_ISDIRTY, too. > > > > Ok i need to rename that one to CLEAR_SOFT_DIRTY, the idea is that > > for HMM i would rather not write protect the memory for the device > > and just rely on the regular and conservative dirtying of page. The > > soft dirty is really for migrating a process where you first clear > > the soft dirty bit, then copy memory while process still running, > > then freeze process an only copy memory that was dirtied since > > first copy. Point being that adding soft dirty to HMM is something > > that can be done down the road. We should have enough bit inside > > the device page table for that. > > > > Yes, I think renaming it to CLEAR_SOFT_DIRTY will definitely allow more > accurate behavior in response to these events. > > Looking ahead, a couple things: > > 1. This mechanism is also used for general memory utilization tracking (I > see that Vladimir DavyDov has an "idle memory tracking" proposal that > assumes this works, for example: https://lwn.net/Articles/642202/ and > https://lkml.org/lkml/2015/5/12/449). > > 2. It seems hard to avoid the need to eventually just write protect the > page, whether it is on the CPU or the remote device, if things like device > drivers or user space need to track write accesses to a virtual address. > Either you write protect the page, and trap the page faults, or you wait > until later and read the dirty bit (indirectly, via something like > unmap_mapping_range). Or did you have something else in mind? > > Anyway, none of that needs to hold up this part of the patchset, because > the renaming fixes things up for the future code to do the right thing. I will go over Vladimir patchset it was on my radar but haven't had yet a chance to go over it. We will likely need to do the write protect for device too. But as you said this is not an issue that this patch need a fix for, only HMM would need to change. I will do that. [...] > > > We may have to add MMU_READ_WRITE (and maybe another one, I haven't > > > bottomed out on that), if you agree with the above approach of > > > always sending a precise event, instead of "protection changed". > > > > I think Linus point made sense last time, but i would need to read > > again the thread. The idea of that patch is really to provide context > > information on what kind of CPU page table changes is happening and > > why. > > > > Shoot, I tried to find that conversation, but my search foo is too weak. > If you have a link to that thread, I'd appreciate it, so I can refresh my > memory. > > I was hoping to re-read it and see if anything has changed. It's not > really a huge problem to call find_vma() again, but I do want to be sure > that there's a good reason for doing so. > > Otherwise, I'll just rely on your memory that Linus preferred your current > approach, and call it good, then. http://lkml.iu.edu/hypermail/linux/kernel/1406.3/04880.html I am working on doing some of the changes discussed so far, i will push my tree to git://people.freedesktop.org/~glisse/linux hmm branch once i am done. Cheers, Jérôme -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/36] mmu_notifier: add event information to address invalidation v7
On Wed, 3 Jun 2015, Jerome Glisse wrote: On Mon, Jun 01, 2015 at 04:10:46PM -0700, John Hubbard wrote: On Mon, 1 Jun 2015, Jerome Glisse wrote: On Fri, May 29, 2015 at 08:43:59PM -0700, John Hubbard wrote: On Thu, 21 May 2015, j.gli...@gmail.com wrote: From: Jérôme Glisse jgli...@redhat.com [...] We may have to add MMU_READ_WRITE (and maybe another one, I haven't bottomed out on that), if you agree with the above approach of always sending a precise event, instead of protection changed. I think Linus point made sense last time, but i would need to read again the thread. The idea of that patch is really to provide context information on what kind of CPU page table changes is happening and why. Shoot, I tried to find that conversation, but my search foo is too weak. If you have a link to that thread, I'd appreciate it, so I can refresh my memory. I was hoping to re-read it and see if anything has changed. It's not really a huge problem to call find_vma() again, but I do want to be sure that there's a good reason for doing so. Otherwise, I'll just rely on your memory that Linus preferred your current approach, and call it good, then. http://lkml.iu.edu/hypermail/linux/kernel/1406.3/04880.html I am working on doing some of the changes discussed so far, i will push my tree to git://people.freedesktop.org/~glisse/linux hmm branch once i am done. Aha, OK, that was back when you were passing around the vma. But now, you're not doing that anymore. It's just: mm*, range* (start, end, event_type), and sometimes page* and exclude*). So I think it's still reasonable to either pass down pure vma flags, or else add in new event types, in order to avoid having to lookup the vma later. We could still get NAK'd for adding ugly new event types, but if you're going to add the event types at all, let's make them complete, so that we really *earn* the NAK. :) Cheers, Jérôme
Re: [PATCH 01/36] mmu_notifier: add event information to address invalidation v7
On Mon, Jun 01, 2015 at 04:10:46PM -0700, John Hubbard wrote: On Mon, 1 Jun 2015, Jerome Glisse wrote: On Fri, May 29, 2015 at 08:43:59PM -0700, John Hubbard wrote: On Thu, 21 May 2015, j.gli...@gmail.com wrote: From: Jérôme Glisse jgli...@redhat.com [...] + MMU_ISDIRTY, This MMU_ISDIRTY seems like a problem to me. First of all, it looks backwards: the only place that invokes it is the clear_refs_write() routine, for the soft-dirty tracking feature. And in that case, the pages are *not* being made dirty! Rather, the kernel is actually making the pages non-writable, in order to be able to trap the subsequent page fault and figure out if the page is in active use. So, given that there is only one call site, and that call site should actually be setting MMU_WRITE_PROTECT instead (I think), let's just delete MMU_ISDIRTY. Come to think about it, there is no callback possible for a page became dirty, anyway. Because the dirty and accessed bits are actually set by the hardware, and software is generally unable to know the current state. So MMU_ISDIRTY just seems inappropriate to me, across the board. I'll take a look at the corresponding HMM_ISDIRTY, too. Ok i need to rename that one to CLEAR_SOFT_DIRTY, the idea is that for HMM i would rather not write protect the memory for the device and just rely on the regular and conservative dirtying of page. The soft dirty is really for migrating a process where you first clear the soft dirty bit, then copy memory while process still running, then freeze process an only copy memory that was dirtied since first copy. Point being that adding soft dirty to HMM is something that can be done down the road. We should have enough bit inside the device page table for that. Yes, I think renaming it to CLEAR_SOFT_DIRTY will definitely allow more accurate behavior in response to these events. Looking ahead, a couple things: 1. This mechanism is also used for general memory utilization tracking (I see that Vladimir DavyDov has an idle memory tracking proposal that assumes this works, for example: https://lwn.net/Articles/642202/ and https://lkml.org/lkml/2015/5/12/449). 2. It seems hard to avoid the need to eventually just write protect the page, whether it is on the CPU or the remote device, if things like device drivers or user space need to track write accesses to a virtual address. Either you write protect the page, and trap the page faults, or you wait until later and read the dirty bit (indirectly, via something like unmap_mapping_range). Or did you have something else in mind? Anyway, none of that needs to hold up this part of the patchset, because the renaming fixes things up for the future code to do the right thing. I will go over Vladimir patchset it was on my radar but haven't had yet a chance to go over it. We will likely need to do the write protect for device too. But as you said this is not an issue that this patch need a fix for, only HMM would need to change. I will do that. [...] We may have to add MMU_READ_WRITE (and maybe another one, I haven't bottomed out on that), if you agree with the above approach of always sending a precise event, instead of protection changed. I think Linus point made sense last time, but i would need to read again the thread. The idea of that patch is really to provide context information on what kind of CPU page table changes is happening and why. Shoot, I tried to find that conversation, but my search foo is too weak. If you have a link to that thread, I'd appreciate it, so I can refresh my memory. I was hoping to re-read it and see if anything has changed. It's not really a huge problem to call find_vma() again, but I do want to be sure that there's a good reason for doing so. Otherwise, I'll just rely on your memory that Linus preferred your current approach, and call it good, then. http://lkml.iu.edu/hypermail/linux/kernel/1406.3/04880.html I am working on doing some of the changes discussed so far, i will push my tree to git://people.freedesktop.org/~glisse/linux hmm branch once i am done. Cheers, Jérôme -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/36] mmu_notifier: add event information to address invalidation v7
On Mon, 1 Jun 2015, Jerome Glisse wrote: > On Fri, May 29, 2015 at 08:43:59PM -0700, John Hubbard wrote: > > On Thu, 21 May 2015, j.gli...@gmail.com wrote: > > > > > From: Jérôme Glisse > > > > > > The event information will be useful for new user of mmu_notifier API. > > > The event argument differentiate between a vma disappearing, a page > > > being write protected or simply a page being unmaped. This allow new > > > user to take different path for different event for instance on unmap > > > the resource used to track a vma are still valid and should stay around. > > > While if the event is saying that a vma is being destroy it means that any > > > resources used to track this vma can be free. > > > > > > Changed since v1: > > > - renamed action into event (updated commit message too). > > > - simplified the event names and clarified their usage > > > also documenting what exceptation the listener can have in > > > respect to each event. > > > > > > Changed since v2: > > > - Avoid crazy name. > > > - Do not move code that do not need to move. > > > > > > Changed since v3: > > > - Separate hugue page split from mlock/munlock and softdirty. > > > > Do we care about fixing up patch comments? If so: > > > > s/hugue/huge/ > > I am noting them down and will go over them. > > > [...] > > > + MMU_HSPLIT, > > > > Let's rename MMU_HSPLIT to one of the following, take your pick: > > > > MMU_HUGE_PAGE_SPLIT (too long, but you can't possibly misunderstand it) > > MMU_PAGE_SPLIT (my favorite: only huge pages are ever split, so it works) > > MMU_HUGE_SPLIT (ugly, but still hard to misunderstand) > > I will go with MMU_HUGE_PAGE_SPLIT > > > [...] > > > > > + MMU_ISDIRTY, > > > > This MMU_ISDIRTY seems like a problem to me. First of all, it looks > > backwards: the only place that invokes it is the clear_refs_write() > > routine, for the soft-dirty tracking feature. And in that case, the pages > > are *not* being made dirty! Rather, the kernel is actually making the > > pages non-writable, in order to be able to trap the subsequent page fault > > and figure out if the page is in active use. > > > > So, given that there is only one call site, and that call site should > > actually be setting MMU_WRITE_PROTECT instead (I think), let's just delete > > MMU_ISDIRTY. > > > > Come to think about it, there is no callback possible for "a page became > > dirty", anyway. Because the dirty and accessed bits are actually set by > > the hardware, and software is generally unable to know the current state. > > So MMU_ISDIRTY just seems inappropriate to me, across the board. > > > > I'll take a look at the corresponding HMM_ISDIRTY, too. > > Ok i need to rename that one to CLEAR_SOFT_DIRTY, the idea is that > for HMM i would rather not write protect the memory for the device > and just rely on the regular and conservative dirtying of page. The > soft dirty is really for migrating a process where you first clear > the soft dirty bit, then copy memory while process still running, > then freeze process an only copy memory that was dirtied since > first copy. Point being that adding soft dirty to HMM is something > that can be done down the road. We should have enough bit inside > the device page table for that. > Yes, I think renaming it to CLEAR_SOFT_DIRTY will definitely allow more accurate behavior in response to these events. Looking ahead, a couple things: 1. This mechanism is also used for general memory utilization tracking (I see that Vladimir DavyDov has an "idle memory tracking" proposal that assumes this works, for example: https://lwn.net/Articles/642202/ and https://lkml.org/lkml/2015/5/12/449). 2. It seems hard to avoid the need to eventually just write protect the page, whether it is on the CPU or the remote device, if things like device drivers or user space need to track write accesses to a virtual address. Either you write protect the page, and trap the page faults, or you wait until later and read the dirty bit (indirectly, via something like unmap_mapping_range). Or did you have something else in mind? Anyway, none of that needs to hold up this part of the patchset, because the renaming fixes things up for the future code to do the right thing. > > > > > > + MMU_MIGRATE, > > > + MMU_MPROT, > > > > The MMU_PROT also looks questionable. Short answer: probably better to > > read the protection, and pass either MMU_WRITE_PROTECT, MMU_READ_WRITE > > (that's a new item, of course), or MMU_UNMAP. > > > > Here's why: the call site knows the protection, but by the time it filters > > down to HMM (in later patches), that information is lost, and HMM ends up > > doing (ouch!) another find_vma() call in order to retrieve it--and then > > translates it into only three possible things: > > > > // hmm_mmu_mprot_to_etype() sets one of these: > > > >HMM_MUNMAP > >HMM_WRITE_PROTECT > >HMM_NONE > > Linus complained of my previous version where i
Re: [PATCH 01/36] mmu_notifier: add event information to address invalidation v7
On Fri, May 29, 2015 at 08:43:59PM -0700, John Hubbard wrote: > On Thu, 21 May 2015, j.gli...@gmail.com wrote: > > > From: Jérôme Glisse > > > > The event information will be useful for new user of mmu_notifier API. > > The event argument differentiate between a vma disappearing, a page > > being write protected or simply a page being unmaped. This allow new > > user to take different path for different event for instance on unmap > > the resource used to track a vma are still valid and should stay around. > > While if the event is saying that a vma is being destroy it means that any > > resources used to track this vma can be free. > > > > Changed since v1: > > - renamed action into event (updated commit message too). > > - simplified the event names and clarified their usage > > also documenting what exceptation the listener can have in > > respect to each event. > > > > Changed since v2: > > - Avoid crazy name. > > - Do not move code that do not need to move. > > > > Changed since v3: > > - Separate hugue page split from mlock/munlock and softdirty. > > Do we care about fixing up patch comments? If so: > > s/hugue/huge/ I am noting them down and will go over them. [...] > > + MMU_HSPLIT, > > Let's rename MMU_HSPLIT to one of the following, take your pick: > > MMU_HUGE_PAGE_SPLIT (too long, but you can't possibly misunderstand it) > MMU_PAGE_SPLIT (my favorite: only huge pages are ever split, so it works) > MMU_HUGE_SPLIT (ugly, but still hard to misunderstand) I will go with MMU_HUGE_PAGE_SPLIT [...] > > > + MMU_ISDIRTY, > > This MMU_ISDIRTY seems like a problem to me. First of all, it looks > backwards: the only place that invokes it is the clear_refs_write() > routine, for the soft-dirty tracking feature. And in that case, the pages > are *not* being made dirty! Rather, the kernel is actually making the > pages non-writable, in order to be able to trap the subsequent page fault > and figure out if the page is in active use. > > So, given that there is only one call site, and that call site should > actually be setting MMU_WRITE_PROTECT instead (I think), let's just delete > MMU_ISDIRTY. > > Come to think about it, there is no callback possible for "a page became > dirty", anyway. Because the dirty and accessed bits are actually set by > the hardware, and software is generally unable to know the current state. > So MMU_ISDIRTY just seems inappropriate to me, across the board. > > I'll take a look at the corresponding HMM_ISDIRTY, too. Ok i need to rename that one to CLEAR_SOFT_DIRTY, the idea is that for HMM i would rather not write protect the memory for the device and just rely on the regular and conservative dirtying of page. The soft dirty is really for migrating a process where you first clear the soft dirty bit, then copy memory while process still running, then freeze process an only copy memory that was dirtied since first copy. Point being that adding soft dirty to HMM is something that can be done down the road. We should have enough bit inside the device page table for that. > > > + MMU_MIGRATE, > > + MMU_MPROT, > > The MMU_PROT also looks questionable. Short answer: probably better to > read the protection, and pass either MMU_WRITE_PROTECT, MMU_READ_WRITE > (that's a new item, of course), or MMU_UNMAP. > > Here's why: the call site knows the protection, but by the time it filters > down to HMM (in later patches), that information is lost, and HMM ends up > doing (ouch!) another find_vma() call in order to retrieve it--and then > translates it into only three possible things: > > // hmm_mmu_mprot_to_etype() sets one of these: > >HMM_MUNMAP >HMM_WRITE_PROTECT >HMM_NONE Linus complained of my previous version where i differenciated the kind of protection change that was happening, hence why i only pass down mprot. > > > > + MMU_MUNLOCK, > > I think MMU_UNLOCK would be clearer. We already know the scope, so the > extra "M" isn't adding anything. I named it that way so it matches syscall name munlock(). I think it is clearer to use MUNLOCK, or maybe SYSCALL_MUNLOCK > > > + MMU_MUNMAP, > > Same thing here: MMU_UNMAP seems better. Well same idea here. > > > + MMU_WRITE_BACK, > > + MMU_WRITE_PROTECT, > > We may have to add MMU_READ_WRITE (and maybe another one, I haven't > bottomed out on that), if you agree with the above approach of > always sending a precise event, instead of "protection changed". I think Linus point made sense last time, but i would need to read again the thread. The idea of that patch is really to provide context information on what kind of CPU page table changes is happening and why. In that respect i should probably change MMU_WRITE_PROTECT to MMU_KSM_WRITE_PROTECT. Cheers, Jérôme -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at
Re: [PATCH 01/36] mmu_notifier: add event information to address invalidation v7
On Fri, May 29, 2015 at 08:43:59PM -0700, John Hubbard wrote: On Thu, 21 May 2015, j.gli...@gmail.com wrote: From: Jérôme Glisse jgli...@redhat.com The event information will be useful for new user of mmu_notifier API. The event argument differentiate between a vma disappearing, a page being write protected or simply a page being unmaped. This allow new user to take different path for different event for instance on unmap the resource used to track a vma are still valid and should stay around. While if the event is saying that a vma is being destroy it means that any resources used to track this vma can be free. Changed since v1: - renamed action into event (updated commit message too). - simplified the event names and clarified their usage also documenting what exceptation the listener can have in respect to each event. Changed since v2: - Avoid crazy name. - Do not move code that do not need to move. Changed since v3: - Separate hugue page split from mlock/munlock and softdirty. Do we care about fixing up patch comments? If so: s/hugue/huge/ I am noting them down and will go over them. [...] + MMU_HSPLIT, Let's rename MMU_HSPLIT to one of the following, take your pick: MMU_HUGE_PAGE_SPLIT (too long, but you can't possibly misunderstand it) MMU_PAGE_SPLIT (my favorite: only huge pages are ever split, so it works) MMU_HUGE_SPLIT (ugly, but still hard to misunderstand) I will go with MMU_HUGE_PAGE_SPLIT [...] + MMU_ISDIRTY, This MMU_ISDIRTY seems like a problem to me. First of all, it looks backwards: the only place that invokes it is the clear_refs_write() routine, for the soft-dirty tracking feature. And in that case, the pages are *not* being made dirty! Rather, the kernel is actually making the pages non-writable, in order to be able to trap the subsequent page fault and figure out if the page is in active use. So, given that there is only one call site, and that call site should actually be setting MMU_WRITE_PROTECT instead (I think), let's just delete MMU_ISDIRTY. Come to think about it, there is no callback possible for a page became dirty, anyway. Because the dirty and accessed bits are actually set by the hardware, and software is generally unable to know the current state. So MMU_ISDIRTY just seems inappropriate to me, across the board. I'll take a look at the corresponding HMM_ISDIRTY, too. Ok i need to rename that one to CLEAR_SOFT_DIRTY, the idea is that for HMM i would rather not write protect the memory for the device and just rely on the regular and conservative dirtying of page. The soft dirty is really for migrating a process where you first clear the soft dirty bit, then copy memory while process still running, then freeze process an only copy memory that was dirtied since first copy. Point being that adding soft dirty to HMM is something that can be done down the road. We should have enough bit inside the device page table for that. + MMU_MIGRATE, + MMU_MPROT, The MMU_PROT also looks questionable. Short answer: probably better to read the protection, and pass either MMU_WRITE_PROTECT, MMU_READ_WRITE (that's a new item, of course), or MMU_UNMAP. Here's why: the call site knows the protection, but by the time it filters down to HMM (in later patches), that information is lost, and HMM ends up doing (ouch!) another find_vma() call in order to retrieve it--and then translates it into only three possible things: // hmm_mmu_mprot_to_etype() sets one of these: HMM_MUNMAP HMM_WRITE_PROTECT HMM_NONE Linus complained of my previous version where i differenciated the kind of protection change that was happening, hence why i only pass down mprot. + MMU_MUNLOCK, I think MMU_UNLOCK would be clearer. We already know the scope, so the extra M isn't adding anything. I named it that way so it matches syscall name munlock(). I think it is clearer to use MUNLOCK, or maybe SYSCALL_MUNLOCK + MMU_MUNMAP, Same thing here: MMU_UNMAP seems better. Well same idea here. + MMU_WRITE_BACK, + MMU_WRITE_PROTECT, We may have to add MMU_READ_WRITE (and maybe another one, I haven't bottomed out on that), if you agree with the above approach of always sending a precise event, instead of protection changed. I think Linus point made sense last time, but i would need to read again the thread. The idea of that patch is really to provide context information on what kind of CPU page table changes is happening and why. In that respect i should probably change MMU_WRITE_PROTECT to MMU_KSM_WRITE_PROTECT. Cheers, Jérôme -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/36] mmu_notifier: add event information to address invalidation v7
On Mon, 1 Jun 2015, Jerome Glisse wrote: On Fri, May 29, 2015 at 08:43:59PM -0700, John Hubbard wrote: On Thu, 21 May 2015, j.gli...@gmail.com wrote: From: Jérôme Glisse jgli...@redhat.com The event information will be useful for new user of mmu_notifier API. The event argument differentiate between a vma disappearing, a page being write protected or simply a page being unmaped. This allow new user to take different path for different event for instance on unmap the resource used to track a vma are still valid and should stay around. While if the event is saying that a vma is being destroy it means that any resources used to track this vma can be free. Changed since v1: - renamed action into event (updated commit message too). - simplified the event names and clarified their usage also documenting what exceptation the listener can have in respect to each event. Changed since v2: - Avoid crazy name. - Do not move code that do not need to move. Changed since v3: - Separate hugue page split from mlock/munlock and softdirty. Do we care about fixing up patch comments? If so: s/hugue/huge/ I am noting them down and will go over them. [...] + MMU_HSPLIT, Let's rename MMU_HSPLIT to one of the following, take your pick: MMU_HUGE_PAGE_SPLIT (too long, but you can't possibly misunderstand it) MMU_PAGE_SPLIT (my favorite: only huge pages are ever split, so it works) MMU_HUGE_SPLIT (ugly, but still hard to misunderstand) I will go with MMU_HUGE_PAGE_SPLIT [...] + MMU_ISDIRTY, This MMU_ISDIRTY seems like a problem to me. First of all, it looks backwards: the only place that invokes it is the clear_refs_write() routine, for the soft-dirty tracking feature. And in that case, the pages are *not* being made dirty! Rather, the kernel is actually making the pages non-writable, in order to be able to trap the subsequent page fault and figure out if the page is in active use. So, given that there is only one call site, and that call site should actually be setting MMU_WRITE_PROTECT instead (I think), let's just delete MMU_ISDIRTY. Come to think about it, there is no callback possible for a page became dirty, anyway. Because the dirty and accessed bits are actually set by the hardware, and software is generally unable to know the current state. So MMU_ISDIRTY just seems inappropriate to me, across the board. I'll take a look at the corresponding HMM_ISDIRTY, too. Ok i need to rename that one to CLEAR_SOFT_DIRTY, the idea is that for HMM i would rather not write protect the memory for the device and just rely on the regular and conservative dirtying of page. The soft dirty is really for migrating a process where you first clear the soft dirty bit, then copy memory while process still running, then freeze process an only copy memory that was dirtied since first copy. Point being that adding soft dirty to HMM is something that can be done down the road. We should have enough bit inside the device page table for that. Yes, I think renaming it to CLEAR_SOFT_DIRTY will definitely allow more accurate behavior in response to these events. Looking ahead, a couple things: 1. This mechanism is also used for general memory utilization tracking (I see that Vladimir DavyDov has an idle memory tracking proposal that assumes this works, for example: https://lwn.net/Articles/642202/ and https://lkml.org/lkml/2015/5/12/449). 2. It seems hard to avoid the need to eventually just write protect the page, whether it is on the CPU or the remote device, if things like device drivers or user space need to track write accesses to a virtual address. Either you write protect the page, and trap the page faults, or you wait until later and read the dirty bit (indirectly, via something like unmap_mapping_range). Or did you have something else in mind? Anyway, none of that needs to hold up this part of the patchset, because the renaming fixes things up for the future code to do the right thing. + MMU_MIGRATE, + MMU_MPROT, The MMU_PROT also looks questionable. Short answer: probably better to read the protection, and pass either MMU_WRITE_PROTECT, MMU_READ_WRITE (that's a new item, of course), or MMU_UNMAP. Here's why: the call site knows the protection, but by the time it filters down to HMM (in later patches), that information is lost, and HMM ends up doing (ouch!) another find_vma() call in order to retrieve it--and then translates it into only three possible things: // hmm_mmu_mprot_to_etype() sets one of these: HMM_MUNMAP HMM_WRITE_PROTECT HMM_NONE Linus complained of my previous version where i differenciated the kind of protection change that was happening, hence why i only pass down mprot. + MMU_MUNLOCK, I think MMU_UNLOCK would be clearer. We already
Re: [PATCH 01/36] mmu_notifier: add event information to address invalidation v7
On Thu, 21 May 2015, j.gli...@gmail.com wrote:
> From: Jérôme Glisse
>
> The event information will be useful for new user of mmu_notifier API.
> The event argument differentiate between a vma disappearing, a page
> being write protected or simply a page being unmaped. This allow new
> user to take different path for different event for instance on unmap
> the resource used to track a vma are still valid and should stay around.
> While if the event is saying that a vma is being destroy it means that any
> resources used to track this vma can be free.
>
> Changed since v1:
> - renamed action into event (updated commit message too).
> - simplified the event names and clarified their usage
> also documenting what exceptation the listener can have in
> respect to each event.
>
> Changed since v2:
> - Avoid crazy name.
> - Do not move code that do not need to move.
>
> Changed since v3:
> - Separate hugue page split from mlock/munlock and softdirty.
Do we care about fixing up patch comments? If so:
s/hugue/huge/
>
> Changed since v4:
> - Rebase (no other changes).
>
> Changed since v5:
> - Typo fix.
> - Changed zap_page_range from MMU_MUNMAP to MMU_MIGRATE to reflect the
> fact that the address range is still valid just the page backing it
> are no longer.
>
> Changed since v6:
> - try_to_unmap_one() only invalidate when doing migration.
> - Differentiate fork from other case.
>
> Signed-off-by: Jérôme Glisse
> Reviewed-by: Rik van Riel
> ---
> drivers/gpu/drm/i915/i915_gem_userptr.c | 3 +-
> drivers/gpu/drm/radeon/radeon_mn.c | 3 +-
> drivers/infiniband/core/umem_odp.c | 9 ++-
> drivers/iommu/amd_iommu_v2.c| 3 +-
> drivers/misc/sgi-gru/grutlbpurge.c | 9 ++-
> drivers/xen/gntdev.c| 9 ++-
> fs/proc/task_mmu.c | 6 +-
> include/linux/mmu_notifier.h| 135
> ++--
> kernel/events/uprobes.c | 10 ++-
> mm/huge_memory.c| 39 ++---
> mm/hugetlb.c| 23 +++---
> mm/ksm.c| 18 +++--
> mm/madvise.c| 4 +-
> mm/memory.c | 27 ---
> mm/migrate.c| 9 ++-
> mm/mmu_notifier.c | 28 ---
> mm/mprotect.c | 6 +-
> mm/mremap.c | 6 +-
> mm/rmap.c | 4 +-
> virt/kvm/kvm_main.c | 12 ++-
> 20 files changed, 261 insertions(+), 102 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c
> b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 4039ede..452e9b1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -132,7 +132,8 @@ restart:
> static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier
> *_mn,
> struct mm_struct *mm,
> unsigned long start,
> -unsigned long end)
> +unsigned long end,
> +enum mmu_event event)
> {
> struct i915_mmu_notifier *mn = container_of(_mn, struct
> i915_mmu_notifier, mn);
> struct interval_tree_node *it = NULL;
> diff --git a/drivers/gpu/drm/radeon/radeon_mn.c
> b/drivers/gpu/drm/radeon/radeon_mn.c
> index eef006c..3a9615b 100644
> --- a/drivers/gpu/drm/radeon/radeon_mn.c
> +++ b/drivers/gpu/drm/radeon/radeon_mn.c
> @@ -121,7 +121,8 @@ static void radeon_mn_release(struct mmu_notifier *mn,
> static void radeon_mn_invalidate_range_start(struct mmu_notifier *mn,
>struct mm_struct *mm,
>unsigned long start,
> - unsigned long end)
> + unsigned long end,
> + enum mmu_event event)
> {
> struct radeon_mn *rmn = container_of(mn, struct radeon_mn, mn);
> struct interval_tree_node *it;
> diff --git a/drivers/infiniband/core/umem_odp.c
> b/drivers/infiniband/core/umem_odp.c
> index 40becdb..6ed69fa 100644
> --- a/drivers/infiniband/core/umem_odp.c
> +++ b/drivers/infiniband/core/umem_odp.c
> @@ -165,7 +165,8 @@ static int invalidate_page_trampoline(struct ib_umem
> *item, u64 start,
>
> static void ib_umem_notifier_invalidate_page(struct mmu_notifier *mn,
>struct mm_struct *mm,
> - unsigned long address)
> + unsigned long address,
> + enum mmu_event event)
>
Re: [PATCH 01/36] mmu_notifier: add event information to address invalidation v7
On Thu, 21 May 2015, j.gli...@gmail.com wrote:
From: Jérôme Glisse jgli...@redhat.com
The event information will be useful for new user of mmu_notifier API.
The event argument differentiate between a vma disappearing, a page
being write protected or simply a page being unmaped. This allow new
user to take different path for different event for instance on unmap
the resource used to track a vma are still valid and should stay around.
While if the event is saying that a vma is being destroy it means that any
resources used to track this vma can be free.
Changed since v1:
- renamed action into event (updated commit message too).
- simplified the event names and clarified their usage
also documenting what exceptation the listener can have in
respect to each event.
Changed since v2:
- Avoid crazy name.
- Do not move code that do not need to move.
Changed since v3:
- Separate hugue page split from mlock/munlock and softdirty.
Do we care about fixing up patch comments? If so:
s/hugue/huge/
Changed since v4:
- Rebase (no other changes).
Changed since v5:
- Typo fix.
- Changed zap_page_range from MMU_MUNMAP to MMU_MIGRATE to reflect the
fact that the address range is still valid just the page backing it
are no longer.
Changed since v6:
- try_to_unmap_one() only invalidate when doing migration.
- Differentiate fork from other case.
Signed-off-by: Jérôme Glisse jgli...@redhat.com
Reviewed-by: Rik van Riel r...@redhat.com
---
drivers/gpu/drm/i915/i915_gem_userptr.c | 3 +-
drivers/gpu/drm/radeon/radeon_mn.c | 3 +-
drivers/infiniband/core/umem_odp.c | 9 ++-
drivers/iommu/amd_iommu_v2.c| 3 +-
drivers/misc/sgi-gru/grutlbpurge.c | 9 ++-
drivers/xen/gntdev.c| 9 ++-
fs/proc/task_mmu.c | 6 +-
include/linux/mmu_notifier.h| 135
++--
kernel/events/uprobes.c | 10 ++-
mm/huge_memory.c| 39 ++---
mm/hugetlb.c| 23 +++---
mm/ksm.c| 18 +++--
mm/madvise.c| 4 +-
mm/memory.c | 27 ---
mm/migrate.c| 9 ++-
mm/mmu_notifier.c | 28 ---
mm/mprotect.c | 6 +-
mm/mremap.c | 6 +-
mm/rmap.c | 4 +-
virt/kvm/kvm_main.c | 12 ++-
20 files changed, 261 insertions(+), 102 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c
b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 4039ede..452e9b1 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -132,7 +132,8 @@ restart:
static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier
*_mn,
struct mm_struct *mm,
unsigned long start,
-unsigned long end)
+unsigned long end,
+enum mmu_event event)
{
struct i915_mmu_notifier *mn = container_of(_mn, struct
i915_mmu_notifier, mn);
struct interval_tree_node *it = NULL;
diff --git a/drivers/gpu/drm/radeon/radeon_mn.c
b/drivers/gpu/drm/radeon/radeon_mn.c
index eef006c..3a9615b 100644
--- a/drivers/gpu/drm/radeon/radeon_mn.c
+++ b/drivers/gpu/drm/radeon/radeon_mn.c
@@ -121,7 +121,8 @@ static void radeon_mn_release(struct mmu_notifier *mn,
static void radeon_mn_invalidate_range_start(struct mmu_notifier *mn,
struct mm_struct *mm,
unsigned long start,
- unsigned long end)
+ unsigned long end,
+ enum mmu_event event)
{
struct radeon_mn *rmn = container_of(mn, struct radeon_mn, mn);
struct interval_tree_node *it;
diff --git a/drivers/infiniband/core/umem_odp.c
b/drivers/infiniband/core/umem_odp.c
index 40becdb..6ed69fa 100644
--- a/drivers/infiniband/core/umem_odp.c
+++ b/drivers/infiniband/core/umem_odp.c
@@ -165,7 +165,8 @@ static int invalidate_page_trampoline(struct ib_umem
*item, u64 start,
static void ib_umem_notifier_invalidate_page(struct mmu_notifier *mn,
struct mm_struct *mm,
- unsigned long address)
+ unsigned long address,
+ enum mmu_event event)
{
struct ib_ucontext *context = container_of(mn,
[PATCH 01/36] mmu_notifier: add event information to address invalidation v7
From: Jérôme Glisse
The event information will be useful for new user of mmu_notifier API.
The event argument differentiate between a vma disappearing, a page
being write protected or simply a page being unmaped. This allow new
user to take different path for different event for instance on unmap
the resource used to track a vma are still valid and should stay around.
While if the event is saying that a vma is being destroy it means that any
resources used to track this vma can be free.
Changed since v1:
- renamed action into event (updated commit message too).
- simplified the event names and clarified their usage
also documenting what exceptation the listener can have in
respect to each event.
Changed since v2:
- Avoid crazy name.
- Do not move code that do not need to move.
Changed since v3:
- Separate hugue page split from mlock/munlock and softdirty.
Changed since v4:
- Rebase (no other changes).
Changed since v5:
- Typo fix.
- Changed zap_page_range from MMU_MUNMAP to MMU_MIGRATE to reflect the
fact that the address range is still valid just the page backing it
are no longer.
Changed since v6:
- try_to_unmap_one() only invalidate when doing migration.
- Differentiate fork from other case.
Signed-off-by: Jérôme Glisse
Reviewed-by: Rik van Riel
---
drivers/gpu/drm/i915/i915_gem_userptr.c | 3 +-
drivers/gpu/drm/radeon/radeon_mn.c | 3 +-
drivers/infiniband/core/umem_odp.c | 9 ++-
drivers/iommu/amd_iommu_v2.c| 3 +-
drivers/misc/sgi-gru/grutlbpurge.c | 9 ++-
drivers/xen/gntdev.c| 9 ++-
fs/proc/task_mmu.c | 6 +-
include/linux/mmu_notifier.h| 135 ++--
kernel/events/uprobes.c | 10 ++-
mm/huge_memory.c| 39 ++---
mm/hugetlb.c| 23 +++---
mm/ksm.c| 18 +++--
mm/madvise.c| 4 +-
mm/memory.c | 27 ---
mm/migrate.c| 9 ++-
mm/mmu_notifier.c | 28 ---
mm/mprotect.c | 6 +-
mm/mremap.c | 6 +-
mm/rmap.c | 4 +-
virt/kvm/kvm_main.c | 12 ++-
20 files changed, 261 insertions(+), 102 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c
b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 4039ede..452e9b1 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -132,7 +132,8 @@ restart:
static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier
*_mn,
struct mm_struct *mm,
unsigned long start,
- unsigned long end)
+ unsigned long end,
+ enum mmu_event event)
{
struct i915_mmu_notifier *mn = container_of(_mn, struct
i915_mmu_notifier, mn);
struct interval_tree_node *it = NULL;
diff --git a/drivers/gpu/drm/radeon/radeon_mn.c
b/drivers/gpu/drm/radeon/radeon_mn.c
index eef006c..3a9615b 100644
--- a/drivers/gpu/drm/radeon/radeon_mn.c
+++ b/drivers/gpu/drm/radeon/radeon_mn.c
@@ -121,7 +121,8 @@ static void radeon_mn_release(struct mmu_notifier *mn,
static void radeon_mn_invalidate_range_start(struct mmu_notifier *mn,
struct mm_struct *mm,
unsigned long start,
-unsigned long end)
+unsigned long end,
+enum mmu_event event)
{
struct radeon_mn *rmn = container_of(mn, struct radeon_mn, mn);
struct interval_tree_node *it;
diff --git a/drivers/infiniband/core/umem_odp.c
b/drivers/infiniband/core/umem_odp.c
index 40becdb..6ed69fa 100644
--- a/drivers/infiniband/core/umem_odp.c
+++ b/drivers/infiniband/core/umem_odp.c
@@ -165,7 +165,8 @@ static int invalidate_page_trampoline(struct ib_umem *item,
u64 start,
static void ib_umem_notifier_invalidate_page(struct mmu_notifier *mn,
struct mm_struct *mm,
-unsigned long address)
+unsigned long address,
+enum mmu_event event)
{
struct ib_ucontext *context = container_of(mn, struct ib_ucontext, mn);
@@ -192,7 +193,8 @@ static int invalidate_range_start_trampoline(struct ib_umem
*item, u64 start,
static void ib_umem_notifier_invalidate_range_start(struct mmu_notifier *mn,
[PATCH 01/36] mmu_notifier: add event information to address invalidation v7
From: Jérôme Glisse jgli...@redhat.com
The event information will be useful for new user of mmu_notifier API.
The event argument differentiate between a vma disappearing, a page
being write protected or simply a page being unmaped. This allow new
user to take different path for different event for instance on unmap
the resource used to track a vma are still valid and should stay around.
While if the event is saying that a vma is being destroy it means that any
resources used to track this vma can be free.
Changed since v1:
- renamed action into event (updated commit message too).
- simplified the event names and clarified their usage
also documenting what exceptation the listener can have in
respect to each event.
Changed since v2:
- Avoid crazy name.
- Do not move code that do not need to move.
Changed since v3:
- Separate hugue page split from mlock/munlock and softdirty.
Changed since v4:
- Rebase (no other changes).
Changed since v5:
- Typo fix.
- Changed zap_page_range from MMU_MUNMAP to MMU_MIGRATE to reflect the
fact that the address range is still valid just the page backing it
are no longer.
Changed since v6:
- try_to_unmap_one() only invalidate when doing migration.
- Differentiate fork from other case.
Signed-off-by: Jérôme Glisse jgli...@redhat.com
Reviewed-by: Rik van Riel r...@redhat.com
---
drivers/gpu/drm/i915/i915_gem_userptr.c | 3 +-
drivers/gpu/drm/radeon/radeon_mn.c | 3 +-
drivers/infiniband/core/umem_odp.c | 9 ++-
drivers/iommu/amd_iommu_v2.c| 3 +-
drivers/misc/sgi-gru/grutlbpurge.c | 9 ++-
drivers/xen/gntdev.c| 9 ++-
fs/proc/task_mmu.c | 6 +-
include/linux/mmu_notifier.h| 135 ++--
kernel/events/uprobes.c | 10 ++-
mm/huge_memory.c| 39 ++---
mm/hugetlb.c| 23 +++---
mm/ksm.c| 18 +++--
mm/madvise.c| 4 +-
mm/memory.c | 27 ---
mm/migrate.c| 9 ++-
mm/mmu_notifier.c | 28 ---
mm/mprotect.c | 6 +-
mm/mremap.c | 6 +-
mm/rmap.c | 4 +-
virt/kvm/kvm_main.c | 12 ++-
20 files changed, 261 insertions(+), 102 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c
b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 4039ede..452e9b1 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -132,7 +132,8 @@ restart:
static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier
*_mn,
struct mm_struct *mm,
unsigned long start,
- unsigned long end)
+ unsigned long end,
+ enum mmu_event event)
{
struct i915_mmu_notifier *mn = container_of(_mn, struct
i915_mmu_notifier, mn);
struct interval_tree_node *it = NULL;
diff --git a/drivers/gpu/drm/radeon/radeon_mn.c
b/drivers/gpu/drm/radeon/radeon_mn.c
index eef006c..3a9615b 100644
--- a/drivers/gpu/drm/radeon/radeon_mn.c
+++ b/drivers/gpu/drm/radeon/radeon_mn.c
@@ -121,7 +121,8 @@ static void radeon_mn_release(struct mmu_notifier *mn,
static void radeon_mn_invalidate_range_start(struct mmu_notifier *mn,
struct mm_struct *mm,
unsigned long start,
-unsigned long end)
+unsigned long end,
+enum mmu_event event)
{
struct radeon_mn *rmn = container_of(mn, struct radeon_mn, mn);
struct interval_tree_node *it;
diff --git a/drivers/infiniband/core/umem_odp.c
b/drivers/infiniband/core/umem_odp.c
index 40becdb..6ed69fa 100644
--- a/drivers/infiniband/core/umem_odp.c
+++ b/drivers/infiniband/core/umem_odp.c
@@ -165,7 +165,8 @@ static int invalidate_page_trampoline(struct ib_umem *item,
u64 start,
static void ib_umem_notifier_invalidate_page(struct mmu_notifier *mn,
struct mm_struct *mm,
-unsigned long address)
+unsigned long address,
+enum mmu_event event)
{
struct ib_ucontext *context = container_of(mn, struct ib_ucontext, mn);
@@ -192,7 +193,8 @@ static int invalidate_range_start_trampoline(struct ib_umem
*item, u64 start,
static void ib_umem_notifier_invalidate_range_start(struct
