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

Groups > linux.kernel > #1383404 > unrolled thread

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

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

  Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable Peter Zijlstra <peterz@infradead.org> - 2016-04-20 15:50 +0200
    Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable "H. Peter Anvin" <hpa@zytor.com> - 2016-04-20 20:10 +0200
      Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable Borislav Petkov <bp@alien8.de> - 2016-04-20 22:50 +0200
        Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-04-20 23:00 +0200
        Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable "H. Peter Anvin" <hpa@zytor.com> - 2016-04-20 23:10 +0200
          Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable Borislav Petkov <bp@alien8.de> - 2016-04-20 23:40 +0200
            Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable "H. Peter Anvin" <hpa@zytor.com> - 2016-04-21 00:40 +0200
              Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable Borislav Petkov <bp@alien8.de> - 2016-04-21 13:40 +0200
                Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-04-21 15:10 +0200
                  Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable Borislav Petkov <bp@alien8.de> - 2016-04-21 15:30 +0200
              [PATCH] x86/locking/rwsem: Cleanup ____down_write() Borislav Petkov <bp@alien8.de> - 2016-04-27 14:10 +0200
                [tip:locking/rwsem] locking/rwsem, x86: Clean up ____down_write() tip-bot for Borislav Petkov <tipbot@zytor.com> - 2016-04-28 12:30 +0200
    Re: [PATCH 10/11] x86, rwsem: provide __down_write_killable Ingo Molnar <mingo@kernel.org> - 2016-04-22 09:00 +0200

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

On Wed, Apr 13, 2016 at 02:49:43PM +0200, Michal Hocko wrote:
> 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.

I completely blow at this gcc-asm constraints thing too :/

In any case, Ingo will you look after the rest of these patches, or do
you want me to pick up the remaining bits?

[toc] | [next] | [standalone]

#1383617

On April 20, 2016 6:40:19 AM PDT, Peter Zijlstra <peterz@infradead.org> wrote:
>On Wed, Apr 13, 2016 at 02:49:43PM +0200, Michal Hocko wrote:
>> 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.
>
>I completely blow at this gcc-asm constraints thing too :/
>
>In any case, Ingo will you look after the rest of these patches, or do
>you want me to pick up the remaining bits?

The reason it breaks is because the same register can't be an input-output register and a separate input.  However, the input side of the input-output is probably undefined, and so gcc may not notice.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

#1383733

On Wed, Apr 20, 2016 at 11:04:05AM -0700, H. Peter Anvin wrote:
> The reason it breaks is because the same register can't be an
> input-output register and a separate input. However, the input side of
> the input-output is probably undefined, and so gcc may not notice.

So Michal and I talked about this a while ago. Why do we need the '"a"
(sem)' input dependency if '"+a" (ret)' already supplies the same thing?

There's also that "=d" (tmp) thing which we don't really need as an
output, right?

I.e., can we simplify like this?

---
#define ____down_write(sem, slow_path)                  \
({                                                      \
        long tmp = RWSEM_ACTIVE_WRITE_BIAS;             \
	struct rw_semaphore* ret = sem;			\
                                                        \
        asm volatile("# beginning down_write\n\t"       \
                     LOCK_PREFIX "  xadd      %[tmp],(%[ret])\n\t"      \
                     /* adds 0xffff0001, returns the old value */ \
                     "  test " __ASM_SEL(%w[tmp],%k[tmp]) "," __ASM_SEL(%w[tmp],%k[tmp]) "\n\t" \
                     /* was the active mask 0 before? */\
                     "  jz        1f\n"                 \
                     "  call " slow_path "\n"           \
                     "1:\n"                             \
                     "# ending down_write"              \
                     : "+m" (sem->count), [ret] "+a" (ret) \
                     : [tmp] "d" (tmp)                  \
                     : "memory", "cc");                 \
        ret;                                            \
})

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

#1383737

