From: Waiman Long <longman@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>, mingo@redhat.com, will@kernel.org
Cc: linux-kernel@vger.kernel.org, boqun.feng@gmail.com
Subject: Re: [PATCH 5/6] locking/rwsem: Unify wait loop
Date: Thu, 23 Feb 2023 17:45:56 -0500 [thread overview]
Message-ID: <0be586e4-8c08-63ff-a78d-ffb5ae37adc3@redhat.com> (raw)
In-Reply-To: <20230223123319.608133045@infradead.org>
[-- Attachment #1: Type: text/plain, Size: 5211 bytes --]
On 2/23/23 07:26, Peter Zijlstra wrote:
> Now that the reader and writer wait loops are identical, share the
> code.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> kernel/locking/rwsem.c | 117 +++++++++++++++++++------------------------------
> 1 file changed, 47 insertions(+), 70 deletions(-)
>
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -650,13 +650,11 @@ static void rwsem_mark_wake(struct rw_se
> * optionally wake up waiters before it returns.
> */
> static inline void
> -rwsem_del_wake_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter,
> - struct wake_q_head *wake_q)
> +rwsem_del_wake_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
> __releases(&sem->wait_lock)
> {
> bool first = rwsem_first_waiter(sem) == waiter;
> -
> - wake_q_init(wake_q);
> + DEFINE_WAKE_Q(wake_q);
>
> /*
> * If the wait_list isn't empty and the waiter to be deleted is
> @@ -664,10 +662,10 @@ rwsem_del_wake_waiter(struct rw_semaphor
> * be eligible to acquire or spin on the lock.
> */
> if (rwsem_del_waiter(sem, waiter) && first)
> - rwsem_mark_wake(sem, RWSEM_WAKE_ANY, wake_q);
> + rwsem_mark_wake(sem, RWSEM_WAKE_ANY, &wake_q);
> raw_spin_unlock_irq(&sem->wait_lock);
> - if (!wake_q_empty(wake_q))
> - wake_up_q(wake_q);
> + if (!wake_q_empty(&wake_q))
> + wake_up_q(&wake_q);
> }
>
> /*
> @@ -993,6 +991,46 @@ static inline void rwsem_cond_wake_waite
> rwsem_mark_wake(sem, wake_type, wake_q);
> }
>
> +#define waiter_type(_waiter, _r, _w) \
> + ((_waiter)->type == RWSEM_WAITING_FOR_READ ? (_r) : (_w))
> +
> +static __always_inline struct rw_semaphore *
> +rwsem_waiter_wait(struct rw_semaphore *sem, struct rwsem_waiter *waiter, int state)
> +{
> + trace_contention_begin(sem, waiter_type(waiter, LCB_F_READ, LCB_F_WRITE));
> +
> + /* wait to be given the lock */
> + for (;;) {
> + set_current_state(state);
> + if (!smp_load_acquire(&waiter->task)) {
> + /* Matches rwsem_waiter_wake()'s smp_store_release(). */
> + break;
> + }
> + if (signal_pending_state(state, current)) {
> + raw_spin_lock_irq(&sem->wait_lock);
> + if (waiter->task)
> + goto out_nolock;
> + raw_spin_unlock_irq(&sem->wait_lock);
> + /* Ordered by sem->wait_lock against rwsem_mark_wake(). */
> + break;
> + }
> + schedule_preempt_disabled();
> + lockevent_inc(waiter_type(waiter, rwsem_sleep_reader, rwsem_sleep_writer));
> + }
> +
> + __set_current_state(TASK_RUNNING);
> + lockevent_inc(waiter_type(waiter, rwsem_rlock, rwsem_wlock));
> + trace_contention_end(sem, 0);
> + return sem;
> +
> +out_nolock:
> + rwsem_del_wake_waiter(sem, waiter);
> + __set_current_state(TASK_RUNNING);
Similar to boqun's comment, we should move __set_current_state() before
rwsem_del_wake_waiter().
Unfortunately, lockevent_inc() doesn't work with waiter_type() like that
as the compilation will fail if CONFIG_LOCK_EVENT_COUNTS is enabled.
Could you include the attached patch in your series and make the
following changes?
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index deb0d016a6ce..5b14b0d076fd 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -1021,13 +1021,14 @@ static inline void rwsem_cond_wake_waiter(struct
rw_semaphore *sem, long count,
rwsem_mark_wake(sem, wake_type, wake_q);
}
-#define waiter_type(_waiter, _r, _w) \
- ((_waiter)->type == RWSEM_WAITING_FOR_READ ? (_r) : (_w))
+#define waiter_type(_reader, _r, _w) ((_reader) ? (_r) : (_w))
static __always_inline struct rw_semaphore *
rwsem_waiter_wait(struct rw_semaphore *sem, struct rwsem_waiter
*waiter, int state)
{
- trace_contention_begin(sem, waiter_type(waiter, LCB_F_READ,
LCB_F_WRITE));
+ bool reader = waiter->type == RWSEM_WAITING_FOR_READ;
+
+ trace_contention_begin(sem, waiter_type(reader, LCB_F_READ,
LCB_F_WRITE));
/* wait to be given the lock */
for (;;) {
@@ -1045,18 +1046,18 @@ rwsem_waiter_wait(struct rw_semaphore *sem,
struct rwsem_waiter *waiter, int sta
break;
}
schedule_preempt_disabled();
- lockevent_inc(waiter_type(waiter, rwsem_sleep_reader,
rwsem_sleep_writer));
+ lockevent_cond_inc2(reader, rwsem_sleep_reader,
rwsem_sleep_writer);
}
__set_current_state(TASK_RUNNING);
- lockevent_inc(waiter_type(waiter, rwsem_rlock, rwsem_wlock));
+ lockevent_cond_inc2(reader, rwsem_rlock, rwsem_wlock);
trace_contention_end(sem, 0);
return sem;
out_nolock:
rwsem_del_wake_waiter(sem, waiter);
__set_current_state(TASK_RUNNING);
- lockevent_inc(waiter_type(waiter, rwsem_rlock_fail,
rwsem_wlock_fail));
+ lockevent_cond_inc2(reader, rwsem_rlock_fail, rwsem_wlock_fail);
trace_contention_end(sem, -EINTR);
return ERR_PTR(-EINTR);
}
Thanks,
Longman
lockevent_inc
[-- Attachment #2: 0001-locking-lock_events-Add-a-new-lockevent_cond_inc2-he.patch --]
[-- Type: text/x-patch, Size: 1724 bytes --]
From 490fb153006941ec7b576c9e89cb220a0739a95c Mon Sep 17 00:00:00 2001
From: Waiman Long <longman@redhat.com>
Date: Thu, 23 Feb 2023 17:32:06 -0500
Subject: [PATCH] locking/lock_events: Add a new lockevent_cond_inc2() helper
Add a new lockevent_cond_inc2(cond, true_event, false_event) helper
to conditionally increment one of the 2 given events.
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/lock_events.h | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/kernel/locking/lock_events.h b/kernel/locking/lock_events.h
index 8c7e7d25f09c..668c2f1397f6 100644
--- a/kernel/locking/lock_events.h
+++ b/kernel/locking/lock_events.h
@@ -43,6 +43,21 @@ static inline void __lockevent_inc(enum lock_events event, bool cond)
#define lockevent_inc(ev) __lockevent_inc(LOCKEVENT_ ##ev, true)
#define lockevent_cond_inc(ev, c) __lockevent_inc(LOCKEVENT_ ##ev, c)
+/*
+ * Increment either the tevent (cond true) or fevent (cond false)
+ */
+static inline void __lockevent_cond_inc2(bool cond, enum lock_events tevent,
+ enum lock_events fevent)
+{
+ if (cond)
+ raw_cpu_inc(lockevents[tevent]);
+ else
+ raw_cpu_inc(lockevents[fevent]);
+
+}
+#define lockevent_cond_inc2(c, tev, fev) \
+ __lockevent_cond_inc2(c, LOCKEVENT_ ##tev, LOCKEVENT_ ##fev)
+
static inline void __lockevent_add(enum lock_events event, int inc)
{
raw_cpu_add(lockevents[event], inc);
@@ -55,6 +70,7 @@ static inline void __lockevent_add(enum lock_events event, int inc)
#define lockevent_inc(ev)
#define lockevent_add(ev, c)
#define lockevent_cond_inc(ev, c)
+#define lockevent_cond_inc2(c, tev, fev)
#endif /* CONFIG_LOCK_EVENT_COUNTS */
#endif /* __LOCKING_LOCK_EVENTS_H */
--
2.31.1
next prev parent reply other threads:[~2023-02-23 22:47 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-23 12:26 [PATCH 0/6] locking/rwsem: Rework writer wakeup and handoff Peter Zijlstra
2023-02-23 12:26 ` [PATCH 1/6] locking/rwsem: Minor code refactoring in rwsem_mark_wake() Peter Zijlstra
2023-02-23 12:26 ` [PATCH 2/6] locking/rwsem: Enforce queueing when HANDOFF Peter Zijlstra
2023-02-23 12:26 ` [PATCH 3/6] locking/rwsem: Rework writer wakeup Peter Zijlstra
2023-02-23 21:38 ` Waiman Long
2023-02-26 11:58 ` Peter Zijlstra
2023-02-26 12:00 ` Peter Zijlstra
2023-02-26 21:31 ` Waiman Long
2023-02-26 11:59 ` Peter Zijlstra
2023-02-26 15:04 ` Peter Zijlstra
2023-02-26 16:51 ` Peter Zijlstra
2023-02-27 0:22 ` Waiman Long
2023-02-27 10:31 ` Peter Zijlstra
2023-02-27 20:16 ` Waiman Long
2023-03-20 8:12 ` Peter Zijlstra
2023-03-20 17:36 ` Waiman Long
2023-02-23 12:26 ` [PATCH 4/6] locking/rwsem: Split out rwsem_reader_wake() Peter Zijlstra
2023-02-23 12:26 ` [PATCH 5/6] locking/rwsem: Unify wait loop Peter Zijlstra
2023-02-23 19:31 ` Boqun Feng
2023-02-24 1:33 ` Boqun Feng
2023-02-26 12:01 ` Peter Zijlstra
2023-02-26 18:22 ` Boqun Feng
2023-02-23 22:45 ` Waiman Long [this message]
2023-02-26 16:15 ` Peter Zijlstra
2023-02-23 12:26 ` [PATCH 6/6] locking/rwsem: Use the force Peter Zijlstra
2023-02-24 1:19 ` [PATCH 0/6] locking/rwsem: Rework writer wakeup and handoff Waiman Long
2023-02-24 11:55 ` Jiri Wiesner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0be586e4-8c08-63ff-a78d-ffb5ae37adc3@redhat.com \
--to=longman@redhat.com \
--cc=boqun.feng@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).