On Monday 02 August 2004 09:36, Alan Stern wrote: > > My main point: There's no need to use locktree() in hub_events() or > usb_reset_device(). (Although your latest changes didn't actually add > locktree to usb_reset_device, there was a comment about doing it.)
I guess you didn't like my examples ... :) The reset code changes aren't critical, at least not yet. I expect locking arguments to continue for a while yet, regardless. If you don't think that issuing non-data signaling on a port (affecting everything downstream) shouldn't be a reason to lock that whole tree, how would you propose keeping tasks using that sort of signaling from interfering with each other? Or keeping other tasks from interfering, even if they don't use that sort of signaling? The kind of answer I want is simple, like "everyone just use locktree() and it works". Special cases are negatives. Clearly there's no "need" to have a simple rule like that; an armload of special cases (or even just a handful) could work too. Likewise no need to have a cheap way to lock multiple devices; one _could_ just as well grab all the device locks for a whole subtree before starting some complex operation. > Basically, there are no situations where using locktree in either of those > two places would provide any advantage, although there are times when > doing so would cause unnecessary delays. If you disagree, then try to > provide an example where using locktree in either of those routines would > work but using down(&dev->serialize) wouldn't! The "unnecessary" delays thing is at most a performance tweak, as I've pointed out. I'd say the others are necessary to provide a simple predictable event sequencing rule (top down), so the locking rule can be described with fewer exceptions. > > Remember that the khubd change was a "minimal change" patch; > > I think you want a performance-tuned one! > > Not at all. I think I see what you're saying: You don't see preventing one task from interfering with another as a goal. So you don't draw the distinction between prevention that's necessary, and prevention that's stronger than necessary (since that patch changed one line vs, say, a couple dozen). > And a "minimal change" patch wouldn't have changed > down(&hdev->serialize) in hub_events() to locktree(hdev)! No, it had to do that to prevent khubd from interfering with (or being interfered with by) some other task manipulating portions of the same tree, that's how the call sequences work. Anyway, khubd is the primary task manipulating the forest of USB busses ... it'd make no sense to have a lock policy with a big "except for khubd" clause. Your locking change patch changed khubd in the same spot, and for much the same reason... > > > 2. The upstream hub is being suspended. Your recursive suspend > > > code will lock the downstream device when it gets there. So > > > > That is, two tasks concurrently changing power/state needing to > > coordinate with each other. In this case they're both suspending, > > and the "topmost" suspend started first. > > "... two tasks ... both suspending..."? Where did that come from? I was > talking about two threads, one suspending an upstream hub, then other > running either usb_reset_device() on a downstream device or else running > hub_events on a downstream hub. Your later comments seem to include the > same mistaken assumption that I was talking about two threads both > suspending or resuming. That wasn't clear to me from what you first wrote ... > > > it doesn't matter what sort of locking is used by the other > > > thread working on the device; both locktree and regular locking > > > will have to wait until the device is suspended. > > > > I don't know what you mean by "regular", but that's certainly > > not true of what's (not) in RC2 ... :) > > By "regular" locking I mean down(&usbdev->serialize) or its equivalent. > In other words, the kind of locking that was in hub_events() before you > changed it to use locktree. RC2 doesn't have suspend code, much less suspend code that tries to lock that way... > > > 3. The upstream hub is being resumed. Under these circumstances > > > it's unlikely that anyone would want to do anything to the > > > downstream device since it must still be suspended, but never > > > mind that. > > > > > Resume processing will recursively lock devices > > > below the upstream hub as they are resumed. The downstream > > > device will get locked in its turn, so again it doesn't matter > > > whether the other thread uses locktree or regular locking. > > > > Erm, and what is this second thread doing that sequence wouldn't matter? > > I didn't say that _sequencing_ wouldn't matter; I said that the _type of > locking_ wouldn't matter. And as I mentioned above, the second thread > is either doing usb_reset_device() or hub_events(). Since the two styles of locking produce different event sequences (by design/intent), I'm not sure how to interpret your comment. If the sequencing matters, then the type of locking matters; if locking doesn't matter, then sequencing can't either. And it's only become clear just now what you were implying that the second task was doing. I'm not sure yet how my responses would change given that new reading. > > Disconnecting ... locktree uses the (newish) spinlock and is failfast for > > all callers, so not that. Resetting .... the resume needs to finish > > first, not that then. > > Come again? Sorry, the mousepad on this keyboard sometimes acts up so that typing doesn't land where it should ... annoying when I forget to turn it off! Re disconnect, everyone should be failfast so that doesn't matter. The resume needs to finish first, else the suspend fails needlessly. > > Powering off ... ditto, to ensure clean shutdown. > > Suspending ... same. > > > > That is, the sequence DOES matter, and that's another part of what > > locktree() is delivering. > > Of course sequence matters, and NO, locktree does not deliver it any more > than regular down(&usbdev->serialize) type locking does, apart from > causing certain races to go one way rather than the other. Well, the database folk have bad things to say about event sequences that can't be serialized ... in fact locktree() is a serialization algorithm, designed to let tasks work concurrently without sacrificing several handy characteristics. > First, notice that it's _not_ okay for khubd to completely drop all locks > on a hub while processing its events. There has to be some way for it to > synchronize when the hub is disconnected. Until recently this was done > using the private khubd_semaphore. I recall you being very keen on the > idea that this semaphore could now go away and be replaced with > hdev->serialize. Have you changed your mind about that? No, but I don't see your point. If everything is fail-fast on the disconnect paths, anything that touches a disconnected device (hub or otherwise) will drop it like a hot potato, so that very quickly it's only disconnect() processing at work. I think we're pretty much at that point now. > > See above; event sequencing is important, and it's easy to drop > > the hub lock earlier on certain event paths. > > Let's talk about event sequencing too. As an example, let's consider > thread A doing a suspend on an upstream hub H while thread B is trying to > reset a downstream device D. The question is, does using locktree(D) in > thread B give better sequencing than just doing down(&D->serialize)? Depends on what "better" means. For example, if it means that a task working on a subtree is guaranteed no other task will interfere with it, locktree() is much better. > > Locking them for what purpose? > > I meant to say "suspending them", not "locking them", sorry. Given that > usbcore must take responsibility for suspending USB devices and can't > delegate it to the driver core, what happens when the device tree > traversal needed for proper suspension goes outside the USB boundary? Good question. I think that the PM core should have a routine that's responsible for suspending all of a subtree. Instead it only has code to do that for the degenerate subtree: the whole darn system! - Dave ------------------------------------------------------- This SF.Net email is sponsored by OSTG. Have you noticed the changes on Linux.com, ITManagersJournal and NewsForge in the past few weeks? Now, one more big change to announce. We are now OSTG- Open Source Technology Group. Come see the changes on the new OSTG site. www.ostg.com _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
