LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] semaphore: use raw_spin_lock_irq instead of raw_spin_lock_irqsave
@ 2018-05-30 15:24 Mikulas Patocka
  2018-06-21 13:50 ` Ingo Molnar
  0 siblings, 1 reply; 6+ messages in thread
From: Mikulas Patocka @ 2018-05-30 15:24 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Matthew Wilcox; +Cc: linux-kernel

The sleeping functions down, down_interruptible, down_killable and
down_timeout can't be called with interrupts disabled, so we don't have to
save and restore interrupt flag.

This patch avoids the costly pushf and popf instructions on the semaphore
path.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 kernel/locking/semaphore.c |   21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

Index: linux-2.6/kernel/locking/semaphore.c
===================================================================
--- linux-2.6.orig/kernel/locking/semaphore.c	2017-06-03 00:22:44.000000000 +0200
+++ linux-2.6/kernel/locking/semaphore.c	2018-05-30 14:46:35.000000000 +0200
@@ -53,14 +53,12 @@ static noinline void __up(struct semapho
  */
 void down(struct semaphore *sem)
 {
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&sem->lock, flags);
+	raw_spin_lock_irq(&sem->lock);
 	if (likely(sem->count > 0))
 		sem->count--;
 	else
 		__down(sem);
-	raw_spin_unlock_irqrestore(&sem->lock, flags);
+	raw_spin_unlock_irq(&sem->lock);
 }
 EXPORT_SYMBOL(down);
 
@@ -75,15 +73,14 @@ EXPORT_SYMBOL(down);
  */
 int down_interruptible(struct semaphore *sem)
 {
-	unsigned long flags;
 	int result = 0;
 
-	raw_spin_lock_irqsave(&sem->lock, flags);
+	raw_spin_lock_irq(&sem->lock);
 	if (likely(sem->count > 0))
 		sem->count--;
 	else
 		result = __down_interruptible(sem);
-	raw_spin_unlock_irqrestore(&sem->lock, flags);
+	raw_spin_unlock_irq(&sem->lock);
 
 	return result;
 }
@@ -101,15 +98,14 @@ EXPORT_SYMBOL(down_interruptible);
  */
 int down_killable(struct semaphore *sem)
 {
-	unsigned long flags;
 	int result = 0;
 
-	raw_spin_lock_irqsave(&sem->lock, flags);
+	raw_spin_lock_irq(&sem->lock);
 	if (likely(sem->count > 0))
 		sem->count--;
 	else
 		result = __down_killable(sem);
-	raw_spin_unlock_irqrestore(&sem->lock, flags);
+	raw_spin_unlock_irq(&sem->lock);
 
 	return result;
 }
@@ -155,15 +151,14 @@ EXPORT_SYMBOL(down_trylock);
  */
 int down_timeout(struct semaphore *sem, long timeout)
 {
-	unsigned long flags;
 	int result = 0;
 
-	raw_spin_lock_irqsave(&sem->lock, flags);
+	raw_spin_lock_irq(&sem->lock);
 	if (likely(sem->count > 0))
 		sem->count--;
 	else
 		result = __down_timeout(sem, timeout);
-	raw_spin_unlock_irqrestore(&sem->lock, flags);
+	raw_spin_unlock_irq(&sem->lock);
 
 	return result;
 }

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] semaphore: use raw_spin_lock_irq instead of raw_spin_lock_irqsave
  2018-05-30 15:24 [PATCH] semaphore: use raw_spin_lock_irq instead of raw_spin_lock_irqsave Mikulas Patocka
@ 2018-06-21 13:50 ` Ingo Molnar
  2018-06-21 14:02   ` Ingo Molnar
  0 siblings, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2018-06-21 13:50 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Peter Zijlstra, Ingo Molnar, Matthew Wilcox, linux-kernel,
	Thomas Gleixner


* Mikulas Patocka <mpatocka@redhat.com> wrote:

> The sleeping functions down, down_interruptible, down_killable and
> down_timeout can't be called with interrupts disabled, so we don't have to
> save and restore interrupt flag.
> 
> This patch avoids the costly pushf and popf instructions on the semaphore
> path.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  kernel/locking/semaphore.c |   21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)

I've applied this to the locking tree, I suspect we can do this on the condition 
that it doesn't explode in early boot code (which has irqs disabled) and doesn't 
generate early boot lockdep splats either.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] semaphore: use raw_spin_lock_irq instead of raw_spin_lock_irqsave
  2018-06-21 13:50 ` Ingo Molnar
