| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1364489492.3559.18.camel@thor.lan>
Date: Thu, 28 Mar 2013 12:51:32 -0400
From: Peter Hurley <peter@...leysoftware.com>
To: Michel Lespinasse <walken@...gle.com>
Cc: Alex Shi <alex.shi@...el.com>, Ingo Molnar <mingo@...nel.org>,
David Howells <dhowells@...hat.com>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Thomas Gleixner <tglx@...utronix.de>,
Yuanhan Liu <yuanhan.liu@...ux.intel.com>,
Rik van Riel <riel@...hat.com>,
Dave Chinner <david@...morbit.com>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 10/13] rwsem: simplify __rwsem_do_wake
On Fri, 2013-03-15 at 03:54 -0700, Michel Lespinasse wrote:
> This is mostly for cleanup value:
>
> - We don't need several gotos to handle the case where the first
> waiter is a writer. Two simple tests will do (and generate very
> similar code).
>
> - In the remainder of the function, we know the first waiter is a reader,
> so we don't have to double check that. We can use do..while loops
> to iterate over the readers to wake (generates slightly better code).
>
> Signed-off-by: Michel Lespinasse <walken@...gle.com>
>
> ---
> lib/rwsem-spinlock.c | 25 ++++++++-----------------
> lib/rwsem.c | 26 ++++++++++++--------------
> 2 files changed, 20 insertions(+), 31 deletions(-)
>
> diff --git a/lib/rwsem-spinlock.c b/lib/rwsem-spinlock.c
> index 5f117f37ac0a..20207a6e8ac3 100644
> --- a/lib/rwsem-spinlock.c
> +++ b/lib/rwsem-spinlock.c
> @@ -70,26 +70,17 @@ __rwsem_do_wake(struct rw_semaphore *sem, int wakewrite)
>
> waiter = list_entry(sem->wait_list.next, struct rwsem_waiter, list);
>
> - if (!wakewrite) {
> - if (waiter->type == RWSEM_WAITING_FOR_WRITE)
> - goto out;
> - goto dont_wake_writers;
> - }
> -
> - /*
> - * as we support write lock stealing, we can't set sem->activity
> - * to -1 here to indicate we get the lock. Instead, we wake it up
> - * to let it go get it again.
> - */
> if (waiter->type == RWSEM_WAITING_FOR_WRITE) {
> - wake_up_process(waiter->task);
> + if (wakewrite)
> + /* Wake up a writer. Note that we do not grant it the
> + * lock - it will have to acquire it when it runs. */
> + wake_up_process(waiter->task);
> goto out;
> }
>
> - /* grant an infinite number of read locks to the front of the queue */
> - dont_wake_writers:
> + /* grant read locks to all queued readers. */
I think the original comment still applies here now?
> woken = 0;
> - while (waiter->type == RWSEM_WAITING_FOR_READ) {
> + do {
> struct list_head *next = waiter->list.next;
>
> list_del(&waiter->list);
> @@ -99,10 +90,10 @@ __rwsem_do_wake(struct rw_semaphore *sem, int wakewrite)
> wake_up_process(tsk);
> put_task_struct(tsk);
> woken++;
> - if (list_empty(&sem->wait_list))
> + if (next == &sem->wait_list)
> break;
> waiter = list_entry(next, struct rwsem_waiter, list);
> - }
> + } while (waiter->type != RWSEM_WAITING_FOR_WRITE);
Maybe convert this to list_for_each_entry_safe() since you're cleaning
it up?
>
> sem->activity += woken;
>
> diff --git a/lib/rwsem.c b/lib/rwsem.c
> index 0d50e46d5b0c..9a675fa9d78e 100644
> --- a/lib/rwsem.c
> +++ b/lib/rwsem.c
> @@ -68,20 +68,17 @@ __rwsem_do_wake(struct rw_semaphore *sem, int wake_type)
> signed long woken, loop, adjustment;
>
> waiter = list_entry(sem->wait_list.next, struct rwsem_waiter, list);
> - if (waiter->type != RWSEM_WAITING_FOR_WRITE)
> - goto readers_only;
> -
> - if (wake_type == RWSEM_WAKE_READ_OWNED)
> - /* Another active reader was observed, so wakeup is not
> - * likely to succeed. Save the atomic op.
> - */
> + if (waiter->type == RWSEM_WAITING_FOR_WRITE) {
> + if (wake_type != RWSEM_WAKE_READ_OWNED)
> + /* Wake writer at the front of the queue, but do not
> + * grant it the lock yet as we want other writers
> + * to be able to steal it. Readers, on the other hand,
> + * will block as they will notice the queued writer.
> + */
> + wake_up_process(waiter->task);
> goto out;
> + }
>
> - /* Wake up the writing waiter and let the task grab the sem: */
> - wake_up_process(waiter->task);
> - goto out;
> -
> - readers_only:
> /* If we come here from up_xxxx(), another thread might have reached
> * rwsem_down_failed_common() before we acquired the spinlock and
> * woken up a waiter, making it now active. We prefer to check for
> @@ -125,7 +122,8 @@ __rwsem_do_wake(struct rw_semaphore *sem, int wake_type)
> rwsem_atomic_add(adjustment, sem);
>
> next = sem->wait_list.next;
> - for (loop = woken; loop > 0; loop--) {
> + loop = woken;
> + do {
> waiter = list_entry(next, struct rwsem_waiter, list);
> next = waiter->list.next;
> tsk = waiter->task;
> @@ -133,7 +131,7 @@ __rwsem_do_wake(struct rw_semaphore *sem, int wake_type)
> waiter->task = NULL;
> wake_up_process(tsk);
> put_task_struct(tsk);
> - }
> + } while (--loop);
>
> sem->wait_list.next = next;
> next->prev = &sem->wait_list;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists