diff mbox series

[RFC,v2,06/47] hugetlb: extend vma lock for shared vmas

Message ID 20221021163703.3218176-7-jthoughton@google.com (mailing list archive)
State New
Headers show
Series hugetlb: introduce HugeTLB high-granularity mapping | expand

Commit Message

James Houghton Oct. 21, 2022, 4:36 p.m. UTC 
This allows us to add more data into the shared structure, which we will
use to store whether or not HGM is enabled for this VMA or not, as HGM
is only available for shared mappings.

It may be better to include HGM as a VMA flag instead of extending the
VMA lock structure.

Signed-off-by: James Houghton <jthoughton@google.com>
---
 include/linux/hugetlb.h |  4 +++
 mm/hugetlb.c            | 65 +++++++++++++++++++++--------------------
 2 files changed, 37 insertions(+), 32 deletions(-)

Comments

Peter Xu Nov. 30, 2022, 9:01 p.m. UTC | #1 
On Fri, Oct 21, 2022 at 04:36:22PM +0000, James Houghton wrote:
> This allows us to add more data into the shared structure, which we will
> use to store whether or not HGM is enabled for this VMA or not, as HGM
> is only available for shared mappings.
> 
> It may be better to include HGM as a VMA flag instead of extending the
> VMA lock structure.
> 
> Signed-off-by: James Houghton <jthoughton@google.com>
> ---
>  include/linux/hugetlb.h |  4 +++
>  mm/hugetlb.c            | 65 +++++++++++++++++++++--------------------
>  2 files changed, 37 insertions(+), 32 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index a899bc76d677..534958499ac4 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -121,6 +121,10 @@ struct hugetlb_vma_lock {
>  	struct vm_area_struct *vma;
>  };
>  
> +struct hugetlb_shared_vma_data {
> +	struct hugetlb_vma_lock vma_lock;
> +};

How about add a comment above hugetlb_vma_lock showing how it should be
used correctly?  We lacked documents on the lock for pmd sharing
protections, now if to reuse the same lock for HGM pgtables I think some
doc will definitely help.

To summarize, I think so far it means:

  - Read lock needed when one wants to stablize VM_SHARED pgtables (covers
    both pmd shared pgtables or hgm low-level pgtables)

  - Write lock needed when one wants to release VM_SHARED pgtable pages
    (covers both pmd unshare or releasing hgm low-level pgtables)

Or something like that.
James Houghton Nov. 30, 2022, 11:29 p.m. UTC | #2 
On Wed, Nov 30, 2022 at 4:01 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Oct 21, 2022 at 04:36:22PM +0000, James Houghton wrote:
> > This allows us to add more data into the shared structure, which we will
> > use to store whether or not HGM is enabled for this VMA or not, as HGM
> > is only available for shared mappings.
> >
> > It may be better to include HGM as a VMA flag instead of extending the
> > VMA lock structure.
> >
> > Signed-off-by: James Houghton <jthoughton@google.com>
> > ---
> >  include/linux/hugetlb.h |  4 +++
> >  mm/hugetlb.c            | 65 +++++++++++++++++++++--------------------
> >  2 files changed, 37 insertions(+), 32 deletions(-)
> >
> > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > index a899bc76d677..534958499ac4 100644
> > --- a/include/linux/hugetlb.h
> > +++ b/include/linux/hugetlb.h
> > @@ -121,6 +121,10 @@ struct hugetlb_vma_lock {
> >       struct vm_area_struct *vma;
> >  };
> >
> > +struct hugetlb_shared_vma_data {
> > +     struct hugetlb_vma_lock vma_lock;
> > +};
>
> How about add a comment above hugetlb_vma_lock showing how it should be
> used correctly?  We lacked documents on the lock for pmd sharing
> protections, now if to reuse the same lock for HGM pgtables I think some
> doc will definitely help.
>
> To summarize, I think so far it means:
>
>   - Read lock needed when one wants to stablize VM_SHARED pgtables (covers
>     both pmd shared pgtables or hgm low-level pgtables)
>
>   - Write lock needed when one wants to release VM_SHARED pgtable pages
>     (covers both pmd unshare or releasing hgm low-level pgtables)
>
> Or something like that.

