From: Shawn Debnath <sdn@amazon.com>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: Introduce timeout capability for ConditionVariableSleep
Date: Tue, 12 Mar 2019 17:53:43 -0700
Message-ID: <20190313005342.GA8301@f01898859afd.ant.amazon.com> (raw)
In-Reply-To: <CA+hUKGJDt3yopu1K1xPW5NtbLq-0n6fbXxZcaOpqD0P0-CruyA@mail.gmail.com>
References: <eeb06007ccfe46e399df6af18bfcd15a@EX13D05UWC002.ant.amazon.com>
<CA+hUKGJDt3yopu1K1xPW5NtbLq-0n6fbXxZcaOpqD0P0-CruyA@mail.gmail.com>
Hi Thomas,
Thanks for reviewing!
On Wed, Mar 13, 2019 at 12:40:57PM +1300, Thomas Munro wrote:
> Can we just refer to the other function's documentation for this? I
> don't want two copies of this blurb (and this copy-paste already
> failed to include "Timed" in the example function name).
Hah - point well taken. Fixed.
> One difference compared to pthread_cond_timedwait() is that pthread
> uses an absolute time and here you use a relative time (as we do in
> WaitEventSet API). The first question is which makes a better API,
> and the second is what the semantics of a relative timeout should be:
> start again every time we get a spurious wake-up, or track time
> already waited? Let's see...
>
> - (void) WaitEventSetWait(cv_wait_event_set, -1, &event, 1,
> + ret = WaitEventSetWait(cv_wait_event_set, timeout, &event, 1,
> wait_event_info);
>
> Here you're restarting the timeout clock every time through the loop
> without adjustment, and I think that's not a good choice: spurious
> wake-ups cause bonus waiting.
Agree. In my testing WaitEventSetWait did the work for calculating the
right timeout remaining. It's a bummer that we have to repeat the same
pattern at the ConditionVariableTimedSleep() but I guess anyone who
loops in such cases will have to adjust their values accordingly.
BTW, I am curious why Andres in 98a64d0bd71 didn't just create an
artificial event with WL_TIMEOUT and return that from
WaitEventSetWait(). Would have made it cleaner than checking checking
return values for -1.
Updated v2 patch attached.
--
Shawn Debnath
Amazon Web Services (AWS)
From 878462c4e05217a6c13107811a676948bb7f2a65 Mon Sep 17 00:00:00 2001
From: Shawn Debnath <sdn@amazon.com>
Date: Tue, 12 Mar 2019 23:21:41 +0000
Subject: [PATCH] Introduce timeout capability for ConditionVariableSleep
This patch introduces ConditionVariableTimedSleep to enable timing out
of waiting for a condition variable to get signalled.
---
src/backend/storage/lmgr/condition_variable.c | 66 ++++++++++++++++++++++++++-
src/include/storage/condition_variable.h | 2 +
2 files changed, 66 insertions(+), 2 deletions(-)
diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.cindex 58b7b51472..3afb726131 100644--- a/src/backend/storage/lmgr/condition_variable.c+++ b/src/backend/storage/lmgr/condition_variable.c@@ -19,6 +19,7 @@
#include "postgres.h"
#include "miscadmin.h"
+#include "portability/instr_time.h"
#include "storage/condition_variable.h"
#include "storage/ipc.h"
#include "storage/proc.h"
@@ -121,9 +122,31 @@ ConditionVariablePrepareToSleep(ConditionVariable *cv)
*/
void
ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
+{+ (void) ConditionVariableTimedSleep(cv, -1 /* no timeout */, wait_event_info);+}++/*+ * Wait for the given condition variable to be signaled or till timeout.+ *+ * In the event of a timeout, we simply return and the caller+ * calls ConditionVariableCancelSleep to remove themselves from the+ * wait queue. See ConditionVariableSleep() for notes on how to correctly check+ * for the exit condition.+ *+ * Returns 0, or -1 if timed out.+ */+int+ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,+ uint32 wait_event_info)
{
WaitEvent event;
bool done = false;
+ int rc;+ int ret = 0;+ long rem_timeout = -1;+ instr_time start_time;+ instr_time cur_time;
/*
* If the caller didn't prepare to sleep explicitly, then do so now and
@@ -143,7 +166,18 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
if (cv_sleep_target != cv)
{
ConditionVariablePrepareToSleep(cv);
- return;+ return ret;+ }++ /*+ * Track the current time so that we can calculate the remaining timeout+ * if we are woken up spuriously.+ */+ if (timeout >= 0)+ {+ INSTR_TIME_SET_CURRENT(start_time);+ Assert(timeout >= 0 && timeout <= INT_MAX);+ rem_timeout = timeout;
}
do
@@ -154,12 +188,19 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
* Wait for latch to be set. (If we're awakened for some other
* reason, the code below will cope anyway.)
*/
- (void) WaitEventSetWait(cv_wait_event_set, -1, &event, 1,+ rc = WaitEventSetWait(cv_wait_event_set, rem_timeout, &event, 1,
wait_event_info);
/* Reset latch before examining the state of the wait list. */
ResetLatch(MyLatch);
+ /* Timed out */+ if (rc == 0 && timeout >= 0)+ {+ ret = -1; /* timeout */+ break;+ }+
/*
* If this process has been taken out of the wait list, then we know
* that it has been signaled by ConditionVariableSignal (or
@@ -179,10 +220,31 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
if (!proclist_contains(&cv->wakeup, MyProc->pgprocno, cvWaitLink))
{
done = true;
+ Assert(ret == 0);
proclist_push_tail(&cv->wakeup, MyProc->pgprocno, cvWaitLink);
}
SpinLockRelease(&cv->mutex);
++ /* If we're not done, update rem_timeout for next iteration */+ if (!done && timeout >= 0)+ {+ INSTR_TIME_SET_CURRENT(cur_time);+ INSTR_TIME_SUBTRACT(cur_time, start_time);+ rem_timeout = timeout - (long) INSTR_TIME_GET_MILLISEC(cur_time);++ /*+ * Check if we have reached the timeout threshold after calculating+ * the remaining timeout value.+ */+ if (rem_timeout <= 0)+ {+ ret = -1; /* timeout */+ break;+ }+ }
} while (!done);
++ return ret;
}
/*
diff --git a/src/include/storage/condition_variable.h b/src/include/storage/condition_variable.hindex 2a0249392c..f62164e483 100644--- a/src/include/storage/condition_variable.h+++ b/src/include/storage/condition_variable.h@@ -43,6 +43,8 @@ extern void ConditionVariableInit(ConditionVariable *cv);
* the condition variable.
*/
extern void ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info);
+extern int ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,+ uint32 wait_event_info);
extern void ConditionVariableCancelSleep(void);
/*
--
2.16.5
Attachments:
[text/plain] 0001-Introduce-timeout-capability-for-ConditionVariableSleep-v2.patch (4.4K, ../20190313005342.GA8301@f01898859afd.ant.amazon.com/2-0001-Introduce-timeout-capability-for-ConditionVariableSleep-v2.patch)
download | inline diff:
From 878462c4e05217a6c13107811a676948bb7f2a65 Mon Sep 17 00:00:00 2001
From: Shawn Debnath <sdn@amazon.com>
Date: Tue, 12 Mar 2019 23:21:41 +0000
Subject: [PATCH] Introduce timeout capability for ConditionVariableSleep
This patch introduces ConditionVariableTimedSleep to enable timing out
of waiting for a condition variable to get signalled.
---
src/backend/storage/lmgr/condition_variable.c | 66 ++++++++++++++++++++++++++-
src/include/storage/condition_variable.h | 2 +
2 files changed, 66 insertions(+), 2 deletions(-)
diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.cindex 58b7b51472..3afb726131 100644--- a/src/backend/storage/lmgr/condition_variable.c+++ b/src/backend/storage/lmgr/condition_variable.c@@ -19,6 +19,7 @@
#include "postgres.h"
#include "miscadmin.h"
+#include "portability/instr_time.h"
#include "storage/condition_variable.h"
#include "storage/ipc.h"
#include "storage/proc.h"
@@ -121,9 +122,31 @@ ConditionVariablePrepareToSleep(ConditionVariable *cv)
*/
void
ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
+{+ (void) ConditionVariableTimedSleep(cv, -1 /* no timeout */, wait_event_info);+}++/*+ * Wait for the given condition variable to be signaled or till timeout.+ *+ * In the event of a timeout, we simply return and the caller+ * calls ConditionVariableCancelSleep to remove themselves from the+ * wait queue. See ConditionVariableSleep() for notes on how to correctly check+ * for the exit condition.+ *+ * Returns 0, or -1 if timed out.+ */+int+ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,+ uint32 wait_event_info)
{
WaitEvent event;
bool done = false;
+ int rc;+ int ret = 0;+ long rem_timeout = -1;+ instr_time start_time;+ instr_time cur_time;
/*
* If the caller didn't prepare to sleep explicitly, then do so now and
@@ -143,7 +166,18 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
if (cv_sleep_target != cv)
{
ConditionVariablePrepareToSleep(cv);
- return;+ return ret;+ }++ /*+ * Track the current time so that we can calculate the remaining timeout+ * if we are woken up spuriously.+ */+ if (timeout >= 0)+ {+ INSTR_TIME_SET_CURRENT(start_time);+ Assert(timeout >= 0 && timeout <= INT_MAX);+ rem_timeout = timeout;
}
do
@@ -154,12 +188,19 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
* Wait for latch to be set. (If we're awakened for some other
* reason, the code below will cope anyway.)
*/
- (void) WaitEventSetWait(cv_wait_event_set, -1, &event, 1,+ rc = WaitEventSetWait(cv_wait_event_set, rem_timeout, &event, 1,
wait_event_info);
/* Reset latch before examining the state of the wait list. */
ResetLatch(MyLatch);
+ /* Timed out */+ if (rc == 0 && timeout >= 0)+ {+ ret = -1; /* timeout */+ break;+ }+
/*
* If this process has been taken out of the wait list, then we know
* that it has been signaled by ConditionVariableSignal (or
@@ -179,10 +220,31 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
if (!proclist_contains(&cv->wakeup, MyProc->pgprocno, cvWaitLink))
{
done = true;
+ Assert(ret == 0);
proclist_push_tail(&cv->wakeup, MyProc->pgprocno, cvWaitLink);
}
SpinLockRelease(&cv->mutex);
++ /* If we're not done, update rem_timeout for next iteration */+ if (!done && timeout >= 0)+ {+ INSTR_TIME_SET_CURRENT(cur_time);+ INSTR_TIME_SUBTRACT(cur_time, start_time);+ rem_timeout = timeout - (long) INSTR_TIME_GET_MILLISEC(cur_time);++ /*+ * Check if we have reached the timeout threshold after calculating+ * the remaining timeout value.+ */+ if (rem_timeout <= 0)+ {+ ret = -1; /* timeout */+ break;+ }+ }
} while (!done);
++ return ret;
}
/*
diff --git a/src/include/storage/condition_variable.h b/src/include/storage/condition_variable.hindex 2a0249392c..f62164e483 100644--- a/src/include/storage/condition_variable.h+++ b/src/include/storage/condition_variable.h@@ -43,6 +43,8 @@ extern void ConditionVariableInit(ConditionVariable *cv);
* the condition variable.
*/
extern void ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info);
+extern int ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,+ uint32 wait_event_info);
extern void ConditionVariableCancelSleep(void);
/*
--
2.16.5
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sdn@amazon.com, thomas.munro@gmail.com
Subject: Re: Introduce timeout capability for ConditionVariableSleep
In-Reply-To: <20190313005342.GA8301@f01898859afd.ant.amazon.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox