Skip to content

Commit

Permalink
atomic/tty: Fix up atomic abuse in ldsem
Browse files Browse the repository at this point in the history 
Mark found ldsem_cmpxchg() needed an (atomic_long_t *) cast to keep
working after making the atomic_long interface type safe.

Needing casts is bad form, which made me look at the code. There are no
ld_semaphore::count users outside of these functions so there is no
reason why it can not be an atomic_long_t in the first place, obviating
the need for this cast.

That also ensures the loads use atomic_long_read(), which implies (at
least) READ_ONCE() in order to guarantee single-copy-atomic loads.

When using atomic_long_try_cmpxchg() the ldsem_cmpxchg() wrapper gets
very thin (the only difference is not changing *old on success, which
most callers don't seem to care about).

So rework the whole thing to use atomic_long_t and its accessors
directly.

While there, fixup all the horrible comment styles.

Cc: Peter Hurley <peter@hurleysoftware.com>
Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
Peter Zijlstra authored and Greg Kroah-Hartman committed Jun 28, 2018
1 parent 9d93989 commit 5fd691a
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 49 deletions.
82 changes: 35 additions & 47 deletions drivers/tty/tty_ldsem.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,28 +74,6 @@ struct ldsem_waiter {
struct task_struct *task;
};

static inline long ldsem_atomic_update(long delta, struct ld_semaphore *sem)
{
return atomic_long_add_return(delta, (atomic_long_t *)&sem->count);
}

/*
* ldsem_cmpxchg() updates @*old with the last-known sem->count value.
* Returns 1 if count was successfully changed; @*old will have @new value.
* Returns 0 if count was not changed; @*old will have most recent sem->count
*/
static inline int ldsem_cmpxchg(long *old, long new, struct ld_semaphore *sem)
{
long tmp = atomic_long_cmpxchg(&sem->count, *old, new);
if (tmp == *old) {
*old = new;
return 1;
} else {
*old = tmp;
return 0;
}
}

/*
* Initialize an ldsem:
*/
Expand All @@ -109,7 +87,7 @@ void __init_ldsem(struct ld_semaphore *sem, const char *name,
debug_check_no_locks_freed((void *)sem, sizeof(*sem));
lockdep_init_map(&sem->dep_map, name, key, 0);
#endif
sem->count = LDSEM_UNLOCKED;
atomic_long_set(&sem->count, LDSEM_UNLOCKED);
sem->wait_readers = 0;
raw_spin_lock_init(&sem->wait_lock);
INIT_LIST_HEAD(&sem->read_wait);
Expand All @@ -122,16 +100,17 @@ static void __ldsem_wake_readers(struct ld_semaphore *sem)
struct task_struct *tsk;
long adjust, count;

/* Try to grant read locks to all readers on the read wait list.
/*
* Try to grant read locks to all readers on the read wait list.
* Note the 'active part' of the count is incremented by
* the number of readers before waking any processes up.
*/
adjust = sem->wait_readers * (LDSEM_ACTIVE_BIAS - LDSEM_WAIT_BIAS);
count = ldsem_atomic_update(adjust, sem);
count = atomic_long_add_return(adjust, &sem->count);
do {
if (count > 0)
break;
if (ldsem_cmpxchg(&count, count - adjust, sem))
if (atomic_long_try_cmpxchg(&sem->count, &count, count - adjust))
return;
} while (1);

Expand All @@ -148,14 +127,15 @@ static void __ldsem_wake_readers(struct ld_semaphore *sem)

static inline int writer_trylock(struct ld_semaphore *sem)
{
/* only wake this writer if the active part of the count can be
/*
* Only wake this writer if the active part of the count can be
* transitioned from 0 -> 1
*/
long count = ldsem_atomic_update(LDSEM_ACTIVE_BIAS, sem);
long count = atomic_long_add_return(LDSEM_ACTIVE_BIAS, &sem->count);
do {
if ((count & LDSEM_ACTIVE_MASK) == LDSEM_ACTIVE_BIAS)
return 1;
if (ldsem_cmpxchg(&count, count - LDSEM_ACTIVE_BIAS, sem))
if (atomic_long_try_cmpxchg(&sem->count, &count, count - LDSEM_ACTIVE_BIAS))
return 0;
} while (1);
}
Expand Down Expand Up @@ -205,12 +185,16 @@ down_read_failed(struct ld_semaphore *sem, long count, long timeout)
/* set up my own style of waitqueue */
raw_spin_lock_irq(&sem->wait_lock);

/* Try to reverse the lock attempt but if the count has changed
/*
* Try to reverse the lock attempt but if the count has changed
* so that reversing fails, check if there are are no waiters,
* and early-out if not */
* and early-out if not
*/
do {
if (ldsem_cmpxchg(&count, count + adjust, sem))
if (atomic_long_try_cmpxchg(&sem->count, &count, count + adjust)) {
count += adjust;
break;
}
if (count > 0) {
raw_spin_unlock_irq(&sem->wait_lock);
return sem;
Expand Down Expand Up @@ -243,12 +227,14 @@ down_read_failed(struct ld_semaphore *sem, long count, long timeout)
__set_current_state(TASK_RUNNING);

if (!timeout) {
/* lock timed out but check if this task was just
/*
* Lock timed out but check if this task was just
* granted lock ownership - if so, pretend there
* was no timeout; otherwise, cleanup lock wait */
* was no timeout; otherwise, cleanup lock wait.
*/
raw_spin_lock_irq(&sem->wait_lock);
if (waiter.task) {
ldsem_atomic_update(-LDSEM_WAIT_BIAS, sem);
atomic_long_add_return(-LDSEM_WAIT_BIAS, &sem->count);
list_del(&waiter.list);
raw_spin_unlock_irq(&sem->wait_lock);
put_task_struct(waiter.task);
Expand All @@ -273,11 +259,13 @@ down_write_failed(struct ld_semaphore *sem, long count, long timeout)
/* set up my own style of waitqueue */
raw_spin_lock_irq(&sem->wait_lock);

/* Try to reverse the lock attempt but if the count has changed
/*
* Try to reverse the lock attempt but if the count has changed
* so that reversing fails, check if the lock is now owned,
* and early-out if so */
* and early-out if so.
*/
do {
if (ldsem_cmpxchg(&count, count + adjust, sem))
if (atomic_long_try_cmpxchg(&sem->count, &count, count + adjust))
break;
if ((count & LDSEM_ACTIVE_MASK) == LDSEM_ACTIVE_BIAS) {
raw_spin_unlock_irq(&sem->wait_lock);
Expand All @@ -303,7 +291,7 @@ down_write_failed(struct ld_semaphore *sem, long count, long timeout)
}

if (!locked)
ldsem_atomic_update(-LDSEM_WAIT_BIAS, sem);
atomic_long_add_return(-LDSEM_WAIT_BIAS, &sem->count);
list_del(&waiter.list);
raw_spin_unlock_irq(&sem->wait_lock);

Expand All @@ -324,7 +312,7 @@ static int __ldsem_down_read_nested(struct ld_semaphore *sem,

lockdep_acquire_read(sem, subclass, 0, _RET_IP_);

count = ldsem_atomic_update(LDSEM_READ_BIAS, sem);
count = atomic_long_add_return(LDSEM_READ_BIAS, &sem->count);
if (count <= 0) {
lock_stat(sem, contended);
if (!down_read_failed(sem, count, timeout)) {
Expand All @@ -343,7 +331,7 @@ static int __ldsem_down_write_nested(struct ld_semaphore *sem,

lockdep_acquire(sem, subclass, 0, _RET_IP_);

count = ldsem_atomic_update(LDSEM_WRITE_BIAS, sem);
count = atomic_long_add_return(LDSEM_WRITE_BIAS, &sem->count);
if ((count & LDSEM_ACTIVE_MASK) != LDSEM_ACTIVE_BIAS) {
lock_stat(sem, contended);
if (!down_write_failed(sem, count, timeout)) {
Expand All @@ -370,10 +358,10 @@ int __sched ldsem_down_read(struct ld_semaphore *sem, long timeout)
*/
int ldsem_down_read_trylock(struct ld_semaphore *sem)
{
long count = sem->count;
long count = atomic_long_read(&sem->count);

while (count >= 0) {
if (ldsem_cmpxchg(&count, count + LDSEM_READ_BIAS, sem)) {
if (atomic_long_try_cmpxchg(&sem->count, &count, count + LDSEM_READ_BIAS)) {
lockdep_acquire_read(sem, 0, 1, _RET_IP_);
lock_stat(sem, acquired);
return 1;
Expand All @@ -396,10 +384,10 @@ int __sched ldsem_down_write(struct ld_semaphore *sem, long timeout)
*/
int ldsem_down_write_trylock(struct ld_semaphore *sem)
{
long count = sem->count;
long count = atomic_long_read(&sem->count);

while ((count & LDSEM_ACTIVE_MASK) == 0) {
if (ldsem_cmpxchg(&count, count + LDSEM_WRITE_BIAS, sem)) {
if (atomic_long_try_cmpxchg(&sem->count, &count, count + LDSEM_WRITE_BIAS)) {
lockdep_acquire(sem, 0, 1, _RET_IP_);
lock_stat(sem, acquired);
return 1;
Expand All @@ -417,7 +405,7 @@ void ldsem_up_read(struct ld_semaphore *sem)

lockdep_release(sem, 1, _RET_IP_);

count = ldsem_atomic_update(-LDSEM_READ_BIAS, sem);
count = atomic_long_add_return(-LDSEM_READ_BIAS, &sem->count);
if (count < 0 && (count & LDSEM_ACTIVE_MASK) == 0)
ldsem_wake(sem);
}
Expand All @@ -431,7 +419,7 @@ void ldsem_up_write(struct ld_semaphore *sem)

lockdep_release(sem, 1, _RET_IP_);

count = ldsem_atomic_update(-LDSEM_WRITE_BIAS, sem);
count = atomic_long_add_return(-LDSEM_WRITE_BIAS, &sem->count);
if (count < 0)
ldsem_wake(sem);
}
Expand Down
4 changes: 2 additions & 2 deletions include/linux/tty_ldisc.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,13 @@

#include <linux/fs.h>
#include <linux/wait.h>

#include <linux/atomic.h>

/*
* the semaphore definition
*/
struct ld_semaphore {
long count;
atomic_long_t count;
raw_spinlock_t wait_lock;
unsigned int wait_readers;
struct list_head read_wait;
Expand Down

0 comments on commit 5fd691a

Please sign in to comment.