Introduce atomic_dec_and_lock_irqsave()

Introduce atomic_dec_and_lock_irqsave()

[Sebastian Andrzej Siewior]

This series introduces atomic_dec_and_lock_irqsave() and converts a few
users to use it. They were using local_irq_save() +
atomic_dec_and_lock() before that series.

Sebastian

[PATCH 1/5] spinlock: atomic_dec_and_lock: Add an irqsave variant

[Sebastian Andrzej Siewior]

From: Anna-Maria Gleixner <anna-maria@linutronix.de>

There are in-tree users of atomic_dec_and_lock() which must acquire the
spin lock with interrupts disabled. To workaround the lack of an irqsave
variant of atomic_dec_and_lock() they use local_irq_save() at the call
site. This causes extra code and creates in some places unneeded long
interrupt disabled times. These places need also extra treatment for
PREEMPT_RT due to the disconnect of the irq disabling and the lock
function.

Implement the missing irqsave variant of the function.

Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/spinlock.h |  5 +++++
 lib/dec_and_lock.c       | 16 ++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 4894d322d258..803536c992f5 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -409,6 +409,11 @@ extern int _atomic_dec_and_lock(atomic_t *atomic, spinlock_t *lock);
 #define atomic_dec_and_lock(atomic, lock) \
 		__cond_lock(lock, _atomic_dec_and_lock(atomic, lock))
 
+extern int _atomic_dec_and_lock_irqsave(atomic_t *atomic, spinlock_t *lock,
+					unsigned long *flags);
+#define atomic_dec_and_lock_irqsave(atomic, lock, flags) \
+		__cond_lock(lock, _atomic_dec_and_lock_irqsave(atomic, lock, &(flags)))
+
 int alloc_bucket_spinlocks(spinlock_t **locks, unsigned int *lock_mask,
 			   size_t max_size, unsigned int cpu_mult,
 			   gfp_t gfp);
diff --git a/lib/dec_and_lock.c b/lib/dec_and_lock.c
index 347fa7ac2e8a..9555b68bb774 100644
--- a/lib/dec_and_lock.c
+++ b/lib/dec_and_lock.c
@@ -33,3 +33,19 @@ int _atomic_dec_and_lock(atomic_t *atomic, spinlock_t *lock)
 }
 
 EXPORT_SYMBOL(_atomic_dec_and_lock);
+
+int _atomic_dec_and_lock_irqsave(atomic_t *atomic, spinlock_t *lock,
+				 unsigned long *flags)
+{
+	/* Subtract 1 from counter unless that drops it to 0 (ie. it was 1) */
+	if (atomic_add_unless(atomic, -1, 1))
+		return 0;
+
+	/* Otherwise do it the slow way */
+	spin_lock_irqsave(lock, *flags);
+	if (atomic_dec_and_test(atomic))
+		return 1;
+	spin_unlock_irqrestore(lock, *flags);
+	return 0;
+}
+EXPORT_SYMBOL(_atomic_dec_and_lock_irqsave);
-- 
2.17.0

[PATCH 2/5] mm/backing-dev: Use irqsave variant of atomic_dec_and_lock()

[Sebastian Andrzej Siewior]

From: Anna-Maria Gleixner <anna-maria@linutronix.de>

The irqsave variant of atomic_dec_and_lock handles irqsave/restore when
taking/releasing the spin lock. With this variant the call of
local_irq_save/restore is no longer required.

Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 mm/backing-dev.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 023190c69dce..c28418914591 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -484,11 +484,8 @@ void wb_congested_put(struct bdi_writeback_congested *congested)
 {
 	unsigned long flags;
 
-	local_irq_save(flags);
-	if (!atomic_dec_and_lock(&congested->refcnt, &cgwb_lock)) {
-		local_irq_restore(flags);
+	if (!atomic_dec_and_lock_irqsave(&congested->refcnt, &cgwb_lock, flags))
 		return;
-	}
 
 	/* bdi might already have been destroyed leaving @congested unlinked */
 	if (congested->__bdi) {
-- 
2.17.0

[PATCH 3/5] kernel/user: Use irqsave variant of atomic_dec_and_lock()

[Sebastian Andrzej Siewior]

From: Anna-Maria Gleixner <anna-maria@linutronix.de>

The irqsave variant of atomic_dec_and_lock handles irqsave/restore when
taking/releasing the spin lock. With this variant the call of
local_irq_save/restore is no longer required.

Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/user.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/user.c b/kernel/user.c
index 36288d840675..8959ad11d766 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -169,11 +169,8 @@ void free_uid(struct user_struct *up)
 	if (!up)
 		return;
 
-	local_irq_save(flags);
-	if (atomic_dec_and_lock(&up->__count, &uidhash_lock))
+	if (atomic_dec_and_lock_irqsave(&up->__count, &uidhash_lock, flags))
 		free_user(up, flags);
-	else
-		local_irq_restore(flags);
 }
 
 struct user_struct *alloc_uid(kuid_t uid)
-- 
2.17.0

[PATCH 4/5] drivers/md/raid5: Use irqsave variant of atomic_dec_and_lock()

[Sebastian Andrzej Siewior]

From: Anna-Maria Gleixner <anna-maria@linutronix.de>

The irqsave variant of atomic_dec_and_lock handles irqsave/restore when
taking/releasing the spin lock. With this variant the call of
local_irq_save is no longer required.

Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/md/raid5.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index be117d0a65a8..0a5e6f5d271d 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -409,16 +409,15 @@ void raid5_release_stripe(struct stripe_head *sh)
 		md_wakeup_thread(conf->mddev->thread);
 	return;
 slow_path:
-	local_irq_save(flags);
 	/* we are ok here if STRIPE_ON_RELEASE_LIST is set or not */
-	if (atomic_dec_and_lock(&sh->count, &conf->device_lock)) {
+	if (atomic_dec_and_lock_irqsave(&sh->count, &conf->device_lock, flags)) {
 		INIT_LIST_HEAD(&list);
 		hash = sh->hash_lock_index;
 		do_release_stripe(conf, sh, &list);
 		spin_unlock(&conf->device_lock);
 		release_inactive_stripe_list(conf, &list, hash);
+		local_irq_restore(flags);
 	}
-	local_irq_restore(flags);
 }
 
 static inline void remove_hash(struct stripe_head *sh)
-- 
2.17.0

[PATCH 5/5] drivers/md/raid5: Do not disable irq on release_inactive_stripe_list() call

[Sebastian Andrzej Siewior]

From: Anna-Maria Gleixner <anna-maria@linutronix.de>

There is no need to invoke release_inactive_stripe_list() with interrupts
disabled. All call sites, except raid5_release_stripe(), unlock
->device_lock and enable interrupts before invoking the function.

Make it consistent.

Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/md/raid5.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 0a5e6f5d271d..5ee974fcd42e 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -414,9 +414,8 @@ void raid5_release_stripe(struct stripe_head *sh)
 		INIT_LIST_HEAD(&list);
 		hash = sh->hash_lock_index;
 		do_release_stripe(conf, sh, &list);
-		spin_unlock(&conf->device_lock);
+		spin_unlock_irqrestore(&conf->device_lock, flags);
 		release_inactive_stripe_list(conf, &list, hash);
-		local_irq_restore(flags);
 	}
 }
 
