Re: [PATCH v2] USB: cdc-acm: Prevent data loss when filling tty buffer.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux