On 01/09/2010, at 8:13 PM, Mark Kettenis wrote: >> From: David Gwynne <[email protected]> >> Date: Wed, 1 Sep 2010 19:35:15 +1000 >> >> On 01/09/2010, at 7:30 PM, Mark Kettenis wrote: >>> I realise you committed this already, but can you elaborate on those >>> XXX's in there? >> >> these two? > > yes > >> + if (bq->bufq_outstanding == 0 /* XXX and quiesced */) >> + SLIST_FOREACH(bq, &bufqs, bufq_entries) { /* XXX */ >> >> the first one refers to the fact that we call wakeup when we run out >> of io on the device, as opposed to when we run out of io on the >> device when we're waiting for the device to quiesce. > > Ah, that's a possible optimization. Could be worthwhile, since > wakeup(9) walks a list. I think > > if (bq->bufq_stop && bq->bufq_outstanding == 0) > > should do the trick. I'll look at that again tonight.
i just looked at it, that is the trick. ok by me. > >> the second is there cos the SLIST is handled without taking the >> global bufqs_mtx, which every other user of the SLIST takes before >> mucking around with it. > > That is completely intential, and actually essential for the quiescing > to work. The bufqs_mtx doesn't really protect the bufqs list, but > rather the bufqs_stop variable. The bufqs_stop variable protects the > list from being modified as soon as it is set to nonzero. > > I agree that this isn't entirely intuitive. It took us a couple of > iterations to fix it during c2k10 and afterwards. I'll replace the > XXX with a comment explaining what's going on. i get it now. the list is protected by bufqs_stop, which in turn is protected by the mutex. dlg