-- 
2.17.0

Re: Introduce atomic_dec_and_lock_irqsave()

[Peter Zijlstra]

On Fri, May 04, 2018 at 05:45:28PM +0200, Sebastian Andrzej Siewior wrote:
> This series introduces atomic_dec_and_lock_irqsave() and converts a few
> users to use it. They were using local_irq_save() +
> atomic_dec_and_lock() before that series.

Should not all these users be converted to refcount_t, and thus, should
we not introduce refcount_dec_and_lock_irqsave() instead?

Re: Introduce atomic_dec_and_lock_irqsave()

[Sebastian Andrzej Siewior]

On 2018-05-04 17:54:46 [+0200], Peter Zijlstra wrote:
> On Fri, May 04, 2018 at 05:45:28PM +0200, Sebastian Andrzej Siewior wrote:
> > This series introduces atomic_dec_and_lock_irqsave() and converts a few
> > users to use it. They were using local_irq_save() +
> > atomic_dec_and_lock() before that series.
> 
> Should not all these users be converted to refcount_t, and thus, should
> we not introduce refcount_dec_and_lock_irqsave() instead?

do you intend to kill refcount_dec_and_lock() in the longterm?
I haz this but instead we do
- atomic_dec_and_lock() -> refcount_dec_and_lock()
- add refcount_dec_and_lock_irqsave()
- patch 2+ use refcount_dec_and_lock_irqsave().

Sebastian

Re: Introduce atomic_dec_and_lock_irqsave()

[Peter Zijlstra]

On Fri, May 04, 2018 at 06:07:26PM +0200, Sebastian Andrzej Siewior wrote:

> do you intend to kill refcount_dec_and_lock() in the longterm?

You meant to say atomic_dec_and_lock() ? Dunno if we ever get there, but
typically dec_and_lock is fairly refcounty, but I suppose it is possible
to have !refcount users, in which case we're eternally stuck with it.

But a quick look at the sites you converted, they all appear to be true
refcounts, and would thus benefit from being converted to refcount_t.

Re: Introduce atomic_dec_and_lock_irqsave()

[Al Viro]

On Fri, May 04, 2018 at 06:21:02PM +0200, Peter Zijlstra wrote:
> On Fri, May 04, 2018 at 06:07:26PM +0200, Sebastian Andrzej Siewior wrote:
> 
> > do you intend to kill refcount_dec_and_lock() in the longterm?
> 
> You meant to say atomic_dec_and_lock() ? Dunno if we ever get there, but
> typically dec_and_lock is fairly refcounty, but I suppose it is possible
> to have !refcount users, in which case we're eternally stuck with it.

Yes, there are - consider e.g.