On Wed 20-04-16 22:45:01, Borislav Petkov wrote:
> On Wed, Apr 20, 2016 at 11:04:05AM -0700, H. Peter Anvin wrote:
> > The reason it breaks is because the same register can't be an
> > input-output register and a separate input. However, the input side of
> > the input-output is probably undefined, and so gcc may not notice.
> 
> So Michal and I talked about this a while ago. Why do we need the '"a"
> (sem)' input dependency if '"+a" (ret)' already supplies the same thing?
> 
> There's also that "=d" (tmp) thing which we don't really need as an
> output, right?
> 
> I.e., can we simplify like this?

I am for any simplification, my gcc-asm-foo is just too weak and I
wanted my change to be as minimal as possible. So if you feel you can
clean up this I would more than welcome that. Maybe a follow up patch
would be a better approach so that we can check that the generated code
hasn't changed.

Thanks!
-- 
Michal Hocko
SUSE Labs

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

#1383746

On 04/20/2016 01:45 PM, Borislav Petkov wrote:
> On Wed, Apr 20, 2016 at 11:04:05AM -0700, H. Peter Anvin wrote:
>> The reason it breaks is because the same register can't be an
>> input-output register and a separate input. However, the input side of
>> the input-output is probably undefined, and so gcc may not notice.
> 
> So Michal and I talked about this a while ago. Why do we need the '"a"
> (sem)' input dependency if '"+a" (ret)' already supplies the same thing?
> 

Setting ret to sem doesn't make any sense.  Just use "=a" and "a".

	-hpa

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

#1383760

On Wed, Apr 20, 2016 at 02:06:33PM -0700, H. Peter Anvin wrote:
> Setting ret to sem doesn't make any sense.  Just use "=a" and "a".

Yeah, that's what Michal's patch ontop does.

And to answer my own question: we need the "a" (sem) input for the fast
path.

I guess we can still move "1" (RWSEM_ACTIVE_WRITE_BIAS) before the asm():

	long tmp = RWSEM_ACTIVE_WRITE_BIAS;

One thing I'm still not clear on is why we need the output tmp operand:
"=d" (tmp) ?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

#1383776

On April 20, 2016 2:36:37 PM PDT, Borislav Petkov <bp@alien8.de> wrote:
>On Wed, Apr 20, 2016 at 02:06:33PM -0700, H. Peter Anvin wrote:
>> Setting ret to sem doesn't make any sense.  Just use "=a" and "a".
>
>Yeah, that's what Michal's patch ontop does.
>
>And to answer my own question: we need the "a" (sem) input for the fast
>path.
>
>I guess we can still move "1" (RWSEM_ACTIVE_WRITE_BIAS) before the
>asm():
>
>	long tmp = RWSEM_ACTIVE_WRITE_BIAS;
>
>One thing I'm still not clear on is why we need the output tmp operand:
>"=d" (tmp) ?

Since it is a fixed register we could just mark edx clobbered, but with more flexible register constraints it can permit gcc to allocate a temp resister for us.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

#1384110

On Wed, Apr 20, 2016 at 03:29:30PM -0700, H. Peter Anvin wrote:
> Since it is a fixed register we could just mark edx clobbered, but
> with more flexible register constraints it can permit gcc to allocate
> a temp resister for us. --

Right.

I'll try to hack up a cleanup ontop once the dust here settles and Ingo
pushes out the pile.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

#1384195

On Thu 21-04-16 13:35:16, Borislav Petkov wrote:
> On Wed, Apr 20, 2016 at 03:29:30PM -0700, H. Peter Anvin wrote:
> > Since it is a fixed register we could just mark edx clobbered, but
> > with more flexible register constraints it can permit gcc to allocate
> > a temp resister for us. --
> 
> Right.
> 
> I'll try to hack up a cleanup ontop once the dust here settles and Ingo
> pushes out the pile.

Thanks Boris! This is highly appreciated.
-- 
Michal Hocko
SUSE Labs

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

#1384216

On Thu, Apr 21, 2016 at 09:09:11AM -0400, Michal Hocko wrote:
> Thanks Boris! This is highly appreciated.

I know ;-)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

#1388948 — [PATCH] x86/locking/rwsem: Cleanup ____down_write()

