[v4,08/10] mmap locking API: add MMAP_LOCK_INITIALIZER
diff mbox series

Message ID 20200415004353.130248-9-walken@google.com
State New
Headers show
Series
  • Add a new mmap locking API wrapping mmap_sem calls
Related show

Commit Message

Michel Lespinasse April 15, 2020, 12:43 a.m. UTC
Define a new initializer for the mmap locking api.
Initially this just evaluates to __RWSEM_INITIALIZER as the API
is defined as wrappers around rwsem.

Signed-off-by: Michel Lespinasse <walken@google.com>
---
 arch/x86/kernel/tboot.c    | 2 +-
 drivers/firmware/efi/efi.c | 2 +-
 include/linux/mmap_lock.h  | 2 ++
 mm/init-mm.c               | 2 +-
 4 files changed, 5 insertions(+), 3 deletions(-)

Comments

Daniel Jordan April 20, 2020, 6:23 p.m. UTC | #1
On Tue, Apr 14, 2020 at 05:43:51PM -0700, Michel Lespinasse wrote:
> Define a new initializer for the mmap locking api.
> Initially this just evaluates to __RWSEM_INITIALIZER as the API
> is defined as wrappers around rwsem.

Shouldn't this take mm as the argument like the other stuff in mmap_lock.h?
Matthew Wilcox (Oracle) April 20, 2020, 7:28 p.m. UTC | #2
On Tue, Apr 14, 2020 at 05:43:51PM -0700, Michel Lespinasse wrote:
> @@ -90,7 +90,7 @@ static struct mm_struct tboot_mm = {
                                            ^^^^^^^^
>  	.pgd            = swapper_pg_dir,
>  	.mm_users       = ATOMIC_INIT(2),
>  	.mm_count       = ATOMIC_INIT(1),
> -	.mmap_sem       = __RWSEM_INITIALIZER(init_mm.mmap_sem),
> +	.mmap_sem       = MMAP_LOCK_INITIALIZER(init_mm.mmap_sem),
                                                ^^^^^^^

Shome mishtake, shirley?

I don't see that this particular patch buys us much.  The name 'mmap_sem'
is still used, and I appreciate we abstract away the type of the lock,
but wouldn't this be better?

-	.mmap_sem       = __RWSEM_INITIALIZER(init_mm.mmap_sem),
+	MMAP_LOCK_INITIALIZER(tboot_mm),
Michel Lespinasse April 21, 2020, 12:57 a.m. UTC | #3
On Mon, Apr 20, 2020 at 12:28 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Apr 14, 2020 at 05:43:51PM -0700, Michel Lespinasse wrote:
> > @@ -90,7 +90,7 @@ static struct mm_struct tboot_mm = {
>                                             ^^^^^^^^
> >       .pgd            = swapper_pg_dir,
> >       .mm_users       = ATOMIC_INIT(2),
> >       .mm_count       = ATOMIC_INIT(1),
> > -     .mmap_sem       = __RWSEM_INITIALIZER(init_mm.mmap_sem),
> > +     .mmap_sem       = MMAP_LOCK_INITIALIZER(init_mm.mmap_sem),
>                                                 ^^^^^^^
>
> Shome mishtake, shirley?
>
> I don't see that this particular patch buys us much.  The name 'mmap_sem'
> is still used, and I appreciate we abstract away the type of the lock,
> but wouldn't this be better?
>
> -       .mmap_sem       = __RWSEM_INITIALIZER(init_mm.mmap_sem),
> +       MMAP_LOCK_INITIALIZER(tboot_mm),

Hmmm, that's significantly different from other initializers we have,
which may be a downside ? But other than that, it does seem completely
workable to me. Do you have a strong preference ?

Patch
diff mbox series

diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index b89f6ac6a0c0..4b79335624b1 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -90,7 +90,7 @@  static struct mm_struct tboot_mm = {
 	.pgd            = swapper_pg_dir,
 	.mm_users       = ATOMIC_INIT(2),
 	.mm_count       = ATOMIC_INIT(1),
-	.mmap_sem       = __RWSEM_INITIALIZER(init_mm.mmap_sem),
+	.mmap_sem       = MMAP_LOCK_INITIALIZER(init_mm.mmap_sem),
 	.page_table_lock =  __SPIN_LOCK_UNLOCKED(init_mm.page_table_lock),
 	.mmlist         = LIST_HEAD_INIT(init_mm.mmlist),
 };
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 911a2bd0f6b7..4bac9d7a48a6 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -54,7 +54,7 @@  struct mm_struct efi_mm = {
 	.mm_rb			= RB_ROOT,
 	.mm_users		= ATOMIC_INIT(2),
 	.mm_count		= ATOMIC_INIT(1),
-	.mmap_sem		= __RWSEM_INITIALIZER(efi_mm.mmap_sem),
+	.mmap_sem		= MMAP_LOCK_INITIALIZER(efi_mm.mmap_sem),
 	.page_table_lock	= __SPIN_LOCK_UNLOCKED(efi_mm.page_table_lock),
 	.mmlist			= LIST_HEAD_INIT(efi_mm.mmlist),
 	.cpu_bitmap		= { [BITS_TO_LONGS(NR_CPUS)] = 0},
diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
index 1050257361aa..07efa379664a 100644
--- a/include/linux/mmap_lock.h
+++ b/include/linux/mmap_lock.h
@@ -1,6 +1,8 @@ 
 #ifndef _LINUX_MMAP_LOCK_H
 #define _LINUX_MMAP_LOCK_H
 
+#define MMAP_LOCK_INITIALIZER(name) __RWSEM_INITIALIZER(name)
+
 static inline void mmap_init_lock(struct mm_struct *mm)
 {
 	init_rwsem(&mm->mmap_sem);
diff --git a/mm/init-mm.c b/mm/init-mm.c
index 19603302a77f..3c128bd6a30c 100644
--- a/mm/init-mm.c
+++ b/mm/init-mm.c
@@ -31,7 +31,7 @@  struct mm_struct init_mm = {
 	.pgd		= swapper_pg_dir,
 	.mm_users	= ATOMIC_INIT(2),
 	.mm_count	= ATOMIC_INIT(1),
-	.mmap_sem	= __RWSEM_INITIALIZER(init_mm.mmap_sem),
+	.mmap_sem	= MMAP_LOCK_INITIALIZER(init_mm.mmap_sem),
 	.page_table_lock =  __SPIN_LOCK_UNLOCKED(init_mm.page_table_lock),
 	.arg_lock	=  __SPIN_LOCK_UNLOCKED(init_mm.arg_lock),
 	.mmlist		= LIST_HEAD_INIT(init_mm.mmlist),