On Tue, May 04, 2021 at 11:24:05AM +0200, Martin Pieuchot wrote:
> On 04/05/21(Tue) 01:10, Scott Cheloha wrote:
> > [...]
> > I want to run softclock() without the kernel lock. The way to go, I
> > think, is to first push the kernel lock down into timeout_run(), and
> > then to remove the kernel lock from each timeout, one by one.
>
> Grabbing and releasing the KERNEL_LOCk() on a per-timeout basis creates
> more latency than running all timeouts in a batch after having grabbed
> the KERNEL_LOCK(). I doubt this is the best way forward.
I have heard different views on this.
visa@ said once, and I paraphrase, "increasing the latency is a good
way to motivate people to remove the kernel lock from their timeouts."
If this "tough love" approach is unacceptable, we could also divide
timeouts up into two queues based on the presence of a flag, say
TIMEOUT_MPSAFE. You would then run part of the softclock() under the
kernel lock and the other part without it. We would probably pre-sort
the timeouts at scheduling time, i.e. timeout_add(9), to avoid an
additional O(n) traversal dring softclock() to sort them.
> > Before we can push the kernel lock down into timeout_run() we need to
> > remove the kernel lock from timeout_del_barrier(9).
>
> Seems worth it on its own.
Yes. dlg@ seems to agree, too.
> > The kernel lock is used in timeout_del_barrier(9) to determine whether
> > the given timeout has stopped running. Because the softclock() runs
> > with the kernel lock we currently assume that once the calling thread
> > has taken the kernel lock any onging softclock() must have returned
> > and relinquished the lock, so the timeout in question has returned.
>
> So you want to stop using the KERNEL_LOCK() to do the serialization?
Exactly.
> > The simplest replacement I can think of is a volatile pointer to the
> > running timeout that we set before leaving the timeout_mutex and clear
> > after reentering the same during timeout_run().
>
> Sounds like a condition variable protected by this mutex. Interesting
> that cond_wait(9) doesn't work with a mutex.
I can't speak on this. dlg@ can.
> > So in the non-TIMEOUT_PROC case the timeout_del_barrier(9) caller just
> > spins until the timeout function returns and the timeout_running
> > pointer is changed. Not every caller can sleep during
> > timeout_del_barrier(9). I think spinning is the simplest thing that
> > will definitely work here.
>
> This keeps the current semantic indeed.
Apparently not. See dlg@'s mail.
> > -void
> > -timeout_barrier(struct timeout *to)
> > +int
> > +timeout_del_barrier(struct timeout *to)
> > {
> > + struct timeout barrier;
> > + struct cond c = COND_INITIALIZER();
> > int needsproc = ISSET(to->to_flags, TIMEOUT_PROC);
> >
> > timeout_sync_order(needsproc);
> >
> > - if (!needsproc) {
> > - KERNEL_LOCK();
> > - splx(splsoftclock());
> > - KERNEL_UNLOCK();
> > - } else {
> > - struct cond c = COND_INITIALIZER();
> > - struct timeout barrier;
> > + mtx_enter(&timeout_mutex);
> > +
> > + if (timeout_del_locked(to)) {
> > + mtx_leave(&timeout_mutex);
> > + return 1;
> > + }
> >
> > + if (needsproc) {
> > timeout_set_proc(&barrier, timeout_proc_barrier, &c);
> > barrier.to_process = curproc->p_p;
> > -
> > - mtx_enter(&timeout_mutex);
> > SET(barrier.to_flags, TIMEOUT_ONQUEUE);
> > CIRCQ_INSERT_TAIL(&timeout_proc, &barrier.to_list);
> > mtx_leave(&timeout_mutex);
> > -
> > wakeup_one(&timeout_proc);
> > -
> > cond_wait(&c, "tmobar");
> > + } else {
> > + mtx_leave(&timeout_mutex);
> > + /* XXX Is this in the right spot? */
> > + splx(splsoftclock());
> > + while (timeout_running == to)
> > + CPU_BUSY_CYCLE();
>
> Won't splx() will execute the soft-interrupt if there's any pending?
Yes, I believe that that's the point. If the calling thread is
running on the primary CPU we want to give softclock() a chance to run
before we spin.
> Shouldn't the barrier be before? Could you add `spc->spc_spinning++'
> around the spinning loop?
That's easy enough, sure.
> What happen if two threads call timeout_del_barrier(9) with the same
> argument at the same time? Is it possible and/or supported?
That would work as expected, yes.
(okay, now onto dlg@'s mail.)