void iput(struct inode *inode)
{ 
        if (!inode)
                return;
        BUG_ON(inode->i_state & I_CLEAR);
retry:
        if (atomic_dec_and_lock(&inode->i_count, &inode->i_lock)) {

inode->i_count sure as hell isn't refcount_t fodder...

Re: Introduce atomic_dec_and_lock_irqsave()

[Peter Zijlstra]

On Fri, May 04, 2018 at 05:26:40PM +0100, Al Viro wrote:
> On Fri, May 04, 2018 at 06:21:02PM +0200, Peter Zijlstra wrote:
> > On Fri, May 04, 2018 at 06:07:26PM +0200, Sebastian Andrzej Siewior wrote:
> > 
> > > do you intend to kill refcount_dec_and_lock() in the longterm?
> > 
> > You meant to say atomic_dec_and_lock() ? Dunno if we ever get there, but
> > typically dec_and_lock is fairly refcounty, but I suppose it is possible
> > to have !refcount users, in which case we're eternally stuck with it.
> 
> Yes, there are - consider e.g.
> 
> void iput(struct inode *inode)
> { 
>         if (!inode)
>                 return;
>         BUG_ON(inode->i_state & I_CLEAR);
> retry:
>         if (atomic_dec_and_lock(&inode->i_count, &inode->i_lock)) {
> 
> inode->i_count sure as hell isn't refcount_t fodder...

Yeah, I should've remembered, I tried to convert that once ;-) i_count is
a usage count, not a refcount.

Re: Introduce atomic_dec_and_lock_irqsave()

[Peter Zijlstra]

On Fri, May 04, 2018 at 05:45:28PM +0200, Sebastian Andrzej Siewior wrote:
> This series introduces atomic_dec_and_lock_irqsave() and converts a few
> users to use it. They were using local_irq_save() +
> atomic_dec_and_lock() before that series.

1,5-6:

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: Introduce atomic_dec_and_lock_irqsave()

[Sebastian Andrzej Siewior]

On 2018-05-23 15:02:41 [+0200], Peter Zijlstra wrote:
> 1,5-6:
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

I sucked them into my try tree an noticed this off by one, I applied the
tags to 1,4-5:
*┬─>[PATCH 1/5] spinlock: atomic_dec_and_lock: Add an irqsave variant
 ├─>[PATCH 2/5] mm/backing-dev: Use irqsave variant of atomic_dec_and_lock()
 ├─>[PATCH 3/5] kernel/user: Use irqsave variant of atomic_dec_and_lock()
*├─>[PATCH 4/5] drivers/md/raid5: Use irqsave variant of atomic_dec_and_lock()
*├─>[PATCH 5/5] drivers/md/raid5: Do not disable irq on release_inactive_stripe_list() call

as we talked about it.

Sebastian

[PATCH 1/5 v2] spinlock: atomic_dec_and_lock: Add an irqsave variant

[Sebastian Andrzej Siewior]

There are in-tree users of atomic_dec_and_lock() which must acquire the
spin lock with interrupts disabled. To workaround the lack of an irqsave
variant of atomic_dec_and_lock() they use local_irq_save() at the call
site. This causes extra code and creates in some places unneeded long
interrupt disabled times. These places need also extra treatment for
PREEMPT_RT due to the disconnect of the irq disabling and the lock
function.

Implement the missing irqsave variant of the function.
Add a stub for Alpha which should work without the assmebly optimisation
for the fastpath.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> [not the Alpha bits]
Cc: Richard Henderson <rth@twiddle.net>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
[bigeasy: adding Alpha bits]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: Add Alpha bits (kbuild reported build failure on Alpha)

 arch/alpha/lib/dec_and_lock.c | 16 ++++++++++++++++
 include/linux/spinlock.h      |  5 +++++
 lib/dec_and_lock.c            | 16 ++++++++++++++++
 3 files changed, 37 insertions(+)

diff --git a/arch/alpha/lib/dec_and_lock.c b/arch/alpha/lib/dec_and_lock.c
index a117707f57fe..069fef7372dc 100644
--- a/arch/alpha/lib/dec_and_lock.c
+++ b/arch/alpha/lib/dec_and_lock.c
@@ -42,3 +42,19 @@ static int __used atomic_dec_and_lock_1(atomic_t *atomic, spinlock_t *lock)
 	return 0;
 }
 EXPORT_SYMBOL(_atomic_dec_and_lock);
+
+int _atomic_dec_and_lock_irqsave(atomic_t *atomic, spinlock_t *lock,
+				 unsigned long *flags)
+{
+	/* Subtract 1 from counter unless that drops it to 0 (ie. it was 1) */
+	if (atomic_add_unless(atomic, -1, 1))
+		return 0;
+
+	/* Otherwise do it the slow way */
+	spin_lock_irqsave(lock, *flags);
+	if (atomic_dec_and_test(atomic))
+		return 1;
+	spin_unlock_irqrestore(lock, *flags);
+	return 0;
+}
+EXPORT_SYMBOL(_atomic_dec_and_lock_irqsave);
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 4894d322d258..803536c992f5 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -409,6 +409,11 @@ extern int _atomic_dec_and_lock(atomic_t *atomic, spinlock_t *lock);
 #define atomic_dec_and_lock(atomic, lock) \
 		__cond_lock(lock, _atomic_dec_and_lock(atomic, lock))
 
+extern int _atomic_dec_and_lock_irqsave(atomic_t *atomic, spinlock_t *lock,
+					unsigned long *flags);
+#define atomic_dec_and_lock_irqsave(atomic, lock, flags) \
+		__cond_lock(lock, _atomic_dec_and_lock_irqsave(atomic, lock, &(flags)))
+
 int alloc_bucket_spinlocks(spinlock_t **locks, unsigned int *lock_mask,
 			   size_t max_size, unsigned int cpu_mult,
 			   gfp_t gfp);
diff --git a/lib/dec_and_lock.c b/lib/dec_and_lock.c
index 347fa7ac2e8a..9555b68bb774 100644
--- a/lib/dec_and_lock.c
+++ b/lib/dec_and_lock.c
@@ -33,3 +33,19 @@ int _atomic_dec_and_lock(atomic_t *atomic, spinlock_t *lock)
 }
 
 EXPORT_SYMBOL(_atomic_dec_and_lock);
+
+int _atomic_dec_and_lock_irqsave(atomic_t *atomic, spinlock_t *lock,
+				 unsigned long *flags)
+{
+	/* Subtract 1 from counter unless that drops it to 0 (ie. it was 1) */
+	if (atomic_add_unless(atomic, -1, 1))
+		return 0;
+
+	/* Otherwise do it the slow way */
+	spin_lock_irqsave(lock, *flags);
+	if (atomic_dec_and_test(atomic))
+		return 1;
+	spin_unlock_irqrestore(lock, *flags);
+	return 0;
+}
+EXPORT_SYMBOL(_atomic_dec_and_lock_irqsave);
-- 
2.17.1

[PATCH 1/5 v2] spinlock: atomic_dec_and_lock: Add an irqsave variant

[Sebastian Andrzej Siewior]

There are in-tree users of atomic_dec_and_lock() which must acquire the
spin lock with interrupts disabled. To workaround the lack of an irqsave
variant of atomic_dec_and_lock() they use local_irq_save() at the call
site. This causes extra code and creates in some places unneeded long
interrupt disabled times. These places need also extra treatment for
PREEMPT_RT due to the disconnect of the irq disabling and the lock
function.

Implement the missing irqsave variant of the function.
Add a stub for Alpha which should work without the assmebly optimisation
for the fastpath.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> [not the Alpha bits]
Cc: Richard Henderson <rth@twiddle.net>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
[bigeasy: adding Alpha bits]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: Add Alpha bits (kbuild reported build failure on Alpha)

 arch/alpha/lib/dec_and_lock.c | 16 ++++++++++++++++
 include/linux/spinlock.h      |  5 +++++
 lib/dec_and_lock.c            | 16 ++++++++++++++++
 3 files changed, 37 insertions(+)

diff --git a/arch/alpha/lib/dec_and_lock.c b/arch/alpha/lib/dec_and_lock.c
index a117707f57fe..069fef7372dc 100644
--- a/arch/alpha/lib/dec_and_lock.c
+++ b/arch/alpha/lib/dec_and_lock.c
@@ -42,3 +42,19 @@ static int __used atomic_dec_and_lock_1(atomic_t *atomic, spinlock_t *lock)
 	return 0;
 }
 EXPORT_SYMBOL(_atomic_dec_and_lock);
+
+int _atomic_dec_and_lock_irqsave(atomic_t *atomic, spinlock_t *lock,
+				 unsigned long *flags)
+{
+	/* Subtract 1 from counter unless that drops it to 0 (ie. it was 1) */
+	if (atomic_add_unless(atomic, -1, 1))
+		return 0;
+
+	/* Otherwise do it the slow way */
+	spin_lock_irqsave(lock, *flags);
+	if (atomic_dec_and_test(atomic))
+		return 1;
+	spin_unlock_irqrestore(lock, *flags);
+	return 0;
+}
+EXPORT_SYMBOL(_atomic_dec_and_lock_irqsave);
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 4894d322d258..803536c992f5 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -409,6 +409,11 @@ extern int _atomic_dec_and_lock(atomic_t *atomic, spinlock_t *lock);
 #define atomic_dec_and_lock(atomic, lock) \
 		__cond_lock(lock, _atomic_dec_and_lock(atomic, lock))
 
+extern int _atomic_dec_and_lock_irqsave(atomic_t *atomic, spinlock_t *lock,
+					unsigned long *flags);
+#define atomic_dec_and_lock_irqsave(atomic, lock, flags) \
+		__cond_lock(lock, _atomic_dec_and_lock_irqsave(atomic, lock, &(flags)))
+
 int alloc_bucket_spinlocks(spinlock_t **locks, unsigned int *lock_mask,
 			   size_t max_size, unsigned int cpu_mult,
 			   gfp_t gfp);
diff --git a/lib/dec_and_lock.c b/lib/dec_and_lock.c
index 347fa7ac2e8a..9555b68bb774 100644
--- a/lib/dec_and_lock.c
+++ b/lib/dec_and_lock.c
@@ -33,3 +33,19 @@ int _atomic_dec_and_lock(atomic_t *atomic, spinlock_t *lock)
 }
 
 EXPORT_SYMBOL(_atomic_dec_and_lock);
