[RFC,v2,08/47] hugetlb: add HGM enablement functions

  Currently it is possible for all shared VMAs to use HGM, but it must be
enabled first. This is because with HGM, we lose PMD sharing, and page
table walks require additional synchronization (we need to take the VMA
lock).

Signed-off-by: James Houghton <jthoughton@google.com>
---
 include/linux/hugetlb.h | 22 +++++++++++++
 mm/hugetlb.c            | 69 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)
  

On Fri, Oct 21, 2022 at 04:36:24PM +0000, James Houghton wrote:
> Currently it is possible for all shared VMAs to use HGM, but it must be
> enabled first. This is because with HGM, we lose PMD sharing, and page
> table walks require additional synchronization (we need to take the VMA
> lock).
> 
> Signed-off-by: James Houghton <jthoughton@google.com>
> ---
>  include/linux/hugetlb.h | 22 +++++++++++++
>  mm/hugetlb.c            | 69 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 91 insertions(+)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 534958499ac4..6e0c36b08a0c 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -123,6 +123,9 @@ struct hugetlb_vma_lock {
>  
>  struct hugetlb_shared_vma_data {
>  	struct hugetlb_vma_lock vma_lock;
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +	bool hgm_enabled;
> +#endif
>  };
>  
>  extern struct resv_map *resv_map_alloc(void);
> @@ -1179,6 +1182,25 @@ static inline void hugetlb_unregister_node(struct node *node)
>  }
>  #endif	/* CONFIG_HUGETLB_PAGE */
>  
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +bool hugetlb_hgm_enabled(struct vm_area_struct *vma);
> +bool hugetlb_hgm_eligible(struct vm_area_struct *vma);
> +int enable_hugetlb_hgm(struct vm_area_struct *vma);
> +#else
> +static inline bool hugetlb_hgm_enabled(struct vm_area_struct *vma)
> +{
> +	return false;
> +}
> +static inline bool hugetlb_hgm_eligible(struct vm_area_struct *vma)
> +{
> +	return false;
> +}
> +static inline int enable_hugetlb_hgm(struct vm_area_struct *vma)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
>  static inline spinlock_t *huge_pte_lock(struct hstate *h,
>  					struct mm_struct *mm, pte_t *pte)
>  {
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 5ae8bc8c928e..a18143add956 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6840,6 +6840,10 @@ static bool pmd_sharing_possible(struct vm_area_struct *vma)
>  #ifdef CONFIG_USERFAULTFD
>  	if (uffd_disable_huge_pmd_share(vma))
>  		return false;
> +#endif
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +	if (hugetlb_hgm_enabled(vma))
> +		return false;
>  #endif
>  	/*
>  	 * Only shared VMAs can share PMDs.
> @@ -7033,6 +7037,9 @@ static int hugetlb_vma_data_alloc(struct vm_area_struct *vma)
>  	kref_init(&data->vma_lock.refs);
>  	init_rwsem(&data->vma_lock.rw_sema);
>  	data->vma_lock.vma = vma;
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +	data->hgm_enabled = false;
> +#endif
>  	vma->vm_private_data = data;
>  	return 0;
>  }
> @@ -7290,6 +7297,68 @@ __weak unsigned long hugetlb_mask_last_page(struct hstate *h)
>  
>  #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
>  
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +bool hugetlb_hgm_eligible(struct vm_area_struct *vma)
> +{
> +	/*
> +	 * All shared VMAs may have HGM.
> +	 *
> +	 * HGM requires using the VMA lock, which only exists for shared VMAs.
> +	 * To make HGM work for private VMAs, we would need to use another
> +	 * scheme to prevent collapsing/splitting from invalidating other
> +	 * threads' page table walks.
> +	 */
> +	return vma && (vma->vm_flags & VM_MAYSHARE);
> +}
> +bool hugetlb_hgm_enabled(struct vm_area_struct *vma)
> +{
> +	struct hugetlb_shared_vma_data *data = vma->vm_private_data;
> +
> +	if (!vma || !(vma->vm_flags & VM_MAYSHARE))
> +		return false;

Nit: smells like a open-coded hugetlb_hgm_eligible().

> +
> +	return data && data->hgm_enabled;
> +}
> +
> +/*
> + * Enable high-granularity mapping (HGM) for this VMA. Once enabled, HGM
> + * cannot be turned off.
> + *
> + * PMDs cannot be shared in HGM VMAs.
> + */
> +int enable_hugetlb_hgm(struct vm_area_struct *vma)
> +{
> +	int ret;
> +	struct hugetlb_shared_vma_data *data;
> +
> +	if (!hugetlb_hgm_eligible(vma))
> +		return -EINVAL;
> +
> +	if (hugetlb_hgm_enabled(vma))
> +		return 0;
> +
> +	/*
> +	 * We must hold the mmap lock for writing so that callers can rely on
> +	 * hugetlb_hgm_enabled returning a consistent result while holding
> +	 * the mmap lock for reading.
> +	 */
> +	mmap_assert_write_locked(vma->vm_mm);
> +
> +	/* HugeTLB HGM requires the VMA lock to synchronize collapsing. */
> +	ret = hugetlb_vma_data_alloc(vma);
> +	if (ret)
> +		return ret;
> +
> +	data = vma->vm_private_data;
> +	BUG_ON(!data);

Let's avoid BUG_ON() as afaiu it's mostly not welcomed unless extremely
necessary.  In this case it'll crash immediately in next dereference anyway
with the whole stack dumped, so we won't miss anything important. :)

> +	data->hgm_enabled = true;
> +
> +	/* We don't support PMD sharing with HGM. */
> +	hugetlb_unshare_all_pmds(vma);
> +	return 0;
> +}
> +#endif /* CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING */
> +
>  /*
>   * These functions are overwritable if your architecture needs its own
>   * behavior.
> -- 
> 2.38.0.135.g90850a2211-goog
>
  

On Fri, Oct 21, 2022 at 9:37 AM James Houghton <jthoughton@google.com> wrote:
>
> Currently it is possible for all shared VMAs to use HGM, but it must be
> enabled first. This is because with HGM, we lose PMD sharing, and page
> table walks require additional synchronization (we need to take the VMA
> lock).
>
> Signed-off-by: James Houghton <jthoughton@google.com>
> ---
>  include/linux/hugetlb.h | 22 +++++++++++++
>  mm/hugetlb.c            | 69 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 91 insertions(+)
>
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 534958499ac4..6e0c36b08a0c 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -123,6 +123,9 @@ struct hugetlb_vma_lock {
>
>  struct hugetlb_shared_vma_data {
>         struct hugetlb_vma_lock vma_lock;
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +       bool hgm_enabled;
> +#endif
>  };
>
>  extern struct resv_map *resv_map_alloc(void);
> @@ -1179,6 +1182,25 @@ static inline void hugetlb_unregister_node(struct node *node)
>  }
>  #endif /* CONFIG_HUGETLB_PAGE */
>
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +bool hugetlb_hgm_enabled(struct vm_area_struct *vma);
> +bool hugetlb_hgm_eligible(struct vm_area_struct *vma);
> +int enable_hugetlb_hgm(struct vm_area_struct *vma);
> +#else
> +static inline bool hugetlb_hgm_enabled(struct vm_area_struct *vma)
> +{
> +       return false;
> +}
> +static inline bool hugetlb_hgm_eligible(struct vm_area_struct *vma)
> +{
> +       return false;
> +}
> +static inline int enable_hugetlb_hgm(struct vm_area_struct *vma)
> +{
> +       return -EINVAL;
> +}
> +#endif
> +
>  static inline spinlock_t *huge_pte_lock(struct hstate *h,
>                                         struct mm_struct *mm, pte_t *pte)
>  {
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 5ae8bc8c928e..a18143add956 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6840,6 +6840,10 @@ static bool pmd_sharing_possible(struct vm_area_struct *vma)
>  #ifdef CONFIG_USERFAULTFD
>         if (uffd_disable_huge_pmd_share(vma))
>                 return false;
> +#endif
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +       if (hugetlb_hgm_enabled(vma))
> +               return false;
>  #endif
>         /*
>          * Only shared VMAs can share PMDs.
> @@ -7033,6 +7037,9 @@ static int hugetlb_vma_data_alloc(struct vm_area_struct *vma)
>         kref_init(&data->vma_lock.refs);
>         init_rwsem(&data->vma_lock.rw_sema);
>         data->vma_lock.vma = vma;
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +       data->hgm_enabled = false;
> +#endif
>         vma->vm_private_data = data;
>         return 0;
>  }
> @@ -7290,6 +7297,68 @@ __weak unsigned long hugetlb_mask_last_page(struct hstate *h)
>
>  #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
>
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +bool hugetlb_hgm_eligible(struct vm_area_struct *vma)
> +{
> +       /*
> +        * All shared VMAs may have HGM.
> +        *
> +        * HGM requires using the VMA lock, which only exists for shared VMAs.
> +        * To make HGM work for private VMAs, we would need to use another
> +        * scheme to prevent collapsing/splitting from invalidating other
> +        * threads' page table walks.
> +        */
> +       return vma && (vma->vm_flags & VM_MAYSHARE);
> +}
> +bool hugetlb_hgm_enabled(struct vm_area_struct *vma)
> +{
> +       struct hugetlb_shared_vma_data *data = vma->vm_private_data;
> +
> +       if (!vma || !(vma->vm_flags & VM_MAYSHARE))
> +               return false;
> +
> +       return data && data->hgm_enabled;

Don't you need to lock data->vma_lock before you access data? Or did I
misunderstand the locking? Or are you assuming this is safe before
hgm_enabled can't be disabled?
> +}
> +
> +/*
> + * Enable high-granularity mapping (HGM) for this VMA. Once enabled, HGM
> + * cannot be turned off.
> + *
> + * PMDs cannot be shared in HGM VMAs.
> + */
> +int enable_hugetlb_hgm(struct vm_area_struct *vma)
> +{
> +       int ret;
> +       struct hugetlb_shared_vma_data *data;
> +
> +       if (!hugetlb_hgm_eligible(vma))
> +               return -EINVAL;
> +
> +       if (hugetlb_hgm_enabled(vma))
> +               return 0;
> +
> +       /*
> +        * We must hold the mmap lock for writing so that callers can rely on
> +        * hugetlb_hgm_enabled returning a consistent result while holding
> +        * the mmap lock for reading.
> +        */
> +       mmap_assert_write_locked(vma->vm_mm);
> +
> +       /* HugeTLB HGM requires the VMA lock to synchronize collapsing. */
> +       ret = hugetlb_vma_data_alloc(vma);

Confused we need to vma_data_alloc() here. Shouldn't this be done by
hugetlb_vm_op_open()?

> +       if (ret)
> +               return ret;
> +
> +       data = vma->vm_private_data;
> +       BUG_ON(!data);
> +       data->hgm_enabled = true;
> +
> +       /* We don't support PMD sharing with HGM. */
> +       hugetlb_unshare_all_pmds(vma);
> +       return 0;
> +}
> +#endif /* CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING */
> +
>  /*
>   * These functions are overwritable if your architecture needs its own
>   * behavior.
> --
> 2.38.0.135.g90850a2211-goog
>
  

On Wed, Dec 7, 2022 at 7:26 PM Mina Almasry <almasrymina@google.com> wrote:
>
> On Fri, Oct 21, 2022 at 9:37 AM James Houghton <jthoughton@google.com> wrote:
> >
> > Currently it is possible for all shared VMAs to use HGM, but it must be
> > enabled first. This is because with HGM, we lose PMD sharing, and page
> > table walks require additional synchronization (we need to take the VMA
> > lock).
> >
> > Signed-off-by: James Houghton <jthoughton@google.com>
> > ---
> >  include/linux/hugetlb.h | 22 +++++++++++++
> >  mm/hugetlb.c            | 69 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 91 insertions(+)
> >
> > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > index 534958499ac4..6e0c36b08a0c 100644
> > --- a/include/linux/hugetlb.h
> > +++ b/include/linux/hugetlb.h
> > @@ -123,6 +123,9 @@ struct hugetlb_vma_lock {
> >
> >  struct hugetlb_shared_vma_data {
> >         struct hugetlb_vma_lock vma_lock;
> > +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> > +       bool hgm_enabled;
> > +#endif
> >  };
> >
> >  extern struct resv_map *resv_map_alloc(void);
> > @@ -1179,6 +1182,25 @@ static inline void hugetlb_unregister_node(struct node *node)
> >  }
> >  #endif /* CONFIG_HUGETLB_PAGE */
> >
> > +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> > +bool hugetlb_hgm_enabled(struct vm_area_struct *vma);
> > +bool hugetlb_hgm_eligible(struct vm_area_struct *vma);
> > +int enable_hugetlb_hgm(struct vm_area_struct *vma);
> > +#else
> > +static inline bool hugetlb_hgm_enabled(struct vm_area_struct *vma)
> > +{
> > +       return false;
> > +}
> > +static inline bool hugetlb_hgm_eligible(struct vm_area_struct *vma)
> > +{
> > +       return false;
> > +}
> > +static inline int enable_hugetlb_hgm(struct vm_area_struct *vma)
> > +{
> > +       return -EINVAL;
> > +}
> > +#endif
> > +
> >  static inline spinlock_t *huge_pte_lock(struct hstate *h,
> >                                         struct mm_struct *mm, pte_t *pte)
> >  {
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 5ae8bc8c928e..a18143add956 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -6840,6 +6840,10 @@ static bool pmd_sharing_possible(struct vm_area_struct *vma)
> >  #ifdef CONFIG_USERFAULTFD
> >         if (uffd_disable_huge_pmd_share(vma))
> >                 return false;
> > +#endif
> > +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> > +       if (hugetlb_hgm_enabled(vma))
> > +               return false;
> >  #endif
> >         /*
> >          * Only shared VMAs can share PMDs.
> > @@ -7033,6 +7037,9 @@ static int hugetlb_vma_data_alloc(struct vm_area_struct *vma)
> >         kref_init(&data->vma_lock.refs);
> >         init_rwsem(&data->vma_lock.rw_sema);
> >         data->vma_lock.vma = vma;
> > +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> > +       data->hgm_enabled = false;
> > +#endif
> >         vma->vm_private_data = data;
> >         return 0;
> >  }
> > @@ -7290,6 +7297,68 @@ __weak unsigned long hugetlb_mask_last_page(struct hstate *h)
> >
> >  #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
> >
> > +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> > +bool hugetlb_hgm_eligible(struct vm_area_struct *vma)
> > +{
> > +       /*
> > +        * All shared VMAs may have HGM.
> > +        *
> > +        * HGM requires using the VMA lock, which only exists for shared VMAs.
> > +        * To make HGM work for private VMAs, we would need to use another
> > +        * scheme to prevent collapsing/splitting from invalidating other
> > +        * threads' page table walks.
> > +        */
> > +       return vma && (vma->vm_flags & VM_MAYSHARE);
> > +}
> > +bool hugetlb_hgm_enabled(struct vm_area_struct *vma)
> > +{
> > +       struct hugetlb_shared_vma_data *data = vma->vm_private_data;
> > +
> > +       if (!vma || !(vma->vm_flags & VM_MAYSHARE))
> > +               return false;
> > +
> > +       return data && data->hgm_enabled;
>
> Don't you need to lock data->vma_lock before you access data? Or did I
> misunderstand the locking? Or are you assuming this is safe before
> hgm_enabled can't be disabled?

This should be protected by the mmap_lock (we must be holding it for
at least reading here). `data` and `data->hgm_enabled` are only
changed when holding the mmap_lock for writing.

> > +}
> > +
> > +/*
> > + * Enable high-granularity mapping (HGM) for this VMA. Once enabled, HGM
> > + * cannot be turned off.
> > + *
> > + * PMDs cannot be shared in HGM VMAs.
> > + */
> > +int enable_hugetlb_hgm(struct vm_area_struct *vma)
> > +{
> > +       int ret;
> > +       struct hugetlb_shared_vma_data *data;
> > +
> > +       if (!hugetlb_hgm_eligible(vma))
> > +               return -EINVAL;
> > +
> > +       if (hugetlb_hgm_enabled(vma))
> > +               return 0;
> > +
> > +       /*
> > +        * We must hold the mmap lock for writing so that callers can rely on
> > +        * hugetlb_hgm_enabled returning a consistent result while holding
> > +        * the mmap lock for reading.
> > +        */
> > +       mmap_assert_write_locked(vma->vm_mm);
> > +
> > +       /* HugeTLB HGM requires the VMA lock to synchronize collapsing. */
> > +       ret = hugetlb_vma_data_alloc(vma);
>
> Confused we need to vma_data_alloc() here. Shouldn't this be done by
> hugetlb_vm_op_open()?

hugetlb_vma_data_alloc() can fail. In hugetlb_vm_op_open()/other
places, it is allowed to fail, and so we call it again here and check
that it succeeded so that we can rely on the VMA lock.

I think I need to be a little bit more careful with how I handle VMA
splitting, though. It's possible for `data` not to be allocated after
we split, but for some things to be mapped at high-granularity. The
easiest solution here would be to disallow splitting when HGM is
enabled; not sure what the best solution is though.

Thanks for the review, Mina!

>
> > +       if (ret)
> > +               return ret;
> > +
> > +       data = vma->vm_private_data;
> > +       BUG_ON(!data);
> > +       data->hgm_enabled = true;
> > +
> > +       /* We don't support PMD sharing with HGM. */
> > +       hugetlb_unshare_all_pmds(vma);
> > +       return 0;
> > +}
> > +#endif /* CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING */
> > +
> >  /*
> >   * These functions are overwritable if your architecture needs its own
> >   * behavior.
> > --
> > 2.38.0.135.g90850a2211-goog
> >
  

On 10/21/22 16:36, James Houghton wrote:
> Currently it is possible for all shared VMAs to use HGM, but it must be
> enabled first. This is because with HGM, we lose PMD sharing, and page
> table walks require additional synchronization (we need to take the VMA
> lock).

Not sure yet, but I expect Peter's series will help with locking for
hugetlb specific page table walks.

> 
> Signed-off-by: James Houghton <jthoughton@google.com>
> ---
>  include/linux/hugetlb.h | 22 +++++++++++++
>  mm/hugetlb.c            | 69 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 91 insertions(+)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 534958499ac4..6e0c36b08a0c 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -123,6 +123,9 @@ struct hugetlb_vma_lock {
>  
>  struct hugetlb_shared_vma_data {
>  	struct hugetlb_vma_lock vma_lock;
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +	bool hgm_enabled;
> +#endif
>  };
>  
>  extern struct resv_map *resv_map_alloc(void);
> @@ -1179,6 +1182,25 @@ static inline void hugetlb_unregister_node(struct node *node)
>  }
>  #endif	/* CONFIG_HUGETLB_PAGE */
>  
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +bool hugetlb_hgm_enabled(struct vm_area_struct *vma);
> +bool hugetlb_hgm_eligible(struct vm_area_struct *vma);
> +int enable_hugetlb_hgm(struct vm_area_struct *vma);
> +#else
> +static inline bool hugetlb_hgm_enabled(struct vm_area_struct *vma)
> +{
> +	return false;
> +}
> +static inline bool hugetlb_hgm_eligible(struct vm_area_struct *vma)
> +{
> +	return false;
> +}
> +static inline int enable_hugetlb_hgm(struct vm_area_struct *vma)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
>  static inline spinlock_t *huge_pte_lock(struct hstate *h,
>  					struct mm_struct *mm, pte_t *pte)
>  {
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 5ae8bc8c928e..a18143add956 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6840,6 +6840,10 @@ static bool pmd_sharing_possible(struct vm_area_struct *vma)
>  #ifdef CONFIG_USERFAULTFD
>  	if (uffd_disable_huge_pmd_share(vma))
>  		return false;
> +#endif
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +	if (hugetlb_hgm_enabled(vma))
> +		return false;
>  #endif
>  	/*
>  	 * Only shared VMAs can share PMDs.
> @@ -7033,6 +7037,9 @@ static int hugetlb_vma_data_alloc(struct vm_area_struct *vma)
>  	kref_init(&data->vma_lock.refs);
>  	init_rwsem(&data->vma_lock.rw_sema);
>  	data->vma_lock.vma = vma;
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +	data->hgm_enabled = false;
> +#endif
>  	vma->vm_private_data = data;
>  	return 0;
>  }
> @@ -7290,6 +7297,68 @@ __weak unsigned long hugetlb_mask_last_page(struct hstate *h)
>  
>  #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
>  
> +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> +bool hugetlb_hgm_eligible(struct vm_area_struct *vma)
> +{
> +	/*
> +	 * All shared VMAs may have HGM.
> +	 *
> +	 * HGM requires using the VMA lock, which only exists for shared VMAs.
> +	 * To make HGM work for private VMAs, we would need to use another
> +	 * scheme to prevent collapsing/splitting from invalidating other
> +	 * threads' page table walks.
> +	 */
> +	return vma && (vma->vm_flags & VM_MAYSHARE);

I am not yet 100% convinced you can/will take care of all possible code
paths where hugetlb_vma_data allocation may fail.  If not, then you
should be checking vm_private_data here as well.

> +}
> +bool hugetlb_hgm_enabled(struct vm_area_struct *vma)
> +{
> +	struct hugetlb_shared_vma_data *data = vma->vm_private_data;
> +
> +	if (!vma || !(vma->vm_flags & VM_MAYSHARE))
> +		return false;
> +
> +	return data && data->hgm_enabled;
> +}
> +
> +/*
> + * Enable high-granularity mapping (HGM) for this VMA. Once enabled, HGM
> + * cannot be turned off.
> + *
> + * PMDs cannot be shared in HGM VMAs.
> + */
> +int enable_hugetlb_hgm(struct vm_area_struct *vma)
> +{
> +	int ret;
> +	struct hugetlb_shared_vma_data *data;
> +
> +	if (!hugetlb_hgm_eligible(vma))
> +		return -EINVAL;
> +
> +	if (hugetlb_hgm_enabled(vma))
> +		return 0;
> +
> +	/*
> +	 * We must hold the mmap lock for writing so that callers can rely on
> +	 * hugetlb_hgm_enabled returning a consistent result while holding
> +	 * the mmap lock for reading.
> +	 */
> +	mmap_assert_write_locked(vma->vm_mm);
> +
> +	/* HugeTLB HGM requires the VMA lock to synchronize collapsing. */
> +	ret = hugetlb_vma_data_alloc(vma);
> +	if (ret)
> +		return ret;
> +
> +	data = vma->vm_private_data;
> +	BUG_ON(!data);

Would rather have hugetlb_hgm_eligible check for vm_private_data as
suggested above instead of the BUG here.
  

On Mon, Dec 12, 2022 at 7:14 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> On 10/21/22 16:36, James Houghton wrote:
> > Currently it is possible for all shared VMAs to use HGM, but it must be
> > enabled first. This is because with HGM, we lose PMD sharing, and page
> > table walks require additional synchronization (we need to take the VMA
> > lock).
>
> Not sure yet, but I expect Peter's series will help with locking for
> hugetlb specific page table walks.

It should make things a little bit cleaner in this series; I'll rebase
HGM on top of those patches this week (and hopefully get a v1 out
soon).

I don't think it's possible to implement MADV_COLLAPSE with RCU alone
(as implemented in Peter's series anyway); we still need the VMA lock.

>
> >
> > Signed-off-by: James Houghton <jthoughton@google.com>
> > ---
> >  include/linux/hugetlb.h | 22 +++++++++++++
> >  mm/hugetlb.c            | 69 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 91 insertions(+)
> >
> > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > index 534958499ac4..6e0c36b08a0c 100644
> > --- a/include/linux/hugetlb.h
> > +++ b/include/linux/hugetlb.h
> > @@ -123,6 +123,9 @@ struct hugetlb_vma_lock {
> >
> >  struct hugetlb_shared_vma_data {
> >       struct hugetlb_vma_lock vma_lock;
> > +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> > +     bool hgm_enabled;
> > +#endif
> >  };
> >
> >  extern struct resv_map *resv_map_alloc(void);
> > @@ -1179,6 +1182,25 @@ static inline void hugetlb_unregister_node(struct node *node)
> >  }
> >  #endif       /* CONFIG_HUGETLB_PAGE */
> >
> > +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> > +bool hugetlb_hgm_enabled(struct vm_area_struct *vma);
> > +bool hugetlb_hgm_eligible(struct vm_area_struct *vma);
> > +int enable_hugetlb_hgm(struct vm_area_struct *vma);
> > +#else
> > +static inline bool hugetlb_hgm_enabled(struct vm_area_struct *vma)
> > +{
> > +     return false;
> > +}
> > +static inline bool hugetlb_hgm_eligible(struct vm_area_struct *vma)
> > +{
> > +     return false;
> > +}
> > +static inline int enable_hugetlb_hgm(struct vm_area_struct *vma)
> > +{
> > +     return -EINVAL;
> > +}
> > +#endif
> > +
> >  static inline spinlock_t *huge_pte_lock(struct hstate *h,
> >                                       struct mm_struct *mm, pte_t *pte)
> >  {
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 5ae8bc8c928e..a18143add956 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -6840,6 +6840,10 @@ static bool pmd_sharing_possible(struct vm_area_struct *vma)
> >  #ifdef CONFIG_USERFAULTFD
> >       if (uffd_disable_huge_pmd_share(vma))
> >               return false;
> > +#endif
> > +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> > +     if (hugetlb_hgm_enabled(vma))
> > +             return false;
> >  #endif
> >       /*
> >        * Only shared VMAs can share PMDs.
> > @@ -7033,6 +7037,9 @@ static int hugetlb_vma_data_alloc(struct vm_area_struct *vma)
> >       kref_init(&data->vma_lock.refs);
> >       init_rwsem(&data->vma_lock.rw_sema);
> >       data->vma_lock.vma = vma;
> > +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> > +     data->hgm_enabled = false;
> > +#endif
> >       vma->vm_private_data = data;
> >       return 0;
> >  }
> > @@ -7290,6 +7297,68 @@ __weak unsigned long hugetlb_mask_last_page(struct hstate *h)
> >
> >  #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
> >
> > +#ifdef CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING
> > +bool hugetlb_hgm_eligible(struct vm_area_struct *vma)
> > +{
> > +     /*
> > +      * All shared VMAs may have HGM.
> > +      *
> > +      * HGM requires using the VMA lock, which only exists for shared VMAs.
> > +      * To make HGM work for private VMAs, we would need to use another
> > +      * scheme to prevent collapsing/splitting from invalidating other
> > +      * threads' page table walks.
> > +      */
> > +     return vma && (vma->vm_flags & VM_MAYSHARE);
>
> I am not yet 100% convinced you can/will take care of all possible code
> paths where hugetlb_vma_data allocation may fail.  If not, then you
> should be checking vm_private_data here as well.

I think the check here makes sense -- if a VMA is shared, then it is
eligible for HGM, but we might fail to enable it because we can't
allocate the VMA lock. I'll reword the comment to clearly say this.

There is the problem of splitting, though: if we have high-granularity
mapped PTEs in a VMA and that VMA gets split, we need to remember that
the VMA had HGM enabled even if allocating the VMA lock fails,
otherwise things get out of sync. How does PMD sharing handle the
splitting case?

An easy way HGM could handle this is by disallowing splitting, but I
think we can do better. If we fail to allocate the VMA lock, then we
can no longer MADV_COLLAPSE safely, but everything else can proceed as
normal, and so some "hugetlb_hgm_enabled" checks can be
removed/changed. This should make things easier for when we have to
handle (some bits of) HGM for private mappings, too. I'll make some
improvements here for v1.

>
> > +}
> > +bool hugetlb_hgm_enabled(struct vm_area_struct *vma)
> > +{
> > +     struct hugetlb_shared_vma_data *data = vma->vm_private_data;
> > +
> > +     if (!vma || !(vma->vm_flags & VM_MAYSHARE))
> > +             return false;
> > +
> > +     return data && data->hgm_enabled;
> > +}
> > +
> > +/*
> > + * Enable high-granularity mapping (HGM) for this VMA. Once enabled, HGM
> > + * cannot be turned off.
> > + *
> > + * PMDs cannot be shared in HGM VMAs.
> > + */
> > +int enable_hugetlb_hgm(struct vm_area_struct *vma)
> > +{
> > +     int ret;
> > +     struct hugetlb_shared_vma_data *data;
> > +
> > +     if (!hugetlb_hgm_eligible(vma))
> > +             return -EINVAL;
> > +
> > +     if (hugetlb_hgm_enabled(vma))
> > +             return 0;
> > +
> > +     /*
> > +      * We must hold the mmap lock for writing so that callers can rely on
> > +      * hugetlb_hgm_enabled returning a consistent result while holding
> > +      * the mmap lock for reading.
> > +      */
> > +     mmap_assert_write_locked(vma->vm_mm);
> > +
> > +     /* HugeTLB HGM requires the VMA lock to synchronize collapsing. */
> > +     ret = hugetlb_vma_data_alloc(vma);
> > +     if (ret)
> > +             return ret;
> > +
> > +     data = vma->vm_private_data;
> > +     BUG_ON(!data);
>
> Would rather have hugetlb_hgm_eligible check for vm_private_data as
> suggested above instead of the BUG here.

I don't think we'd ever actually BUG() here. Please correct me if I'm
wrong, but if we are eligible for HGM, then hugetlb_vma_data_alloc()
will only succeed if we actually allocated the VMA data/lock, so
vma->vm_private_data should never be NULL (with the BUG_ON to inform
the reader). Maybe I should just drop the BUG()?

>
> --
> Mike Kravetz
>
> > +     data->hgm_enabled = true;
> > +
> > +     /* We don't support PMD sharing with HGM. */
> > +     hugetlb_unshare_all_pmds(vma);
> > +     return 0;
> > +}
> > +#endif /* CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING */
> > +
> >  /*
> >   * These functions are overwritable if your architecture needs its own
> >   * behavior.
> > --
> > 2.38.0.135.g90850a2211-goog
> >
  

On 12/13/22 10:49, James Houghton wrote:
> On Mon, Dec 12, 2022 at 7:14 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
> >
> > On 10/21/22 16:36, James Houghton wrote:
> > > Currently it is possible for all shared VMAs to use HGM, but it must be
> > > enabled first. This is because with HGM, we lose PMD sharing, and page
> > > table walks require additional synchronization (we need to take the VMA
> > > lock).
> >
> > Not sure yet, but I expect Peter's series will help with locking for
> > hugetlb specific page table walks.
> 
> It should make things a little bit cleaner in this series; I'll rebase
> HGM on top of those patches this week (and hopefully get a v1 out
> soon).
> 
> I don't think it's possible to implement MADV_COLLAPSE with RCU alone
> (as implemented in Peter's series anyway); we still need the VMA lock.

As I continue going through the series, I realize that I am not exactly
sure what synchronization by the vma lock is required by HGM.  As you are
aware, it was originally designed to protect against someone doing a
pmd_unshare and effectively removing part of the page table.  However,
since pmd sharing is disabled for vmas with HGM enabled (I think?), then
it might be a good idea to explicitly say somewhere the reason for using
the lock.
  

On Thu, Dec 15, 2022 at 12:52 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> On 12/13/22 10:49, James Houghton wrote:
> > On Mon, Dec 12, 2022 at 7:14 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
> > >
> > > On 10/21/22 16:36, James Houghton wrote:
> > > > Currently it is possible for all shared VMAs to use HGM, but it must be
> > > > enabled first. This is because with HGM, we lose PMD sharing, and page
> > > > table walks require additional synchronization (we need to take the VMA
> > > > lock).
> > >
> > > Not sure yet, but I expect Peter's series will help with locking for
> > > hugetlb specific page table walks.
> >
> > It should make things a little bit cleaner in this series; I'll rebase
> > HGM on top of those patches this week (and hopefully get a v1 out
> > soon).
> >
> > I don't think it's possible to implement MADV_COLLAPSE with RCU alone
> > (as implemented in Peter's series anyway); we still need the VMA lock.
>
> As I continue going through the series, I realize that I am not exactly
> sure what synchronization by the vma lock is required by HGM.  As you are
> aware, it was originally designed to protect against someone doing a
> pmd_unshare and effectively removing part of the page table.  However,
> since pmd sharing is disabled for vmas with HGM enabled (I think?), then
> it might be a good idea to explicitly say somewhere the reason for using
> the lock.

It synchronizes MADV_COLLAPSE for hugetlb (hugetlb_collapse).
MADV_COLLAPSE will take it for writing and free some page table pages,
and high-granularity walks will generally take it for reading. I'll
make this clear in a comment somewhere and in commit messages.

It might be easier if hugetlb_collapse() had the exact same
synchronization as huge_pmd_unshare, where we not only take the VMA
lock for writing, we also take the i_mmap_rw_sem for writing, so
anywhere where hugetlb_walk() is safe, high-granularity walks are also
safe. I think I should just do that for the sake of simplicity.

- James

> --
> Mike Kravetz