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