+
+int _atomic_dec_and_lock_irqsave(atomic_t *atomic, spinlock_t *lock,
+				 unsigned long *flags)
+{
+	/* Subtract 1 from counter unless that drops it to 0 (ie. it was 1) */
+	if (atomic_add_unless(atomic, -1, 1))
+		return 0;
+
+	/* Otherwise do it the slow way */
+	spin_lock_irqsave(lock, *flags);
+	if (atomic_dec_and_test(atomic))
+		return 1;
+	spin_unlock_irqrestore(lock, *flags);
+	return 0;
+}
+EXPORT_SYMBOL(_atomic_dec_and_lock_irqsave);
-- 
2.17.1

[PATCH 1.5/5] alpha: atomic: provide asm for the fastpath for _atomic_dec_and_lock_irqsave

[Sebastian Andrzej Siewior]

I just looked at Alpha's atomic_dec_and_lock assembly and did something
that should work for atomic_dec_and_lock_irqsave. I think it works but I
would prefer for someone from the Alpha-Camp to ack this before it goes
in. It is not critical because the non-optimized version should work.

Cc: Richard Henderson <rth@twiddle.net>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/alpha/lib/dec_and_lock.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/arch/alpha/lib/dec_and_lock.c b/arch/alpha/lib/dec_and_lock.c
index 069fef7372dc..d29ba1de4f68 100644
--- a/arch/alpha/lib/dec_and_lock.c
+++ b/arch/alpha/lib/dec_and_lock.c
@@ -32,6 +32,28 @@ _atomic_dec_and_lock:				\n\
 	.previous				\n\
 	.end _atomic_dec_and_lock");
 
