Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1373473 > unrolled thread

[PATCH 10/11] x86, rwsem: provide __down_write_killable

Started byMichal Hocko <mhocko@kernel.org>
First post2016-04-07 17:20 +0200
Last post2016-04-13 12:00 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 10/11] x86, rwsem: provide __down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-04-07 17:20 +0200
    Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable Ingo Molnar <mingo@kernel.org> - 2016-04-13 11:10 +0200
      Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-04-13 11:20 +0200
        Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable Ingo Molnar <mingo@kernel.org> - 2016-04-13 11:30 +0200
          Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-04-13 15:00 +0200
    [PATCH] x86: add frame annotation for call_rwsem_down_write_failed_killable Michal Hocko <mhocko@kernel.org> - 2016-04-13 12:00 +0200

#1373473 — [PATCH 10/11] x86, rwsem: provide __down_write_killable

FromMichal Hocko <mhocko@kernel.org>
Date2016-04-07 17:20 +0200
Subject[PATCH 10/11] x86, rwsem: provide __down_write_killable
Message-ID<rljSW-5SJ-15@gated-at.bofh.it>
From: Michal Hocko <mhocko@suse.com>

which uses the same fast path as __down_write except it falls back to
call_rwsem_down_write_failed_killable slow path and return -EINTR if
killed. To prevent from code duplication extract the skeleton of
__down_write into a helper macro which just takes the semaphore
and the slow path function to be called.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 arch/x86/include/asm/rwsem.h | 41 ++++++++++++++++++++++++++++-------------
 arch/x86/lib/rwsem.S         |  8 ++++++++
 2 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/rwsem.h b/arch/x86/include/asm/rwsem.h
index d79a218675bc..4c3d90dbe89a 100644
--- a/arch/x86/include/asm/rwsem.h
+++ b/arch/x86/include/asm/rwsem.h
@@ -99,21 +99,36 @@ static inline int __down_read_trylock(struct rw_semaphore *sem)
 /*
  * lock for writing
  */
+#define ____down_write(sem, slow_path)			\
+({							\
+	long tmp;					\
+	struct rw_semaphore* ret = sem;			\
+	asm volatile("# beginning down_write\n\t"	\
+		     LOCK_PREFIX "  xadd      %1,(%2)\n\t"	\
+		     /* adds 0xffff0001, returns the old value */ \
+		     "  test " __ASM_SEL(%w1,%k1) "," __ASM_SEL(%w1,%k1) "\n\t" \
+		     /* was the active mask 0 before? */\
+		     "  jz        1f\n"			\
+		     "  call " slow_path "\n"		\
+		     "1:\n"				\
+		     "# ending down_write"		\
+		     : "+m" (sem->count), "=d" (tmp), "+a" (ret)	\
+		     : "a" (sem), "1" (RWSEM_ACTIVE_WRITE_BIAS) \
+		     : "memory", "cc");			\
+	ret;						\
+})
+
 static inline void __down_write(struct rw_semaphore *sem)
 {
-	long tmp;
-	asm volatile("# beginning down_write\n\t"
-		     LOCK_PREFIX "  xadd      %1,(%2)\n\t"
-		     /* adds 0xffff0001, returns the old value */
-		     "  test " __ASM_SEL(%w1,%k1) "," __ASM_SEL(%w1,%k1) "\n\t"
-		     /* was the active mask 0 before? */
-		     "  jz        1f\n"
-		     "  call call_rwsem_down_write_failed\n"
-		     "1:\n"
-		     "# ending down_write"
-		     : "+m" (sem->count), "=d" (tmp)
-		     : "a" (sem), "1" (RWSEM_ACTIVE_WRITE_BIAS)
-		     : "memory", "cc");
+	____down_write(sem, "call_rwsem_down_write_failed");
+}
+
+static inline int __down_write_killable(struct rw_semaphore *sem)
+{
+	if (IS_ERR(____down_write(sem, "call_rwsem_down_write_failed_killable")))
+		return -EINTR;
+
+	return 0;
 }
 
 /*
diff --git a/arch/x86/lib/rwsem.S b/arch/x86/lib/rwsem.S
index 40027db99140..d1a1397e1fb3 100644
--- a/arch/x86/lib/rwsem.S
+++ b/arch/x86/lib/rwsem.S
@@ -101,6 +101,14 @@ ENTRY(call_rwsem_down_write_failed)
 	ret
 ENDPROC(call_rwsem_down_write_failed)
 
+ENTRY(call_rwsem_down_write_failed_killable)
+	save_common_regs
+	movq %rax,%rdi
+	call rwsem_down_write_failed_killable
+	restore_common_regs
+	ret
+ENDPROC(call_rwsem_down_write_failed_killable)
+
 ENTRY(call_rwsem_wake)
 	/* do nothing if still outstanding active readers */
 	__ASM_HALF_SIZE(dec) %__ASM_HALF_REG(dx)
