Re: [PATCH v2 01/11] mm/hmm: select mmu notifier when selecting HMM
On 3/29/19 2:15 PM, Jerome Glisse wrote: [...] >> Yes, this is a good move, given that MMU notifiers are completely, >> indispensably part of the HMM design and implementation. >> >> The alternative would also work, but it's not quite as good. I'm >> listing it in order to forestall any debate: >> >> config HMM >> bool >> + depends on MMU_NOTIFIER >> select MIGRATE_VMA_HELPER >> >> ...and "depends on" versus "select" is always a subtle question. But in >> this case, I'd say that if someone wants HMM, there's no advantage in >> making them know that they must first ensure MMU_NOTIFIER is enabled. >> After poking around a bit I don't see any obvious downsides either. > > You can not depend on MMU_NOTIFIER it is one of the kernel config > option that is not selectable. So any config that need MMU_NOTIFIER > must select it. > aha, thanks for explaining that point about the non-user-selectable items, I wasn't aware of that. (I had convinced myself that those were set by hard-coding a choice in one of the Kconfig files.) >> >> However, given that you're making this change, in order to avoid odd >> redundancy, you should also do this: >> >> diff --git a/mm/Kconfig b/mm/Kconfig >> index 0d2944278d80..2e6d24d783f7 100644 >> --- a/mm/Kconfig >> +++ b/mm/Kconfig >> @@ -700,7 +700,6 @@ config HMM >> config HMM_MIRROR >> bool "HMM mirror CPU page table into a device page table" >> depends on ARCH_HAS_HMM >> - select MMU_NOTIFIER >> select HMM >> help >> Select HMM_MIRROR if you want to mirror range of the CPU page >> table of a > > Because it is a select option no harm can come from that hence i do > not remove but i can remove it. > Yes, this is just a tiny housecleaning point, not anything earthshaking. thanks, -- John Hubbard NVIDIA
Re: [PATCH v2 01/11] mm/hmm: select mmu notifier when selecting HMM
On Thu, Mar 28, 2019 at 01:33:42PM -0700, John Hubbard wrote: > On 3/25/19 7:40 AM, jgli...@redhat.com wrote: > > From: Jérôme Glisse > > > > To avoid random config build issue, select mmu notifier when HMM is > > selected. In any cases when HMM get selected it will be by users that > > will also wants the mmu notifier. > > > > Signed-off-by: Jérôme Glisse > > Acked-by: Balbir Singh > > Cc: Ralph Campbell > > Cc: Andrew Morton > > Cc: John Hubbard > > Cc: Dan Williams > > --- > > mm/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/mm/Kconfig b/mm/Kconfig > > index 25c71eb8a7db..0d2944278d80 100644 > > --- a/mm/Kconfig > > +++ b/mm/Kconfig > > @@ -694,6 +694,7 @@ config DEV_PAGEMAP_OPS > > > > config HMM > > bool > > + select MMU_NOTIFIER > > select MIGRATE_VMA_HELPER > > > > config HMM_MIRROR > > > > Yes, this is a good move, given that MMU notifiers are completely, > indispensably part of the HMM design and implementation. > > The alternative would also work, but it's not quite as good. I'm > listing it in order to forestall any debate: > > config HMM > bool > +depends on MMU_NOTIFIER > select MIGRATE_VMA_HELPER > > ...and "depends on" versus "select" is always a subtle question. But in > this case, I'd say that if someone wants HMM, there's no advantage in > making them know that they must first ensure MMU_NOTIFIER is enabled. > After poking around a bit I don't see any obvious downsides either. You can not depend on MMU_NOTIFIER it is one of the kernel config option that is not selectable. So any config that need MMU_NOTIFIER must select it. > > However, given that you're making this change, in order to avoid odd > redundancy, you should also do this: > > diff --git a/mm/Kconfig b/mm/Kconfig > index 0d2944278d80..2e6d24d783f7 100644 > --- a/mm/Kconfig > +++ b/mm/Kconfig > @@ -700,7 +700,6 @@ config HMM > config HMM_MIRROR > bool "HMM mirror CPU page table into a device page table" > depends on ARCH_HAS_HMM > - select MMU_NOTIFIER > select HMM > help > Select HMM_MIRROR if you want to mirror range of the CPU page table > of a Because it is a select option no harm can come from that hence i do not remove but i can remove it. Cheers, Jérôme
Re: [PATCH v2 01/11] mm/hmm: select mmu notifier when selecting HMM
On 3/25/19 7:40 AM, jgli...@redhat.com wrote: > From: Jérôme Glisse > > To avoid random config build issue, select mmu notifier when HMM is > selected. In any cases when HMM get selected it will be by users that > will also wants the mmu notifier. > > Signed-off-by: Jérôme Glisse > Acked-by: Balbir Singh > Cc: Ralph Campbell > Cc: Andrew Morton > Cc: John Hubbard > Cc: Dan Williams > --- > mm/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/Kconfig b/mm/Kconfig > index 25c71eb8a7db..0d2944278d80 100644 > --- a/mm/Kconfig > +++ b/mm/Kconfig > @@ -694,6 +694,7 @@ config DEV_PAGEMAP_OPS > > config HMM > bool > + select MMU_NOTIFIER > select MIGRATE_VMA_HELPER > > config HMM_MIRROR > Yes, this is a good move, given that MMU notifiers are completely, indispensably part of the HMM design and implementation. The alternative would also work, but it's not quite as good. I'm listing it in order to forestall any debate: config HMM bool + depends on MMU_NOTIFIER select MIGRATE_VMA_HELPER ...and "depends on" versus "select" is always a subtle question. But in this case, I'd say that if someone wants HMM, there's no advantage in making them know that they must first ensure MMU_NOTIFIER is enabled. After poking around a bit I don't see any obvious downsides either. However, given that you're making this change, in order to avoid odd redundancy, you should also do this: diff --git a/mm/Kconfig b/mm/Kconfig index 0d2944278d80..2e6d24d783f7 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -700,7 +700,6 @@ config HMM config HMM_MIRROR bool "HMM mirror CPU page table into a device page table" depends on ARCH_HAS_HMM - select MMU_NOTIFIER select HMM help Select HMM_MIRROR if you want to mirror range of the CPU page table of a thanks, -- John Hubbard NVIDIA