On Wed, Apr 20, 2016 at 03:29:30PM -0700, H. Peter Anvin wrote:
> Since it is a fixed register we could just mark edx clobbered, but
> with more flexible register constraints it can permit gcc to allocate
> a temp resister for us.

How about the following?

It boots fine in kvm and the asm changes are only trivial gcc comments
differences:

---
From: Borislav Petkov <bp@suse.de>
Date: Wed, 27 Apr 2016 13:47:32 +0200
Subject: [PATCH] x86/locking/rwsem: Cleanup ____down_write()

Move the RWSEM_ACTIVE_WRITE_BIAS out of the inline asm to reduce the
number of arguments. Also, make it an input argument only (why it was an
output operand, I still don't know...).

For better readability, use symbolic names for the arguments and move
the linebreak backspace to 80 cols.

Resulting asm differs only in the temporary gcc variable names and
locations:

  --- before      2016-04-27 13:39:05.320778458 +0200
  +++ after       2016-04-27 13:52:37.336778994 +0200
  @@ -11,8 +11,8 @@ down_write_killable:
   .LBB84:
   .LBB85:
   .LBB86:
  -        .loc 2 128 0
  -        movabsq $-4294967295, %rdx      #, tmp
  +        .loc 2 130 0
  +        movabsq $-4294967295, %rdx      #, tmp94
           movq    %rdi, %rax      # sem, sem
   .LBE86:
   .LBE85:
  @@ -23,17 +23,17 @@ down_write_killable:
   .LBB89:
   .LBB88:
   .LBB87:
  -        .loc 2 128 0
  +        .loc 2 130 0
   #APP
  -# 128 "./arch/x86/include/asm/rwsem.h" 1
  +# 130 "./arch/x86/include/asm/rwsem.h" 1
           # beginning down_write
           .pushsection .smp_locks,"a"
   .balign 4
   .long 671f - .
   .popsection
   671:
  -        lock;   xadd      %rdx,(%rax)   # tmp, sem
  -          test  %edx , %edx     # tmp
  +        lock;   xadd      %rdx,(%rax)   # tmp94, sem
  +          test  %edx , %edx     # tmp94
             jz        1f
     call call_rwsem_down_write_failed_killable
   1:

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/rwsem.h | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/rwsem.h b/arch/x86/include/asm/rwsem.h
index 453744c1d347..d2f8d10a6d97 100644
--- a/arch/x86/include/asm/rwsem.h
+++ b/arch/x86/include/asm/rwsem.h
@@ -99,23 +99,25 @@ 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;			\
-	asm volatile("# beginning down_write\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? */\
-		     "  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;						\
+#define ____down_write(sem, slow_path)						\
+({										\
+	long tmp = RWSEM_ACTIVE_WRITE_BIAS;					\
+	struct rw_semaphore* ret;						\
+										\
+	asm volatile("# beginning down_write\n\t"				\
+		     LOCK_PREFIX "  xadd      %[tmp],(%[sem])\n\t"		\
+		     /* adds 0xffff0001, returns the old value */		\
+		     "  test " __ASM_SEL(%w[tmp],%k[tmp]) ","			\
+			       __ASM_SEL(%w[tmp],%k[tmp]) "\n\t"		\
+		     /* was the active mask 0 before? */			\
+		     "  jz        1f\n"						\
+		     "  call " slow_path "\n"					\
+		     "1:\n"							\
+		     "# ending down_write"					\
+		     : "+m" (sem->count), "=a" (ret)				\
+		     : [sem] "a" (sem), [tmp] "r" (tmp)				\
+		     : "memory", "cc");						\
+	ret;									\
 })
 
 static inline void __down_write(struct rw_semaphore *sem)
-- 
2.7.3


-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

#1389973 — [tip:locking/rwsem] locking/rwsem, x86: Clean up ____down_write()

Commit-ID:  71c01930b42e5dd65d4820dea116bcbe95a0b768
Gitweb:     http://git.kernel.org/tip/71c01930b42e5dd65d4820dea116bcbe95a0b768
Author:     Borislav Petkov <bp@suse.de>
AuthorDate: Wed, 27 Apr 2016 13:47:32 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 28 Apr 2016 10:42:56 +0200

locking/rwsem, x86: Clean up ____down_write()

Move the RWSEM_ACTIVE_WRITE_BIAS out of the inline asm to reduce the
number of arguments. Also, make it an input argument only (why it was an
output operand, I still don't know...).

For better readability, use symbolic names for the arguments and move
the linebreak backspace to 80 cols.

Resulting asm differs only in the temporary GCC variable names and
locations:

  -- before      2016-04-27 13:39:05.320778458 +0200
  ++ after       2016-04-27 13:52:37.336778994 +0200
  @@ -11,8 +11,8 @@ down_write_killable:
   .LBB84:
   .LBB85:
   .LBB86:
  -        .loc 2 128 0
  -        movabsq $-4294967295, %rdx      #, tmp
  +        .loc 2 130 0
  +        movabsq $-4294967295, %rdx      #, tmp94
           movq    %rdi, %rax      # sem, sem
   .LBE86:
   .LBE85:
  @@ -23,17 +23,17 @@ down_write_killable:
   .LBB89:
   .LBB88:
   .LBB87:
  -        .loc 2 128 0
  +        .loc 2 130 0
   #APP
  -# 128 "./arch/x86/include/asm/rwsem.h" 1
  +# 130 "./arch/x86/include/asm/rwsem.h" 1
           # beginning down_write
           .pushsection .smp_locks,"a"
   .balign 4
   .long 671f - .
   .popsection
   671:
  -        lock;   xadd      %rdx,(%rax)   # tmp, sem
  -          test  %edx , %edx     # tmp
  +        lock;   xadd      %rdx,(%rax)   # tmp94, sem
  +          test  %edx , %edx     # tmp94
             jz        1f
     call call_rwsem_down_write_failed_killable
   1:

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Chris Zankel <chris@zankel.net>
Cc: David S. Miller <davem@davemloft.net>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: linux-alpha@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: linux-ia64@vger.kernel.org
Cc: linux-s390@vger.kernel.org
Cc: linux-sh@vger.kernel.org
Cc: linux-xtensa@linux-xtensa.org
Cc: sparclinux@vger.kernel.org
Link: http://lkml.kernel.org/r/20160427120217.GE21011@pd.tnic
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/rwsem.h | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/rwsem.h b/arch/x86/include/asm/rwsem.h
index 453744c..d2f8d10 100644
--- a/arch/x86/include/asm/rwsem.h
+++ b/arch/x86/include/asm/rwsem.h
@@ -99,23 +99,25 @@ 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;			\
-	asm volatile("# beginning down_write\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? */\
-		     "  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;						\
+#define ____down_write(sem, slow_path)						\
+({										\
+	long tmp = RWSEM_ACTIVE_WRITE_BIAS;					\
+	struct rw_semaphore* ret;						\
+										\
+	asm volatile("# beginning down_write\n\t"				\
+		     LOCK_PREFIX "  xadd      %[tmp],(%[sem])\n\t"		\
+		     /* adds 0xffff0001, returns the old value */		\
+		     "  test " __ASM_SEL(%w[tmp],%k[tmp]) ","			\
+			       __ASM_SEL(%w[tmp],%k[tmp]) "\n\t"		\
+		     /* was the active mask 0 before? */			\
+		     "  jz        1f\n"						\
+		     "  call " slow_path "\n"					\
+		     "1:\n"							\
+		     "# ending down_write"					\
+		     : "+m" (sem->count), "=a" (ret)				\
+		     : [sem] "a" (sem), [tmp] "r" (tmp)				\
+		     : "memory", "cc");						\
+	ret;									\
 })
 
 static inline void __down_write(struct rw_semaphore *sem)

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

#1384726

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Apr 13, 2016 at 02:49:43PM +0200, Michal Hocko wrote:
> > 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.
> 
> I completely blow at this gcc-asm constraints thing too :/
> 
> In any case, Ingo will you look after the rest of these patches, or do
> you want me to pick up the remaining bits?

Yeah, it's on my list!

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web