Will do. I'll make this change together with the rmap comment update
("rmap: update hugetlb lock comment for HGM").

- James

>
> --
> Peter Xu
>
Mike Kravetz Dec. 9, 2022, 10:48 p.m. UTC | #3 
On 11/30/22 16:01, Peter Xu wrote:
> On Fri, Oct 21, 2022 at 04:36:22PM +0000, James Houghton wrote:
> > This allows us to add more data into the shared structure, which we will
> > use to store whether or not HGM is enabled for this VMA or not, as HGM
> > is only available for shared mappings.
> > 
> > It may be better to include HGM as a VMA flag instead of extending the
> > VMA lock structure.
> > 
> > Signed-off-by: James Houghton <jthoughton@google.com>
> > ---
> >  include/linux/hugetlb.h |  4 +++
> >  mm/hugetlb.c            | 65 +++++++++++++++++++++--------------------
> >  2 files changed, 37 insertions(+), 32 deletions(-)
> > 
> > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > index a899bc76d677..534958499ac4 100644
> > --- a/include/linux/hugetlb.h
> > +++ b/include/linux/hugetlb.h
> > @@ -121,6 +121,10 @@ struct hugetlb_vma_lock {
> >  	struct vm_area_struct *vma;
> >  };
> >  
> > +struct hugetlb_shared_vma_data {
> > +	struct hugetlb_vma_lock vma_lock;
> > +};
> 
> How about add a comment above hugetlb_vma_lock showing how it should be
> used correctly?  We lacked documents on the lock for pmd sharing
> protections, now if to reuse the same lock for HGM pgtables I think some
> doc will definitely help.
> 
> To summarize, I think so far it means:
> 
>   - Read lock needed when one wants to stablize VM_SHARED pgtables (covers
>     both pmd shared pgtables or hgm low-level pgtables)
> 
>   - Write lock needed when one wants to release VM_SHARED pgtable pages
>     (covers both pmd unshare or releasing hgm low-level pgtables)

Peter must have read ahead and knows that you plan to use the vma_lock for HGM.

The commit message implies that a you only need some type of indication (a flag
for instance) that HGM is enabled for the vma.

No objections to expanding the structure as is done here.

If this is the direction we take, and someday this is extended to private
mappings we could used the same scheme to expand the reserve map structure.
diff mbox series

Patch

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index a899bc76d677..534958499ac4 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -121,6 +121,10 @@  struct hugetlb_vma_lock {
 	struct vm_area_struct *vma;
 };
 
+struct hugetlb_shared_vma_data {
+	struct hugetlb_vma_lock vma_lock;
+};
+
 extern struct resv_map *resv_map_alloc(void);
 void resv_map_release(struct kref *ref);
 
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index dc82256b89dd..5ae8bc8c928e 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -91,8 +91,8 @@  struct mutex *hugetlb_fault_mutex_table ____cacheline_aligned_in_smp;
 
 /* Forward declaration */
 static int hugetlb_acct_memory(struct hstate *h, long delta);
-static void hugetlb_vma_lock_free(struct vm_area_struct *vma);
-static int hugetlb_vma_lock_alloc(struct vm_area_struct *vma);
+static void hugetlb_vma_data_free(struct vm_area_struct *vma);
+static int hugetlb_vma_data_alloc(struct vm_area_struct *vma);
 static void __hugetlb_vma_unlock_write_free(struct vm_area_struct *vma);
 
 static inline bool subpool_is_free(struct hugepage_subpool *spool)