@ 2018-06-21 14:02   ` Ingo Molnar
  2018-06-21 14:17     ` Ingo Molnar
  2018-06-21 14:20     ` Juergen Gross
  0 siblings, 2 replies; 6+ messages in thread
From: Ingo Molnar @ 2018-06-21 14:02 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Peter Zijlstra, Ingo Molnar, Matthew Wilcox, linux-kernel,
	Thomas Gleixner


* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > The sleeping functions down, down_interruptible, down_killable and
> > down_timeout can't be called with interrupts disabled, so we don't have to
> > save and restore interrupt flag.
> > 
> > This patch avoids the costly pushf and popf instructions on the semaphore
> > path.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > ---
> >  kernel/locking/semaphore.c |   21 ++++++++-------------
> >  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> I've applied this to the locking tree, I suspect we can do this on the condition 
> that it doesn't explode in early boot code (which has irqs disabled) and doesn't 
> generate early boot lockdep splats either.

Hm, this blew up pretty quick on a pretty regular x86-64 PC white-box, during 
early bootup:

  PANIC: early exception 0x08 IP 246:10 error ffffffff811537b2 cr2 0xffff88000240cff8

and I think it's due to your patch - verifying that now.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] semaphore: use raw_spin_lock_irq instead of raw_spin_lock_irqsave
  2018-06-21 14:02   ` Ingo Molnar
@ 2018-06-21 14:17     ` Ingo Molnar
  2018-06-21 14:20     ` Juergen Gross
  1 sibling, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2018-06-21 14:17 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Peter Zijlstra, Ingo Molnar, Matthew Wilcox, linux-kernel,
	Thomas Gleixner


* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Ingo Molnar <mingo@kernel.org> wrote:
> 
> > 
> > * Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> > > The sleeping functions down, down_interruptible, down_killable and
> > > down_timeout can't be called with interrupts disabled, so we don't have to
> > > save and restore interrupt flag.
> > > 
> > > This patch avoids the costly pushf and popf instructions on the semaphore
> > > path.
> > > 
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > 
> > > ---
> > >  kernel/locking/semaphore.c |   21 ++++++++-------------
> > >  1 file changed, 8 insertions(+), 13 deletions(-)
> > 
> > I've applied this to the locking tree, I suspect we can do this on the condition 
> > that it doesn't explode in early boot code (which has irqs disabled) and doesn't 
> > generate early boot lockdep splats either.
> 
> Hm, this blew up pretty quick on a pretty regular x86-64 PC white-box, during 
> early bootup:
> 
>   PANIC: early exception 0x08 IP 246:10 error ffffffff811537b2 cr2 0xffff88000240cff8
> 
> and I think it's due to your patch - verifying that now.

Yeah, went away after removing that patch. So I'm skipping this for now.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] semaphore: use raw_spin_lock_irq instead of raw_spin_lock_irqsave
  2018-06-21 14:02   ` Ingo Molnar
  2018-06-21 14:17     ` Ingo Molnar
@ 2018-06-21 14:20     ` Juergen Gross
  2018-06-21 14:28       ` Ingo Molnar
  1 sibling, 1 reply; 6+ messages in thread
From: Juergen Gross @ 2018-06-21 14:20 UTC (permalink / raw)
  To: Ingo Molnar, Mikulas Patocka
  Cc: Peter Zijlstra, Ingo Molnar, Matthew Wilcox, linux-kernel,
	Thomas Gleixner