-- 
2.8.0.rc3

[toc] | [next] | [standalone]

#1377673

FromIngo Molnar <mingo@kernel.org>
Date2016-04-13 11:10 +0200
Message-ID<rnoYa-7qk-17@gated-at.bofh.it>
In reply to#1373473
* Michal Hocko <mhocko@kernel.org> wrote:

> From: Michal Hocko <mhocko@suse.com>
> 
> which uses the same fast path as __down_write except it falls back to
> call_rwsem_down_write_failed_killable slow path and return -EINTR if
> killed. To prevent from code duplication extract the skeleton of
> __down_write into a helper macro which just takes the semaphore
> and the slow path function to be called.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  arch/x86/include/asm/rwsem.h | 41 ++++++++++++++++++++++++++++-------------
>  arch/x86/lib/rwsem.S         |  8 ++++++++
>  2 files changed, 36 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/include/asm/rwsem.h b/arch/x86/include/asm/rwsem.h
> index d79a218675bc..4c3d90dbe89a 100644
> --- a/arch/x86/include/asm/rwsem.h
> +++ b/arch/x86/include/asm/rwsem.h
> @@ -99,21 +99,36 @@ static inline int __down_read_trylock(struct rw_semaphore *sem)
>  /*
>   * lock for writing
>   */
> +#define ____down_write(sem, slow_path)			\
> +({							\
> +	long tmp;					\
> +	struct rw_semaphore* ret = sem;			\
> +	asm volatile("# beginning down_write\n\t"	\
> +		     LOCK_PREFIX "  xadd      %1,(%2)\n\t"	\
> +		     /* adds 0xffff0001, returns the old value */ \
> +		     "  test " __ASM_SEL(%w1,%k1) "," __ASM_SEL(%w1,%k1) "\n\t" \
> +		     /* was the active mask 0 before? */\
> +		     "  jz        1f\n"			\
> +		     "  call " slow_path "\n"		\
> +		     "1:\n"				\
> +		     "# ending down_write"		\
> +		     : "+m" (sem->count), "=d" (tmp), "+a" (ret)	\
> +		     : "a" (sem), "1" (RWSEM_ACTIVE_WRITE_BIAS) \
> +		     : "memory", "cc");			\
> +	ret;						\
> +})
> +
>  static inline void __down_write(struct rw_semaphore *sem)
>  {
> -	long tmp;
> -	asm volatile("# beginning down_write\n\t"
> -		     LOCK_PREFIX "  xadd      %1,(%2)\n\t"
> -		     /* adds 0xffff0001, returns the old value */
> -		     "  test " __ASM_SEL(%w1,%k1) "," __ASM_SEL(%w1,%k1) "\n\t"
> -		     /* was the active mask 0 before? */
> -		     "  jz        1f\n"
> -		     "  call call_rwsem_down_write_failed\n"
> -		     "1:\n"
> -		     "# ending down_write"
> -		     : "+m" (sem->count), "=d" (tmp)
> -		     : "a" (sem), "1" (RWSEM_ACTIVE_WRITE_BIAS)
> -		     : "memory", "cc");
> +	____down_write(sem, "call_rwsem_down_write_failed");
> +}
> +
> +static inline int __down_write_killable(struct rw_semaphore *sem)
> +{
> +	if (IS_ERR(____down_write(sem, "call_rwsem_down_write_failed_killable")))
> +		return -EINTR;
> +
> +	return 0;
>  }
>  
>  /*
> diff --git a/arch/x86/lib/rwsem.S b/arch/x86/lib/rwsem.S
> index 40027db99140..d1a1397e1fb3 100644
> --- a/arch/x86/lib/rwsem.S
> +++ b/arch/x86/lib/rwsem.S
> @@ -101,6 +101,14 @@ ENTRY(call_rwsem_down_write_failed)
>  	ret
>  ENDPROC(call_rwsem_down_write_failed)
>  
> +ENTRY(call_rwsem_down_write_failed_killable)
> +	save_common_regs
> +	movq %rax,%rdi
> +	call rwsem_down_write_failed_killable
> +	restore_common_regs
> +	ret
> +ENDPROC(call_rwsem_down_write_failed_killable)

Got this objtool warning on x86-64 allyesconfig:

  arch/x86/lib/rwsem.o: warning: objtool: call_rwsem_down_write_failed_killable()+0xe: call without frame pointer save/setup

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1377677

FromMichal Hocko <mhocko@kernel.org>
Date2016-04-13 11:20 +0200
Message-ID<rnp7Q-7vE-5@gated-at.bofh.it>
In reply to#1377673
On Wed 13-04-16 11:08:30, Ingo Molnar wrote:
> 
> * Michal Hocko <mhocko@kernel.org> wrote:
[...]
> > +ENTRY(call_rwsem_down_write_failed_killable)
> > +	save_common_regs
> > +	movq %rax,%rdi
> > +	call rwsem_down_write_failed_killable
> > +	restore_common_regs
> > +	ret
> > +ENDPROC(call_rwsem_down_write_failed_killable)
> 
> Got this objtool warning on x86-64 allyesconfig:
> 
>   arch/x86/lib/rwsem.o: warning: objtool: call_rwsem_down_write_failed_killable()+0xe: call without frame pointer save/setup

Peter has already pointed that out. This is because the 4.5 which I was
basing my work on doesn't have 3387a535ce62 ("x86/asm: Create stack
frames in rwsem functions") which shown up in 4.6-rc1. He mentioned to
add the missing FRAME_{BEGIN,END} during the merge AFAIR.

Does that sound reasonable to you or should I rebase?

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1377686

FromIngo Molnar <mingo@kernel.org>
Date2016-04-13 11:30 +0200
Message-ID<rnphw-7zX-21@gated-at.bofh.it>
In reply to#1377677
* Michal Hocko <mhocko@kernel.org> wrote:

> On Wed 13-04-16 11:08:30, Ingo Molnar wrote:
> > 
> > * Michal Hocko <mhocko@kernel.org> wrote:
> [...]
> > > +ENTRY(call_rwsem_down_write_failed_killable)
> > > +	save_common_regs
> > > +	movq %rax,%rdi
> > > +	call rwsem_down_write_failed_killable
> > > +	restore_common_regs
> > > +	ret
> > > +ENDPROC(call_rwsem_down_write_failed_killable)
> > 
> > Got this objtool warning on x86-64 allyesconfig:
> > 
> >   arch/x86/lib/rwsem.o: warning: objtool: call_rwsem_down_write_failed_killable()+0xe: call without frame pointer save/setup
> 
> Peter has already pointed that out. This is because the 4.5 which I was
> basing my work on doesn't have 3387a535ce62 ("x86/asm: Create stack
> frames in rwsem functions") which shown up in 4.6-rc1. He mentioned to
> add the missing FRAME_{BEGIN,END} during the merge AFAIR.
> 
> Does that sound reasonable to you or should I rebase?

I'm testing your patches today, if they are otherwise OK then please send a delta 
patch against the tip:locking/rwsem tree I'll push out.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1377901

FromMichal Hocko <mhocko@kernel.org>
Date2016-04-13 15:00 +0200
Message-ID<rnsyK-1lV-15@gated-at.bofh.it>
In reply to#1377686
On Wed 13-04-16 12:27:31, Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@kernel.org> wrote:
> 
> > I'm testing your patches today, if they are otherwise OK [...]
> 
> got this build failure:
> 
>   ./arch/x86/include/asm/rwsem.h:106:2: error: ‘asm’ operand has impossible constraints

Hmm, I have no idea why 64b didn't have problem with the asm but 32b
complains. Anyway, the following makes both happy. I have checked the
generated code for 64b and it hasn't changed after the patch. 32b also
seems to be generating a proper code. My gcc asm()-foo is rather weak so
I would feel better if somebody double checked after me.
---
From d23f4e6994670bf2c5d864f2190f21022b4499b2 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Wed, 13 Apr 2016 14:21:25 +0200
Subject: [PATCH] x86: __down_read_trylock fix 32b build failure
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Ingo has noticed the following compilation error with CONFIG_X86_32=y
./arch/x86/include/asm/rwsem.h:106:2: error: ‘asm’ operand has impossible constraints

The reason seems to be that 32b doesn't like ret being input and output
argument sharing the same register with sem which is only the input. Fix
this by making ret output only and use %3 (aka sem) for xadd.

ret initialization is not needed now because this is done implicitly
by the asm even for the fast path as both sem and ret share the same
register.

Reported-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 arch/x86/include/asm/rwsem.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/rwsem.h b/arch/x86/include/asm/rwsem.h
index d759c5f70f49..453744c1d347 100644
--- a/arch/x86/include/asm/rwsem.h
+++ b/arch/x86/include/asm/rwsem.h
@@ -102,9 +102,9 @@ static inline int __down_read_trylock(struct rw_semaphore *sem)
 #define ____down_write(sem, slow_path)			\
 ({							\
 	long tmp;					\
-	struct rw_semaphore* ret = sem;			\
+	struct rw_semaphore* ret;			\
 	asm volatile("# beginning down_write\n\t"	\
-		     LOCK_PREFIX "  xadd      %1,(%2)\n\t"	\
+		     LOCK_PREFIX "  xadd      %1,(%3)\n\t"	\
 		     /* adds 0xffff0001, returns the old value */ \
 		     "  test " __ASM_SEL(%w1,%k1) "," __ASM_SEL(%w1,%k1) "\n\t" \
 		     /* was the active mask 0 before? */\
