On Wed, May 05, 2021 at 11:05:06AM +1000, David Gwynne wrote:
> On Tue, May 04, 2021 at 11:54:31AM -0500, Scott Cheloha wrote:
> >
> > [...]
> >
> > Here is where I get confused.
> >
> > Why do I want to wait for *all* timeouts in the queue to finish
> > running?
>
> You don't, you just want to know that whatever timeout was running
> has finished cos you don't know if that currently running timeout is
> yours or not.
>
> > My understanding of the timeout_del_barrier(9) use case was as
> > follows:
> >
> > 1. I have a dynamically allocated timeout struct. The timeout is
> > scheduled to execute at some point in the future.
> >
> > 2. I want to free the memory allocated to for the timeout.
> >
> > 3. To safely free the memory I need to ensure the timeout
> > is not executing. Until the timeout function, i.e.
> > to->to_func(), has returned it isn't necessarily safe to
> > free that memory.
> >
> > 4. If I call timeout_del_barrier(9), it will only return if the
> > timeout in question is not running. Assuming the timeout itself
> > is not a periodic timeout that reschedules itself we can then
> > safely free the memory.
>
> Barriers aren't about references to timeouts, they're about references
> to the work associated with a timeout.
>
> If you only cared about the timeout struct itself, then you can get
> all the information about whether it's referenced by the timeout
> queues from the return values from timeout_add and timeout_del.
> timeout_add() returns 1 if the timeout subsystem takes the reference
> to the timeout. If the timeout is already scheduled it returns 0
> because it's already got a reference. timeout_del returns 1 if the
> timeout itself was scheduled and it removed it, therefore giving
> up it's reference. If you timeout_del and it returns 0, then it
> wasn't on a queue inside timeouts. Easy.
>
> What timeout_add and timeout_del don't tell you is whether the work
> referenced by the timeout is currently running. The timeout runners
> copy the function and argument onto the stack when they dequeue a
> timeout to run, and give up the reference to the timeout when the
> mutex around the timeout wheels/cirqs is released. However, the argument
> is still referenced on the stack and the function to process it may be
> about to run or is running. You can't tell that from the timeout_add/del
> return values though.
>
> We provide two ways to deal with that. One is you have reference
> counters (or similar) on the thing you're deferring to a timeout.
> The other is you use a barrier so you know the work you deferred
> isn't on the timeout runners stack anymore because it's moved on
> to run the barrier work.
>
> This is consistent with tasks and interrupts.
>
> > Given this understanding, my approach was to focus on the timeout in
> > question. So my code just spins until it the timeout is no longer
> > running.
> >
> > But now I think I am mistaken.
> >
> > IIUC you're telling me (and showing me, in code) that the goal of
> > timeout_barrier() is to wait for the *whole* softclock() to return,
> > not just the one timeout.
>
> No, just the currently running timeout.
>
> Putting the barrier timeout on the end of the proc and todo cirqs
> is because there's no CIRCQ_INSERT_HEAD I can use right now.
>
> > Why do I want to wait for the whole softclock() or softclock_thread()?
> > Why not just wait for the one timeout to finish?
>
> Cos I was too lazy to write CIRCQ_INSERT_HEAD last night :D
Okay. After thinking this over I'm pretty sure we are skinning the
same cat in two different ways.
Your cond_wait(9) approach is fine for both timeout types because
timeout_del_barrier(9) is only safe to call from process context.
When I wrote my first diff I was under the impression it was safe to
call timeout_del_barrier(9) from interrupt context. But I reread the
manpage and now I see that this is not the case, my bad.
> [...]
>
> This discussion has some relevance to taskqs too. I was also lazy when I
> implemented taskq_barrier and used task_add to put the barrier onto the
> list of work, which meant it got added to the end. Now DRM relies on
> this behaviour. Maybe I should have pushed to commit the diff below.
>
> This diff lets taskq barriers run as soon as possible, but adds compat
> to the drm workqueue stuff so they can wait for all pending work to
> finish as they expect.
>
> P.S. don't call timeout_del from inside a timeout handler. I still think
> timeout_set_proc was a mistake.
>
> [...]
Let's pick this whole train of thought (taskq, etc.) up in a
different thread. One thing at a time.
--
Updated patch:
- Keep timeout_barrier(9).
- In timeout_barrier(9) use the barrier timeout for both normal and
process-context timeouts. All callers use cond_wait(9) now.
- Remove the kernel lock from the non-process path. I assume I don't
need to splx(splschedclock()) anymore?
- Rename "timeout_proc_barrier()" to "timeout_barrier_timeout()"
because now we use it for both timeout types.
- Update timeout.9: timeout_barrier(9) no longer takes the kernel
lock.
Am I on the right track? Aside from dropping the kernel lock there is
no behavior change here.
We can talk about inserting the barrier timeouts at the head of the
list to reduce latency in a different thread after we're done removing
the kernel lock here. Don't want to combine a behavior change with a
locking change in the same revision.
Index: sys/kern/kern_timeout.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_timeout.c,v
retrieving revision 1.83
diff -u -p -r1.83 kern_timeout.c
--- sys/kern/kern_timeout.c 8 Feb 2021 08:18:45 -0000 1.83
+++ sys/kern/kern_timeout.c 6 May 2021 16:39:56 -0000
@@ -172,10 +172,10 @@ void softclock_create_thread(void *);
void softclock_process_kclock_timeout(struct timeout *, int);
void softclock_process_tick_timeout(struct timeout *, int);
void softclock_thread(void *);
+void timeout_barrier_timeout(void *);
uint32_t timeout_bucket(const struct timeout *);
uint32_t timeout_maskwheel(uint32_t, const struct timespec *);
void timeout_run(struct timeout *);
-void timeout_proc_barrier(void *);
/*
* The first thing in a struct timeout is its struct circq, so we
@@ -490,34 +490,38 @@ timeout_del_barrier(struct timeout *to)
void
timeout_barrier(struct timeout *to)
{
- int needsproc = ISSET(to->to_flags, TIMEOUT_PROC);
+ struct timeout barrier;
+ struct cond c;
+ int procflag;
+
+ procflag = (to->to_flags & TIMEOUT_PROC);
+ timeout_sync_order(procflag);
+
+ timeout_set_flags(&barrier, timeout_barrier_timeout, &c, procflag);
+ barrier.to_process = curproc->p_p;
+ cond_init(&c);
- timeout_sync_order(needsproc);
-
- if (!needsproc) {
- KERNEL_LOCK();
- splx(splsoftclock());
- KERNEL_UNLOCK();
- } else {
- struct cond c = COND_INITIALIZER();
- struct timeout barrier;
-
- timeout_set_proc(&barrier, timeout_proc_barrier, &c);
- barrier.to_process = curproc->p_p;
+ mtx_enter(&timeout_mutex);
- mtx_enter(&timeout_mutex);
- SET(barrier.to_flags, TIMEOUT_ONQUEUE);
+ barrier.to_time = ticks;
+ SET(barrier.to_flags, TIMEOUT_ONQUEUE);
+ if (procflag)
CIRCQ_INSERT_TAIL(&timeout_proc, &barrier.to_list);
- mtx_leave(&timeout_mutex);
+ else
+ CIRCQ_INSERT_TAIL(&timeout_todo, &barrier.to_list);
+
+ mtx_leave(&timeout_mutex);
+ if (procflag)
wakeup_one(&timeout_proc);
+ else
+ softintr_schedule(softclock_si);
- cond_wait(&c, "tmobar");
- }
+ cond_wait(&c, "tmobar");
}
void
-timeout_proc_barrier(void *arg)
+timeout_barrier_timeout(void *arg)
{
struct cond *c = arg;
Index: share/man/man9/timeout.9
===================================================================
RCS file: /cvs/src/share/man/man9/timeout.9,v
retrieving revision 1.52
diff -u -p -r1.52 timeout.9
--- share/man/man9/timeout.9 26 Apr 2021 20:32:30 -0000 1.52
+++ share/man/man9/timeout.9 6 May 2021 16:39:56 -0000
@@ -194,11 +194,6 @@ but it will wait until any current execu
ensures that any current execution of the timeout in the argument
.Fa to
has completed before returning.
-If the timeout
-.Fa to
-has been initialised with
-.Fn timeout_set
-it will take the kernel lock.
.Pp
The
.Fn timeout_pending