On 21/06/18 16:02, Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@kernel.org> wrote:
> 
>>
>> * Mikulas Patocka <mpatocka@redhat.com> wrote:
>>
>>> The sleeping functions down, down_interruptible, down_killable and
>>> down_timeout can't be called with interrupts disabled, so we don't have to
>>> save and restore interrupt flag.
>>>
>>> This patch avoids the costly pushf and popf instructions on the semaphore
>>> path.
>>>
>>> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
>>>
>>> ---
>>>  kernel/locking/semaphore.c |   21 ++++++++-------------
>>>  1 file changed, 8 insertions(+), 13 deletions(-)
>>
>> I've applied this to the locking tree, I suspect we can do this on the condition 
>> that it doesn't explode in early boot code (which has irqs disabled) and doesn't 
>> generate early boot lockdep splats either.
> 
> Hm, this blew up pretty quick on a pretty regular x86-64 PC white-box, during 
> early bootup:
> 
>   PANIC: early exception 0x08 IP 246:10 error ffffffff811537b2 cr2 0xffff88000240cff8
> 
> and I think it's due to your patch - verifying that now.

I guess local_irq_enable() being called by raw_spin_unlock_irq() makes
its usage in early boot code undesirable.

Maybe it would be possible to use alternatives for that case? They are
applied after enabling interrupts, so pushf and popf instructions could
be patched away.


Juergen

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] semaphore: use raw_spin_lock_irq instead of raw_spin_lock_irqsave
  2018-06-21 14:20     ` Juergen Gross
@ 2018-06-21 14:28       ` Ingo Molnar
  0 siblings, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2018-06-21 14:28 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Mikulas Patocka, Peter Zijlstra, Ingo Molnar, Matthew Wilcox,
	linux-kernel, Thomas Gleixner, Peter Zijlstra


* Juergen Gross <jgross@suse.com> wrote:

> On 21/06/18 16:02, Ingo Molnar wrote:
> > 
> > * Ingo Molnar <mingo@kernel.org> wrote:
> > 
> >>
> >> * Mikulas Patocka <mpatocka@redhat.com> wrote:
> >>
> >>> The sleeping functions down, down_interruptible, down_killable and
> >>> down_timeout can't be called with interrupts disabled, so we don't have to
> >>> save and restore interrupt flag.
> >>>
> >>> This patch avoids the costly pushf and popf instructions on the semaphore
> >>> path.
> >>>
> >>> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> >>>
> >>> ---
> >>>  kernel/locking/semaphore.c |   21 ++++++++-------------
> >>>  1 file changed, 8 insertions(+), 13 deletions(-)
> >>
> >> I've applied this to the locking tree, I suspect we can do this on the condition 
> >> that it doesn't explode in early boot code (which has irqs disabled) and doesn't 
> >> generate early boot lockdep splats either.
> > 
> > Hm, this blew up pretty quick on a pretty regular x86-64 PC white-box, during 
> > early bootup:
> > 
> >   PANIC: early exception 0x08 IP 246:10 error ffffffff811537b2 cr2 0xffff88000240cff8
> > 
> > and I think it's due to your patch - verifying that now.
> 
> I guess local_irq_enable() being called by raw_spin_unlock_irq() makes
> its usage in early boot code undesirable.
> 
> Maybe it would be possible to use alternatives for that case? They are
> applied after enabling interrupts, so pushf and popf instructions could
> be patched away.

Or we could just leave the code as-is: the performance difference between CLI/STI 
and PUSHF/POPF shouldn't be significant, plus the semaphore APIs are obsolete and 
should not be used.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-06-21 14:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-30 15:24 [PATCH] semaphore: use raw_spin_lock_irq instead of raw_spin_lock_irqsave Mikulas Patocka
2018-06-21 13:50 ` Ingo Molnar
2018-06-21 14:02   ` Ingo Molnar
2018-06-21 14:17     ` Ingo Molnar
2018-06-21 14:20     ` Juergen Gross
2018-06-21 14:28       ` Ingo Molnar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).