+  asm (".text					\n\
+	.global _atomic_dec_and_lock_irqsave	\n\
+	.ent _atomic_dec_and_lock_irqsave	\n\
+	.align	4				\n\
+_atomic_dec_and_lock_irqsave:			\n\
+	.prologue 0				\n\
+1:	ldl_l	$1, 0($16)			\n\
+	subl	$1, 1, $1			\n\
+	beq	$1, 2f				\n\
+	stl_c	$1, 0($16)			\n\
+	beq	$1, 4f				\n\
+	mb					\n\
+	clr	$0				\n\
+	ret					\n\
+2:	br	$29, 3f				\n\
+3:	ldgp	$29, 0($29)			\n\
+	br	$atomic_dec_and_lock_irqsave1..ng	\n\
+	.subsection 2				\n\
+4:	br	1b				\n\
+	.previous				\n\
+	.end _atomic_dec_and_lock_irqsave");
+
 static int __used atomic_dec_and_lock_1(atomic_t *atomic, spinlock_t *lock)
 {
 	/* Slow path */
@@ -43,14 +65,11 @@ static int __used atomic_dec_and_lock_1(atomic_t *atomic, spinlock_t *lock)
 }
 EXPORT_SYMBOL(_atomic_dec_and_lock);
 
-int _atomic_dec_and_lock_irqsave(atomic_t *atomic, spinlock_t *lock,
-				 unsigned long *flags)
+static int __used atomic_dec_and_lock_irqsave1(atomic_t *atomic,
+					       spinlock_t *lock,
+					       unsigned long *flags)
 {
-	/* Subtract 1 from counter unless that drops it to 0 (ie. it was 1) */
-	if (atomic_add_unless(atomic, -1, 1))
-		return 0;
-
-	/* Otherwise do it the slow way */
+	/* Slow way */
 	spin_lock_irqsave(lock, *flags);
 	if (atomic_dec_and_test(atomic))
 		return 1;
-- 
2.17.1

Re: [PATCH 1.5/5] alpha: atomic: provide asm for the fastpath for _atomic_dec_and_lock_irqsave

[Peter Zijlstra]

On Mon, Jun 04, 2018 at 12:27:57PM +0200, Sebastian Andrzej Siewior wrote:
> I just looked at Alpha's atomic_dec_and_lock assembly and did something
> that should work for atomic_dec_and_lock_irqsave. I think it works but I
> would prefer for someone from the Alpha-Camp to ack this before it goes
> in. It is not critical because the non-optimized version should work.

I would vote to simply delete this entire file and get alpha on the
generic code.

Afaict, this asm gets the ordering wrong, and I doubt it is much faster
than using atomic_add_unless() in any case (+- the ordering of course).

>  arch/alpha/lib/dec_and_lock.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/alpha/lib/dec_and_lock.c b/arch/alpha/lib/dec_and_lock.c
> index 069fef7372dc..d29ba1de4f68 100644
> --- a/arch/alpha/lib/dec_and_lock.c
> +++ b/arch/alpha/lib/dec_and_lock.c
> @@ -32,6 +32,28 @@ _atomic_dec_and_lock:				\n\
>  	.previous				\n\
>  	.end _atomic_dec_and_lock");
>  
> +  asm (".text					\n\
> +	.global _atomic_dec_and_lock_irqsave	\n\
> +	.ent _atomic_dec_and_lock_irqsave	\n\
> +	.align	4				\n\
> +_atomic_dec_and_lock_irqsave:			\n\
> +	.prologue 0				\n\
> +1:	ldl_l	$1, 0($16)			\n\
> +	subl	$1, 1, $1			\n\
> +	beq	$1, 2f				\n\
> +	stl_c	$1, 0($16)			\n\
> +	beq	$1, 4f				\n\
> +	mb					\n\
> +	clr	$0				\n\
> +	ret					\n\
> +2:	br	$29, 3f				\n\
> +3:	ldgp	$29, 0($29)			\n\
> +	br	$atomic_dec_and_lock_irqsave1..ng	\n\
> +	.subsection 2				\n\
> +4:	br	1b				\n\
> +	.previous				\n\
> +	.end _atomic_dec_and_lock_irqsave");
> +
>  static int __used atomic_dec_and_lock_1(atomic_t *atomic, spinlock_t *lock)
>  {
>  	/* Slow path */
> @@ -43,14 +65,11 @@ static int __used atomic_dec_and_lock_1(atomic_t *atomic, spinlock_t *lock)
>  }
>  EXPORT_SYMBOL(_atomic_dec_and_lock);
>  
> -int _atomic_dec_and_lock_irqsave(atomic_t *atomic, spinlock_t *lock,
> -				 unsigned long *flags)
> +static int __used atomic_dec_and_lock_irqsave1(atomic_t *atomic,
> +					       spinlock_t *lock,
> +					       unsigned long *flags)
>  {
> -	/* Subtract 1 from counter unless that drops it to 0 (ie. it was 1) */
> -	if (atomic_add_unless(atomic, -1, 1))
> -		return 0;
> -
> -	/* Otherwise do it the slow way */
> +	/* Slow way */
>  	spin_lock_irqsave(lock, *flags);
>  	if (atomic_dec_and_test(atomic))
>  		return 1;
> -- 
> 2.17.1
> 

Re: [PATCH 1.5/5] alpha: atomic: provide asm for the fastpath for _atomic_dec_and_lock_irqsave

[Sebastian Andrzej Siewior]

On 2018-06-04 13:48:52 [+0200], Peter Zijlstra wrote:
> On Mon, Jun 04, 2018 at 12:27:57PM +0200, Sebastian Andrzej Siewior wrote:
> > I just looked at Alpha's atomic_dec_and_lock assembly and did something
> > that should work for atomic_dec_and_lock_irqsave. I think it works but I
> > would prefer for someone from the Alpha-Camp to ack this before it goes
> > in. It is not critical because the non-optimized version should work.
> 
> I would vote to simply delete this entire file and get alpha on the
> generic code.
> 
> Afaict, this asm gets the ordering wrong, and I doubt it is much faster
> than using atomic_add_unless() in any case (+- the ordering of course).

I *think* the Alpha version is slightly wrong here. It does
	load
	dec by one
	cmpeq

while the __atomic_add_unless() implementation does
	load
	cmpeq
	
which is the right thing (unless I can't parse the assembly properly).

Sebastian

[PATCH 0.5/5] alpha: remove custom dec_and_lock() implementation

[Sebastian Andrzej Siewior]

Alpha provides a custom implementation of dec_and_lock(). The functions
is split into two parts:
- atomic_add_unless() + return 0 (fast path in assembly)
- remaining part including locking (slow path in C)

Comparing the result of the alpha implementation with the generic
implementation compiled by gcc it looks like the fast path is optimized
by avoiding a stack frame (and reloading the GP), register store and all
this. This is only done in the slowpath.
After marking the slowpath (atomic_dec_and_lock_1()) as "noinline" and
doing the slowpath in C (the atomic_add_unless(atomic, -1, 1) part) I
noticed differences in the resulting assembly:
- the GP is still reloaded
- atomic_add_unless() adds more memory barriers compared to the custom
  assembly
- the custom assembly here does "load, sub, beq" while
  atomic_add_unless() does "load, cmpeq, add, bne". This is okay because
  it compares against zero after subtraction while the generic code
  compares against 1 before.

I'm not sure if avoiding the stack frame (and GP reloading) brings a lot
in terms of performance. I can't tell if the different memory barriers
are an issue but having the same barriers is probably good.
This implementation forces me however to do the same thing for the
upcoming dec_and_lock_irqsave() which I would like to avoid.

I suggest to remove the custom alpha implementation of dec_and_lock()
and if it is an issue (performance wise) we could still inline the fast
path of dec_and_lock().

Cc: Richard Henderson <rth@twiddle.net>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/alpha/Kconfig            |  5 ----
 arch/alpha/lib/Makefile       |  2 --
 arch/alpha/lib/dec_and_lock.c | 44 -----------------------------------
 lib/Makefile                  |  6 +----
 4 files changed, 1 insertion(+), 56 deletions(-)
 delete mode 100644 arch/alpha/lib/dec_and_lock.c

diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
index f19dc31288c8..786649e0fdf9 100644
--- a/arch/alpha/Kconfig
+++ b/arch/alpha/Kconfig
@@ -565,11 +565,6 @@ config SMP
 
 	  If you don't know what to do here, say N.
 
-config HAVE_DEC_LOCK
-	bool
-	depends on SMP
-	default y
-
 config NR_CPUS
 	int "Maximum number of CPUs (2-32)"
 	range 2 32
diff --git a/arch/alpha/lib/Makefile b/arch/alpha/lib/Makefile
index 04f9729de57c..854d5e79979e 100644
--- a/arch/alpha/lib/Makefile
+++ b/arch/alpha/lib/Makefile
@@ -35,8 +35,6 @@ lib-y =	__divqu.o __remqu.o __divlu.o __remlu.o \
 	callback_srm.o srm_puts.o srm_printk.o \
 	fls.o
 
-lib-$(CONFIG_SMP) += dec_and_lock.o
-
 # The division routines are built from single source, with different defines.
 AFLAGS___divqu.o = -DDIV
 AFLAGS___remqu.o =       -DREM
diff --git a/arch/alpha/lib/dec_and_lock.c b/arch/alpha/lib/dec_and_lock.c
deleted file mode 100644
index a117707f57fe..000000000000
--- a/arch/alpha/lib/dec_and_lock.c
+++ /dev/null
@@ -1,44 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * arch/alpha/lib/dec_and_lock.c
- *
- * ll/sc version of atomic_dec_and_lock()
- * 
- */
-
-#include <linux/spinlock.h>
-#include <linux/atomic.h>
-#include <linux/export.h>
-
-  asm (".text					\n\
-	.global _atomic_dec_and_lock		\n\
-	.ent _atomic_dec_and_lock		\n\
-	.align	4				\n\
-_atomic_dec_and_lock:				\n\
-	.prologue 0				\n\
-1:	ldl_l	$1, 0($16)			\n\
-	subl	$1, 1, $1			\n\
-	beq	$1, 2f				\n\
-	stl_c	$1, 0($16)			\n\
-	beq	$1, 4f				\n\
-	mb					\n\
-	clr	$0				\n\
-	ret					\n\
-2:	br	$29, 3f				\n\
-3:	ldgp	$29, 0($29)			\n\
-	br	$atomic_dec_and_lock_1..ng	\n\
-	.subsection 2				\n\
-4:	br	1b				\n\
-	.previous				\n\
-	.end _atomic_dec_and_lock");
-
-static int __used atomic_dec_and_lock_1(atomic_t *atomic, spinlock_t *lock)
-{
-	/* Slow path */
-	spin_lock(lock);
-	if (atomic_dec_and_test(atomic))
-		return 1;
-	spin_unlock(lock);
-	return 0;
-}
-EXPORT_SYMBOL(_atomic_dec_and_lock);
diff --git a/lib/Makefile b/lib/Makefile
index ce20696d5a92..d0c1531c43f4 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -23,7 +23,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
 	 sha1.o chacha20.o irq_regs.o argv_split.o \
 	 flex_proportions.o ratelimit.o show_mem.o \
 	 is_single_threaded.o plist.o decompress.o kobject_uevent.o \
-	 earlycpio.o seq_buf.o siphash.o \
+	 earlycpio.o seq_buf.o siphash.o dec_and_lock.o \
 	 nmi_backtrace.o nodemask.o win_minmax.o
 
 lib-$(CONFIG_PRINTK) += dump_stack.o
@@ -96,10 +96,6 @@ obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
 obj-$(CONFIG_DEBUG_LIST) += list_debug.o
 obj-$(CONFIG_DEBUG_OBJECTS) += debugobjects.o
 
-ifneq ($(CONFIG_HAVE_DEC_LOCK),y)
-  lib-y += dec_and_lock.o
-endif
-
 obj-$(CONFIG_BITREVERSE) += bitrev.o
 obj-$(CONFIG_RATIONAL)	+= rational.o
 obj-$(CONFIG_CRC_CCITT)	+= crc-ccitt.o
-- 
2.17.1

Re: [PATCH 0.5/5] alpha: remove custom dec_and_lock() implementation

[Sebastian Andrzej Siewior]

On 2018-06-06 13:18:50 [+0200], To Peter Zijlstra wrote:
> I suggest to remove the custom alpha implementation of dec_and_lock()
> and if it is an issue (performance wise) we could still inline the fast
> path of dec_and_lock().

For everyone who lost track, we are now down to this:
|577cd5e894ec alpha: remove custom dec_and_lock() implementation
|831923bd9fd3 spinlock: atomic_dec_and_lock: Add an irqsave variant
|f452abc5a3c1 drivers/md/raid5: Use irqsave variant of atomic_dec_and_lock()
|8e609dcd8b0d drivers/md/raid5: Do not disable irq on release_inactive_stripe_list() call
|fdf84cf5bee4 bdi: use refcount_t for reference counting instead atomic_t
|c3fce80b4112 userns: use refcount_t for reference counting instead atomic_t
|a9595ca33f5b locking/refcount: implement refcount_dec_and_lock_irqsave()
|32ded25a41a9 bdi: Use irqsave variant of refcount_dec_and_lock()
|6a63e34ed625 userns: Use irqsave variant of refcount_dec_and_lock()

for reference at
  https://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/staging.git/log/?h=refcount_t_irqsave

Sebastian

Re: [PATCH 0.5/5] alpha: remove custom dec_and_lock() implementation

[Peter Zijlstra]

On Wed, Jun 06, 2018 at 01:18:50PM +0200, Sebastian Andrzej Siewior wrote:

> - atomic_add_unless() adds more memory barriers compared to the custom
>   assembly

> I can't tell if the different memory barriers
> are an issue but having the same barriers is probably good.

refcount decrement needs to be a RELEASE operation, such that all the
load/stores to the object happen before we decrement the refcount.

Otherwise things like:

	obj-foo = 5;
	refcnt_dec(&obj->ref);

can be re-ordered, which then allows fun scenarios like:

	CPU0				CPU1

	refcnt_dec(&obj->ref);
					if (dec_and_test(&obj->ref))
						free(obj);
	obj->foo = 5; // oops UaF


This means (for alpha) that there should be a memory barrier _before_
the decrement, however the dec_and_lock asm thing only has one _after_,
which, per the above, is too late.

The generic version using add_unless will result in memory barrier
before and after (because that is the rule for atomic ops with a return
value) which is strictly too many barriers for the refcount story, but
who knows what other ordering requirements code has.

[tip:locking/urgent] alpha: Remove custom dec_and_lock() implementation

[tip-bot for Sebastian Andrzej Siewior]

Commit-ID:  f2ae67941138a1e53cb1bc6a1b5878a8bdc74d26
Gitweb:     https://git.kernel.org/tip/f2ae67941138a1e53cb1bc6a1b5878a8bdc74d26
Author:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate: Tue, 12 Jun 2018 18:16:19 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 12 Jun 2018 23:33:24 +0200

alpha: Remove custom dec_and_lock() implementation

Alpha provides a custom implementation of dec_and_lock(). The functions
is split into two parts:
- atomic_add_unless() + return 0 (fast path in assembly)
- remaining part including locking (slow path in C)

Comparing the result of the alpha implementation with the generic
implementation compiled by gcc it looks like the fast path is optimized
by avoiding a stack frame (and reloading the GP), register store and all
this. This is only done in the slowpath.
After marking the slowpath (atomic_dec_and_lock_1()) as "noinline" and
doing the slowpath in C (the atomic_add_unless(atomic, -1, 1) part) I
noticed differences in the resulting assembly:
- the GP is still reloaded
- atomic_add_unless() adds more memory barriers compared to the custom
  assembly
- the custom assembly here does "load, sub, beq" while
  atomic_add_unless() does "load, cmpeq, add, bne". This is okay because
  it compares against zero after subtraction while the generic code
  compares against 1 before.

I'm not sure if avoiding the stack frame (and GP reloading) brings a lot
in terms of performance. Regarding the different barriers, Peter
Zijlstra says:

|refcount decrement needs to be a RELEASE operation, such that all the
|load/stores to the object happen before we decrement the refcount.
|
|Otherwise things like:
|
|        obj->foo = 5;
|        refcnt_dec(&obj->ref);
|
|can be re-ordered, which then allows fun scenarios like:
|
|        CPU0                            CPU1
|
|        refcnt_dec(&obj->ref);
|                                        if (dec_and_test(&obj->ref))
|                                                free(obj);
|        obj->foo = 5; // oops UaF
|
|
|This means (for alpha) that there should be a memory barrier _before_
|the decrement, however the dec_and_lock asm thing only has one _after_,
|which, per the above, is too late.
|
|The generic version using add_unless will result in memory barrier
|before and after (because that is the rule for atomic ops with a return
|value) which is strictly too many barriers for the refcount story, but
|who knows what other ordering requirements code has.

Remove the custom alpha implementation of dec_and_lock() and if it is an
issue (performance wise) then the fast path could still be inlined.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
Link: https://lkml.kernel.org/r/20180606115918.GG12198@hirez.programming.kicks-ass.net
Link: https://lkml.kernel.org/r20180612161621.22645-2-bigeasy@linutronix.de

---
 arch/alpha/Kconfig            |  5 -----
 arch/alpha/lib/Makefile       |  2 --
 arch/alpha/lib/dec_and_lock.c | 44 -------------------------------------------
 lib/Makefile                  |  6 +-----
 4 files changed, 1 insertion(+), 56 deletions(-)

diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
index 0c4805a572c8..04a4a138ed13 100644
--- a/arch/alpha/Kconfig
+++ b/arch/alpha/Kconfig
@@ -555,11 +555,6 @@ config SMP
 
 	  If you don't know what to do here, say N.
 
-config HAVE_DEC_LOCK
-	bool
-	depends on SMP
-	default y
-
 config NR_CPUS
 	int "Maximum number of CPUs (2-32)"
 	range 2 32
diff --git a/arch/alpha/lib/Makefile b/arch/alpha/lib/Makefile
index 04f9729de57c..854d5e79979e 100644
--- a/arch/alpha/lib/Makefile
+++ b/arch/alpha/lib/Makefile
@@ -35,8 +35,6 @@ lib-y =	__divqu.o __remqu.o __divlu.o __remlu.o \
 	callback_srm.o srm_puts.o srm_printk.o \
 	fls.o
 
-lib-$(CONFIG_SMP) += dec_and_lock.o
-
 # The division routines are built from single source, with different defines.
 AFLAGS___divqu.o = -DDIV
 AFLAGS___remqu.o =       -DREM
diff --git a/arch/alpha/lib/dec_and_lock.c b/arch/alpha/lib/dec_and_lock.c
deleted file mode 100644
index a117707f57fe..000000000000
--- a/arch/alpha/lib/dec_and_lock.c
+++ /dev/null
@@ -1,44 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * arch/alpha/lib/dec_and_lock.c
- *
- * ll/sc version of atomic_dec_and_lock()
- * 
- */
-
-#include <linux/spinlock.h>
-#include <linux/atomic.h>
-#include <linux/export.h>
-
-  asm (".text					\n\
-	.global _atomic_dec_and_lock		\n\
-	.ent _atomic_dec_and_lock		\n\
-	.align	4				\n\
-_atomic_dec_and_lock:				\n\
-	.prologue 0				\n\
-1:	ldl_l	$1, 0($16)			\n\
-	subl	$1, 1, $1			\n\
-	beq	$1, 2f				\n\
-	stl_c	$1, 0($16)			\n\
-	beq	$1, 4f				\n\
-	mb					\n\
-	clr	$0				\n\
-	ret					\n\
-2:	br	$29, 3f				\n\
-3:	ldgp	$29, 0($29)			\n\
-	br	$atomic_dec_and_lock_1..ng	\n\
-	.subsection 2				\n\
-4:	br	1b				\n\
-	.previous				\n\
-	.end _atomic_dec_and_lock");
-
-static int __used atomic_dec_and_lock_1(atomic_t *atomic, spinlock_t *lock)
-{
-	/* Slow path */
-	spin_lock(lock);
-	if (atomic_dec_and_test(atomic))
-		return 1;
-	spin_unlock(lock);
-	return 0;
-}
-EXPORT_SYMBOL(_atomic_dec_and_lock);
diff --git a/lib/Makefile b/lib/Makefile
index 84c6dcb31fbb..8b59f4a7c0e2 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -23,7 +23,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
 	 sha1.o chacha20.o irq_regs.o argv_split.o \
 	 flex_proportions.o ratelimit.o show_mem.o \
 	 is_single_threaded.o plist.o decompress.o kobject_uevent.o \
-	 earlycpio.o seq_buf.o siphash.o \
+	 earlycpio.o seq_buf.o siphash.o dec_and_lock.o \
 	 nmi_backtrace.o nodemask.o win_minmax.o
 
 lib-$(CONFIG_PRINTK) += dump_stack.o
@@ -98,10 +98,6 @@ obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
 obj-$(CONFIG_DEBUG_LIST) += list_debug.o
 obj-$(CONFIG_DEBUG_OBJECTS) += debugobjects.o
 
-ifneq ($(CONFIG_HAVE_DEC_LOCK),y)
-  lib-y += dec_and_lock.o
-endif
-
 obj-$(CONFIG_BITREVERSE) += bitrev.o
 obj-$(CONFIG_RATIONAL)	+= rational.o
 obj-$(CONFIG_CRC_CCITT)	+= crc-ccitt.o