Date: Wed, 03 Oct 2012 17:05:19 +0300 From: Andriy Gapon <avg@FreeBSD.org> To: attilio@FreeBSD.org Cc: freebsd-hackers <freebsd-hackers@FreeBSD.org>, Jeff Roberson <jeff@FreeBSD.org> Subject: Re: ule+smp: small optimization for turnstile priority lending Message-ID: <506C461F.2050008@FreeBSD.org> In-Reply-To: <CAJ-FndAiiH5ToUtns=jC7_V27BkeOvJ8_T9SOmw2DGdFyfvTdg@mail.gmail.com> References: <50587F8D.9060102@FreeBSD.org> <CAJ-FndCWzTBRYsA0mFDCj8RU06ZUTi3G0LeEFcS9_c5zKd%2BgWQ@mail.gmail.com> <5058C68B.1010508@FreeBSD.org> <CAJ-FndC8j12a95N0QprYTLJLC06R8jjcaHuxEZKu5D9RHW=ZVw@mail.gmail.com> <50596019.5060708@FreeBSD.org> <CAJ-FndBxWYOkRCn-DZTdZ%2BB4RpvsvaxtwDyMc8M7YhRj9DaL2w@mail.gmail.com> <505AF836.7050004@FreeBSD.org> <CAJ-FndAiiH5ToUtns=jC7_V27BkeOvJ8_T9SOmw2DGdFyfvTdg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
on 20/09/2012 16:14 Attilio Rao said the following:
> On 9/20/12, Andriy Gapon <avg@freebsd.org> wrote:
[snip]
>> The patch works well as far as I can tell. Thank you!
>> There is one warning with full witness enables but it appears to be harmless
>> (so
>> far):
>
> Andriy,
> thanks a lot for your testing and reports you made so far.
> Unfortunately I'm going off for 2 weeks now and I won't work on
> FreeBSD for that timeframe. I will get back to those in 2 weeks then.
> If you want to play more with this idea feel free to extend/fix/etc.
> this patch.
Unfortunately I haven't found time to work on this further, but I have some
additional thoughts.
First, I think that the witness message was not benign and it actually warns about
the same kind of deadlock that I originally had.
The problem is that sched_lend_prio() is called with target thread's td_lock held,
which is a lock of tdq on the thread's CPU. Then, in your patch, we acquire
current tdq's lock to modify its load. But if two tdq locks are to be held at the
same time, then they must be locked using tdq_lock_pair, so that lock order is
maintained. With the patch no tdq lock order can be maintained (can arbitrary)
and thus it is possible to run into a deadlock.
I see two possibilities here, but don't rule out that there can be more.
1. Do something like my patch does. That is, manipulate current thread's tdq in
advance before any other thread/tdq locks are acquired (or td_lock is changed to
point to a different lock and current tdq is unlocked). The API can be made more
generic in nature. E.g. it can look like this:
void
sched_thread_block(struct thread *td, int inhibitor)
{
struct tdq *tdq;
THREAD_LOCK_ASSERT(td, MA_OWNED);
KASSERT(td == curthread,
("sched_thread_block: only curthread is supported"));
tdq = TDQ_SELF();
TDQ_LOCK_ASSERT(tdq, MA_OWNED);
MPASS(td->td_lock == TDQ_LOCKPTR(tdq));
TD_SET_INHIB(td, inhibitor);
tdq_load_rem(tdq, td);
tdq_setlowpri(tdq, td);
}
2. Try to do things from sched_lend_prio based on curthread's state. This, as it
seems, would require completely lock-less manipulations of current tdq. E.g. for
tdq_load we could use atomic operations (it is already accessed locklessly, but
not modified). But for tdq_lowpri a more elaborate trick would be required like
having a separate field for a temporary value.
In any case, I'll have to revisit this later.
--
Andriy Gapon
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?506C461F.2050008>