@@ -112,7 +112,7 @@ static inline int __down_read_trylock(struct rw_semaphore *sem)
 		     "  call " slow_path "\n"		\
 		     "1:\n"				\
 		     "# ending down_write"		\
-		     : "+m" (sem->count), "=d" (tmp), "+a" (ret)	\
+		     : "+m" (sem->count), "=d" (tmp), "=a" (ret)	\
 		     : "a" (sem), "1" (RWSEM_ACTIVE_WRITE_BIAS) \
 		     : "memory", "cc");			\
 	ret;						\
-- 
2.8.0.rc3

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1377709 — [PATCH] x86: add frame annotation for call_rwsem_down_write_failed_killable

FromMichal Hocko <mhocko@kernel.org>
Date2016-04-13 12:00 +0200
Subject[PATCH] x86: add frame annotation for call_rwsem_down_write_failed_killable
Message-ID<rnpKy-7Mh-19@gated-at.bofh.it>
In reply to#1373473
From: Michal Hocko <mhocko@suse.com>

3387a535ce62 ("x86/asm: Create stack frames in rwsem functions") has
added FRAME_{BEGIN,END} annotations to rwsem asm stubs. The patch
which has added call_rwsem_down_write_failed_killable was based on an
older tree so it didn't know about annotations. Let's add them.

Reported-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---

Hi Ingo,
please apply this on top of [1] when merging to tip/locking/rwsem.
Thanks!

[1] http://lkml.kernel.org/r/1460041951-22347-11-git-send-email-mhocko@kernel.org
 arch/x86/lib/rwsem.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/lib/rwsem.S b/arch/x86/lib/rwsem.S
index 4534a7e912f3..a37462a23546 100644
--- a/arch/x86/lib/rwsem.S
+++ b/arch/x86/lib/rwsem.S
@@ -107,10 +107,12 @@ ENTRY(call_rwsem_down_write_failed)
 ENDPROC(call_rwsem_down_write_failed)
 
 ENTRY(call_rwsem_down_write_failed_killable)
+	FRAME_BEGIN
 	save_common_regs
 	movq %rax,%rdi
 	call rwsem_down_write_failed_killable
 	restore_common_regs
+	FRAME_END
 	ret
 ENDPROC(call_rwsem_down_write_failed_killable)
 
-- 
2.8.0.rc3

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web