@@ -4643,11 +4643,11 @@  static void hugetlb_vm_op_open(struct vm_area_struct *vma)
 		if (vma_lock) {
 			if (vma_lock->vma != vma) {
 				vma->vm_private_data = NULL;
-				hugetlb_vma_lock_alloc(vma);
+				hugetlb_vma_data_alloc(vma);
 			} else
 				pr_warn("HugeTLB: vma_lock already exists in %s.\n", __func__);
 		} else
-			hugetlb_vma_lock_alloc(vma);
+			hugetlb_vma_data_alloc(vma);
 	}
 }
 
@@ -4659,7 +4659,7 @@  static void hugetlb_vm_op_close(struct vm_area_struct *vma)
 	unsigned long reserve, start, end;
 	long gbl_reserve;
 
-	hugetlb_vma_lock_free(vma);
+	hugetlb_vma_data_free(vma);
 
 	resv = vma_resv_map(vma);
 	if (!resv || !is_vma_resv_set(vma, HPAGE_RESV_OWNER))
@@ -6629,7 +6629,7 @@  bool hugetlb_reserve_pages(struct inode *inode,
 	/*
 	 * vma specific semaphore used for pmd sharing synchronization
 	 */
-	hugetlb_vma_lock_alloc(vma);
+	hugetlb_vma_data_alloc(vma);
 
 	/*
 	 * Only apply hugepage reservation if asked. At fault time, an
@@ -6753,7 +6753,7 @@  bool hugetlb_reserve_pages(struct inode *inode,
 	hugetlb_cgroup_uncharge_cgroup_rsvd(hstate_index(h),
 					    chg * pages_per_huge_page(h), h_cg);
 out_err:
-	hugetlb_vma_lock_free(vma);
+	hugetlb_vma_data_free(vma);
 	if (!vma || vma->vm_flags & VM_MAYSHARE)
 		/* Only call region_abort if the region_chg succeeded but the
 		 * region_add failed or didn't run.
@@ -6901,55 +6901,55 @@  static bool __vma_shareable_flags_pmd(struct vm_area_struct *vma)
 void hugetlb_vma_lock_read(struct vm_area_struct *vma)
 {
 	if (__vma_shareable_flags_pmd(vma)) {
-		struct hugetlb_vma_lock *vma_lock = vma->vm_private_data;
+		struct hugetlb_shared_vma_data *data = vma->vm_private_data;
 
-		down_read(&vma_lock->rw_sema);
+		down_read(&data->vma_lock.rw_sema);
 	}
 }
 
 void hugetlb_vma_unlock_read(struct vm_area_struct *vma)
 {
 	if (__vma_shareable_flags_pmd(vma)) {
-		struct hugetlb_vma_lock *vma_lock = vma->vm_private_data;
+		struct hugetlb_shared_vma_data *data = vma->vm_private_data;
 
-		up_read(&vma_lock->rw_sema);
+		up_read(&data->vma_lock.rw_sema);
 	}
 }
 
 void hugetlb_vma_lock_write(struct vm_area_struct *vma)
 {
 	if (__vma_shareable_flags_pmd(vma)) {
-		struct hugetlb_vma_lock *vma_lock = vma->vm_private_data;
+		struct hugetlb_shared_vma_data *data = vma->vm_private_data;
 
-		down_write(&vma_lock->rw_sema);
+		down_write(&data->vma_lock.rw_sema);
 	}
 }
 
 void hugetlb_vma_unlock_write(struct vm_area_struct *vma)
 {
 	if (__vma_shareable_flags_pmd(vma)) {
-		struct hugetlb_vma_lock *vma_lock = vma->vm_private_data;
+		struct hugetlb_shared_vma_data *data = vma->vm_private_data;
 
-		up_write(&vma_lock->rw_sema);
+		up_write(&data->vma_lock.rw_sema);
 	}
 }
 
 int hugetlb_vma_trylock_write(struct vm_area_struct *vma)
 {
-	struct hugetlb_vma_lock *vma_lock = vma->vm_private_data;
+	struct hugetlb_shared_vma_data *data = vma->vm_private_data;
 
 	if (!__vma_shareable_flags_pmd(vma))
 		return 1;
 
-	return down_write_trylock(&vma_lock->rw_sema);
+	return down_write_trylock(&data->vma_lock.rw_sema);
 }
 
 void hugetlb_vma_assert_locked(struct vm_area_struct *vma)
 {
 	if (__vma_shareable_flags_pmd(vma)) {
-		struct hugetlb_vma_lock *vma_lock = vma->vm_private_data;
+		struct hugetlb_shared_vma_data *data = vma->vm_private_data;
 
-		lockdep_assert_held(&vma_lock->rw_sema);
+		lockdep_assert_held(&data->vma_lock.rw_sema);
 	}
 }
 
@@ -6985,7 +6985,7 @@  static void __hugetlb_vma_unlock_write_free(struct vm_area_struct *vma)
 	}
 }
 
-static void hugetlb_vma_lock_free(struct vm_area_struct *vma)
+static void hugetlb_vma_data_free(struct vm_area_struct *vma)
 {
 	/*
 	 * Only present in sharable vmas.
@@ -6994,16 +6994,17 @@  static void hugetlb_vma_lock_free(struct vm_area_struct *vma)
 		return;
 
 	if (vma->vm_private_data) {
-		struct hugetlb_vma_lock *vma_lock = vma->vm_private_data;
+		struct hugetlb_shared_vma_data *data = vma->vm_private_data;
+		struct hugetlb_vma_lock *vma_lock = &data->vma_lock;
 
 		down_write(&vma_lock->rw_sema);
 		__hugetlb_vma_unlock_write_put(vma_lock);
 	}
 }
 
-static int hugetlb_vma_lock_alloc(struct vm_area_struct *vma)
+static int hugetlb_vma_data_alloc(struct vm_area_struct *vma)
 {
-	struct hugetlb_vma_lock *vma_lock;
+	struct hugetlb_shared_vma_data *data;
 
 	/* Only establish in (flags) sharable vmas */
 	if (!vma || !(vma->vm_flags & VM_MAYSHARE))
@@ -7013,8 +7014,8 @@  static int hugetlb_vma_lock_alloc(struct vm_area_struct *vma)
 	if (vma->vm_private_data)
 		return 0;
 
-	vma_lock = kmalloc(sizeof(*vma_lock), GFP_KERNEL);
-	if (!vma_lock) {
+	data = kmalloc(sizeof(*data), GFP_KERNEL);
+	if (!data) {
 		/*
 		 * If we can not allocate structure, then vma can not
 		 * participate in pmd sharing.  This is only a possible
@@ -7025,14 +7026,14 @@  static int hugetlb_vma_lock_alloc(struct vm_area_struct *vma)
 		 * until the file is removed.  Warn in the unlikely case of
 		 * allocation failure.
 		 */
-		pr_warn_once("HugeTLB: unable to allocate vma specific lock\n");
+		pr_warn_once("HugeTLB: unable to allocate vma shared data\n");
 		return -ENOMEM;
 	}
 
-	kref_init(&vma_lock->refs);
-	init_rwsem(&vma_lock->rw_sema);
-	vma_lock->vma = vma;
-	vma->vm_private_data = vma_lock;
+	kref_init(&data->vma_lock.refs);
+	init_rwsem(&data->vma_lock.rw_sema);
+	data->vma_lock.vma = vma;
+	vma->vm_private_data = data;
 	return 0;
 }
 
@@ -7157,11 +7158,11 @@  static void __hugetlb_vma_unlock_write_free(struct vm_area_struct *vma)
 {
 }
 
-static void hugetlb_vma_lock_free(struct vm_area_struct *vma)
+static void hugetlb_vma_data_free(struct vm_area_struct *vma)
 {
 }
 
-static int hugetlb_vma_lock_alloc(struct vm_area_struct *vma)
+static int hugetlb_vma_data_alloc(struct vm_area_struct *vma)
 {
 	return 0;
 }