Move the wakeup of the done thread to inside the lock to avoid racing
xpt_done. This race is prevented today by the wakeup optimizations of only
deciding to wake up when we believe the thread is already sleeping. Remove the
sleep tracking part of that opatimization (since moving the wakeup solves that),
but still only wakeup when adding the first item to the queue (since we hold the
lock when manipulating the list, there's no race).
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 37775 Build 34664: arc lint + arc unit
Event Timeline
Comment Actions
I am not getting what is the point of bringing the wakeup() under the lock. Being out of the lock may cause extra wakeups, but they are not critical here.
Comment Actions
Doing the wakeup outside of the lock races the waker. If you do it inside of the lock, it won't race. The check to see if the queue is empty before adding will prevent the additional wakeup.
Comment Actions
It's a race that doesn't hurt though. It's actually a bit common to do wakeup's outside the lock as then you avoid having the thread potentially wake up only to immediately block on the lock that's still held. But also, Alexander is right, the only "lose" part of the race is that you might issue extra wakeups, but you shouldn't miss a wakeup.
Comment Actions
Races, and what? IIRC those threads are permanent. Putting more work under the lock increase the lock hold time and possibly contention. Otherwise contention moves to other lock, that is hopefully a bit smaller.
The check to see if the queue is empty before adding will prevent the additional wakeup.
"STAILQ_CONCAT(&doneq, &queue->cam_doneq);" can make the queue empty more often than sleep happen. The check for the queue empty is better then nothing and not require additional memory modifications as present code, so I am not sure about that part.
Comment Actions
Right now the race is between checking to see if the queue is empty and going to sleep with msleep. The cam_doneq_sleep reduces the window, but the window is still there. If we do the wakeup with the lock held, then that race isn't possible and the wakeup won't be missed. The only thing that doing the wakeup under the lock delays is xpt_done_td acquiring the lock at the bottom of the loop after it's finished calling the xpt_done_process routines for the ccbs that it is processing. This will be a rare race in the grand scheme of things.
The check to see if the queue is empty before adding will prevent the additional wakeup.
"STAILQ_CONCAT(&doneq, &queue->cam_doneq);" can make the queue empty more often than sleep happen. The check for the queue empty is better then nothing and not require additional memory modifications as present code, so I am not sure about that part.
There will be one wakeup each time the queue transitions from empty to non-empty due to the locks. This prevents work/wakeups when multiple completions happen before xpt_done_td can actually run and acquire the lock to empty the queue. There will be a few extra calls to wakeup, but wakeup looks up the sleepers in O(1) time and can find there are no sleepers so returns without doing additional work in those cases where xpt_done_process() is still running and we add an item.
Comment Actions
As me and John just told, the wakeup will not be missed in any case, it can only be duplicated due to the race, since list inserting is done before that under the lock.
The only thing that doing the wakeup under the lock delays is xpt_done_td acquiring the lock at the bottom of the loop after it's finished calling the xpt_done_process routines for the ccbs that it is processing. This will be a rare race in the grand scheme of things.
That is one of cases. Another is different thread calling xpt_done(). Since number of xpt_done_td() threads scales from number of CPUs, there can be multiple HBAs or queues to be mapped into the same xpt_done_td() thread, so lock contention there is possible. And I believe I saw one when moving the wakeup() out.
Comment Actions
OK. I worked through all the cases I could think of. Since we only change the sleep condition, any wakeups we might miss due to races will be when the list is empty. We can't hit this race when there's items in the queue, so we'll not sleep and due work in that case.... I'm sold. Thanks for the patient explanations...