David:
Your reply to my last message was so far off-base with respect to the
ideas I had intended to convey, I can only assume there's been a serious
communications failure. I'll do my best to clarify things below and show
where you went astray.
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.)
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!
On Fri, 30 Jul 2004, David Brownell wrote:
> > Suppose an upstream hub is locked for some reason while another thread
> > wants to work on a downstream device, either to reset it or to do event
> > processing (if the downstream device is another hub):
>
> Hubs aren't the only, or main, thing to lock ... see the other patch.
[I'll vent a little frustration just in this one place... Why did you say
that hubs aren't the only thing to lock? It's certainly true, but how is
it relevant to what I was saying? Are you referring to the "upstream hub"
I mentioned? If so then your remark is pointless -- obviously any
upstream device _must_ be a hub. If not, then are you referring to my
parenthetical comment about the downstream device being a hub? Again,
that would be pointless -- I'm specifically talking about the changes you
made to khubd, which only does event processing on hubs!]
Remember that the whole point of my discussion was to examine the worth of
using locktree in usb_reset_device() and hub_events(), and the only
situation in which locktree(usbdev) differs from down(&usbdev->serialize)
is when some hub upstream from usbdev is already locked.
> Remember that the khubd change was a "minimal change" patch;
> I think you want a performance-tuned one!
Not at all. And a "minimal change" patch wouldn't have changed
down(&hdev->serialize) in hub_events() to locktree(hdev)!
> > 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.
> > 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.
> > 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().
> 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?
> 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.
> > 4. The upstream hub is being reset. Part of the code I added
> > (probably in one of the patches that Greg hasn't applied yet)
> > changed usb_reset_device() so that when a hub is reset all of
> > its children are disconnected first. Once the downstream
> > device is gone, the question of whether to use locktree or
> > regular locking is moot.
>
> Mostly controlled by the spinlock ... maybe not as much as it
> should be though. Locktree handles that just fine.
Sure it does. My point is that regular locking does too, so locktree
isn't needed.
> > 5. The upstream hub is processing an overcurrent-change notification
> > for the port leading to the downstream device. All khubd does
> > when this happens is to clear the overcurrent-change feature.
> > There's no need for the other thread to wait for that to
> > complete, as locktree would make it do.
>
> It's not locktree() that makes that specific policy, it's khubd.
>
> Those (potential) performance issues each could be
> handled by khubd doing down(&hdev->child[i]->serialize)
> and then up(&hdev->serialize) before playing with that
> particular port. (Enumeration wouldn't quite work that
> way, though, child[i] being NULL.) But it'd have to get the
> hub lock, with locktree(), at the top of each loop.
Okay, here's a substantial point to consider.
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?
We could go back to using a private lock instead of ->serialize. That
would relieve some of the pressure on locktree. But why bother when
there's not really any need to use locktree in hub_events to begin with?
> > In short, I don't see any situations in which using locktree() for hub
> > event processing or device resets will provide any advantage at all. But
> > there _are_ situations (2 and 5) in which it will provide useless delays.
>
> 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)?
Case 1: B starts its reset before A starts its suspend. No
matter which locking scheme B uses, it will succeed immediately.
Thread A will block when its recursive tree descent reaches D
until the reset has completed, and A will suspend D then.
Result: tie score.
Case 2: B starts its reset after A has started its suspend but
before A has begun to suspend D. If B uses locktree then B will
have to wait until A has finished suspending everything below H
and releases the lock. Then B will see that D is suspended and
the reset will fail. If B uses regular locking then B will lock
D and reset it right away. A will have to wait until the reset
is complete, then it will suspend D and continue. Result: with
regular locking the reset succeeds instead of failing and does so
more quickly.
Case 3: B starts its reset after A has suspended D but before A
has finished suspending everything below H. If B uses locktree
then B will have to wait until A has finished, then B will see
that D is suspended and the reset will fail. If B uses regular
locking then B will lock D right away, will see that D is
suspended, and will fail. Result: with regular locking the reset
fails right away instead of after a delay.
Case 4: B starts its reset after A has finished its suspend. No
matter which locking scheme B uses, the lock will succeed
immediately and then the reset will fail. Result: tie score.
As you can see, in every case there are no sequencing errors and in no
case does regular locking do worse than locktree -- but in some cases it
does better. Other scenarios in which A is doing a resume or a reset, or
where B is running hub_events(), all work out similarly.
> > I foresee more problems. For example, whose responsibility should it
> > ultimately be to suspend a subtree of nodes in the USB tree -- the driver
> > core's or usbcore's?
>
> For now, USB ... the driver core doesn't handle this yet.
That was a rhetorical question. The point I was making (see just below)
is that the driver core can't be expected to handle this in the future,
since it doesn't possess the requisite knowledge. So usbcore has to do it
both now _and_ in the future.
> > The driver core doesn't have the knowledge to apply
> > the proper locking. And what should happen when there are non-USB devices
> > located below a USB node (a SCSI disk for example), who's responsible for
> > locking them?
>
> 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?
Alan Stern
-------------------------------------------------------
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
