On 22/03/2011 10:05, Johan Hovold wrote:
Say you fill up the tty buffer using the last of the sixteen buffers and
return in the else clause above, how will the tasklet ever get
re-scheduled?
That's a good point. I did check that this was happening during testing,
but looking into n_tty.c some more it seems that I probably just got
lucky with n_tty_receive_buf running to completion (and so throttling)
before n_tty_read got to run (and so un-throttling).
However I'm not 100% sure that I'm correctly understanding how
n_tty_receive_buf and n_tty_read interact. I can't see why it's safe for
n_tty_receive_buf to not have any sort of synchronization mechanism
between checking tty->receive_room and calling tty_throttle.
Is there a mechanism preventing a different thread from running
n_tty_read between n_tty_receive_buf finding receive_room to be below
the threshold and tty_throttle being called? If not then isn't there a
race condition when the following happens:
1. n_tty_receive_buf fills up the buffer so that the free space
is below TTY_THRESHOLD_THROTTLE
2. n_tty_receive_buf comes to the check at the end and decide
that it needs to call tty_throttle
3. Thread rescheduling happens and a different thread runs
n_tty_read which empties the buffer
4. After emptying the buffer n_tty_read calls tty_unthrottle,
which does nothing as the throttling bit isn't set
5. The n_tty_receive_buf thread is executed again, calling
tty_throttle, causing throttling, but with an empty buffer.
Or have I not understood a complexity in the interactions within n_tty.c?
[By the way, did you see Filippe Balbi's patch posted today claiming to
fix a bug in n_tty which could cause data loss at high speeds?]
I'd not seen that. Thanks for pointing that out.
I was just about to submit a patch series killing the rx tasklet and
heavily simplifying the cdc-acm driver when you posted last night. I
think that if this mechanism is needed it is more straight-forwardly
implemented on top of those as they removes a lot of complexity and
makes it easier to spot corner cases such as the one pointed out above.
Makes sense. The cdc-acm driver is certainly far more readable with your
changes.
I would also prefer a more generic solution to the problem so that we
don't need to re-introduce driver buffering again. Since we already have
the throttelling mechanism in place, if we could only be notified/find
out that the tty buffers are say half-full, we could throttle (from
within the driver) but still push the remaining buffer still on the wire
as they arrive. It would of course require a guarantee that such a
throttle-is-about-to-happen notification is actually followed by (a
throttle and) unthrottle. Thoughts on that?
I was thinking about this too. Would it be good enough to add the
ability for forcing tty throttling and to call that instead of issuing
another URB read when the space remaining in the buffer would then be
less than the pending URB reads? Or would having a notification at a
particular level be more useful for other drivers?
Regards,
Toby
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
