Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1187873 > unrolled thread

[PATCH] Fix data loss in cdc-acm

Back to article view | Back to linux.kernel

Contents

  [PATCH] Fix data loss in cdc-acm Sven Brauch <mail@svenbrauch.de> - 2015-07-19 23:50 +0200
    Re: [PATCH] Fix data loss in cdc-acm Johan Hovold <johan@kernel.org> - 2015-07-20 19:30 +0200
      Re: [PATCH] Fix data loss in cdc-acm Sven Brauch <mail@svenbrauch.de> - 2015-07-20 20:10 +0200
        Re: [PATCH] Fix data loss in cdc-acm Oliver Neukum <oneukum@suse.com> - 2015-07-22 10:50 +0200
          Re: [PATCH] Fix data loss in cdc-acm Peter Hurley <peter@hurleysoftware.com> - 2015-07-22 16:40 +0200
            Re: [PATCH] Fix data loss in cdc-acm Oliver Neukum <oneukum@suse.com> - 2015-07-22 17:10 +0200
        Re: [PATCH] Fix data loss in cdc-acm Sven Brauch <mail@svenbrauch.de> - 2015-07-23 01:00 +0200
          Re: [PATCH] Fix data loss in cdc-acm Peter Stuge <peter@stuge.se> - 2015-07-27 12:10 +0200

#1187873 — [PATCH] Fix data loss in cdc-acm

[Multipart message — attachments visible in raw view] — view raw

Since acm_process_read_urb does not check the return value
of tty_insert_flip_string, it can happen that not all data
is copied from the urb to the tty if the tty buffer
is full and throttling does not set in quickly enough. This
problem is very evident for devices with high data throughput;
for a device with ~12 MB/s of data transfer, I get a few
missing kB of data every few MB transferred randomly.

To solve this problem, a check is introduced which verifies
that indeed all data was copied from the urb to the tty buffer.
If that is not the case, the urb is held in a queue instead
of resubmitting it to the USB subsystem right away. When new
data arrives or the tty is unthrottled, the queue is emptied
again (as far as possible).

Effectively, this change will force the transmitting USB device
to wait until the tty buffer can accept new data again, instead
of discarding the data in this case.

Please excuse the poor code quality, I have no experience
whatsoever in kernel development.

Signed-off-by: Sven Brauch <mail@svenbrauch.de>
---
 drivers/usb/class/cdc-acm.c | 93 +++++++++++++++++++++++++++++++++++++++++----
 drivers/usb/class/cdc-acm.h |  3 ++
 2 files changed, 89 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 5c8f581..eafe64c 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -402,14 +402,68 @@ static int acm_submit_read_urbs(struct acm *acm, gfp_t mem_flags)
 	return 0;
 }
 
-static void acm_process_read_urb(struct acm *acm, struct urb *urb)
+static int acm_process_read_urb(struct acm *acm, struct urb *urb)
 {
+	int size;
+
 	if (!urb->actual_length)
-		return;
+		return 0;
 
-	tty_insert_flip_string(&acm->port, urb->transfer_buffer,
-			urb->actual_length);
+	size = tty_insert_flip_string(&acm->port, urb->transfer_buffer,
+		urb->actual_length);
 	tty_flip_buffer_push(&acm->port);
+	return size;
+}
+
+static bool acm_push_leftover_data_to_tty(struct acm *acm) {
+	int j, k;
+	int urb_index;
+	unsigned long remaining, offset;
+	int written;
+	struct urb *urb;
+	struct acm_rb *rb;
+	bool may_submit_new = true;
+
+	/* TODO: Does this locking mechanism make any sense? I don't know.
+	         Some sort of lock is certainly required. */
+	unsigned long flags;
+	spin_lock_irqsave(&acm->resubmit_lock, flags);
+
+	/* Loop over the queued read urbs, and submit as much data as possible to the tty. */
+	for (j = 0; j < ACM_NR; j++) {
+		urb_index = acm->held_urbs[j];
+		remaining = acm->remaining_data_in_read_urbs[urb_index];
+		if (remaining == 0)
+			continue;
+
+		urb = acm->read_urbs[urb_index];
+		rb = urb->context;
+		offset = urb->actual_length - remaining;
+		written = tty_insert_flip_string(&acm->port, (char*) (urb->transfer_buffer) + offset,
+			remaining);
+		tty_flip_buffer_push(&acm->port);
+		acm->remaining_data_in_read_urbs[urb_index] = remaining - written;
+		if (remaining == written) {
+			/* The urb's buffer was fully submitted to the tty, it can be passed
+			 * to the USB subsystem again. */
+			set_bit(rb->index, &acm->read_urbs_free);
+			acm_submit_read_urb(acm, rb->index, GFP_ATOMIC);
+			acm->num_held_urbs--;
+		}
+		else {
+			/* There is no point in continuing now, the buffer is full */
+			may_submit_new = false;
+			break;
+		}
+	}
+	/* Move the remaining queued urbs to the beginning of the queue */
+	for (k = 0; k < acm->num_held_urbs; k++) {
+		acm->held_urbs[k] = acm->held_urbs[k+j];
+	}
+
+	spin_unlock_irqrestore(&acm->resubmit_lock, flags);
+
+	return may_submit_new;
 }
 
 static void acm_read_bulk_callback(struct urb *urb)
@@ -418,6 +472,11 @@ static void acm_read_bulk_callback(struct urb *urb)
 	struct acm *acm = rb->instance;
 	unsigned long flags;
 	int status = urb->status;
+	int size;
+	bool may_resubmit, may_submit_new;
+
+	/* First look if any filled urbs are still in the queue */
+	may_submit_new = acm_push_leftover_data_to_tty(acm);
 
 	dev_vdbg(&acm->data->dev, "%s - urb %d, len %d\n", __func__,
 					rb->index, urb->actual_length);
@@ -437,20 +496,36 @@ static void acm_read_bulk_callback(struct urb *urb)
 
 	usb_mark_last_busy(acm->dev);
 
-	acm_process_read_urb(acm, urb);
+	/*
+	 * Only try to post data of this new urb if the queue is empty,
+	 * otherwise put it to the end of the queue.
+	 */
+	size = may_submit_new ? acm_process_read_urb(acm, urb) : 0;
+
+	may_resubmit = true;
 	/*
 	 * Unthrottle may run on another CPU which needs to see events
 	 * in the same order. Submission has an implict barrier
 	 */
 	smp_mb__before_atomic();
-	set_bit(rb->index, &acm->read_urbs_free);
+	if (size != urb->actual_length) {
+		/* do not resubmit the urb, but mark it as filled instead */
+		acm->remaining_data_in_read_urbs[rb->index] = urb->actual_length - size;
+		acm->held_urbs[acm->num_held_urbs++] = rb->index;
+		may_resubmit = false;
+	}
+	else {
+		set_bit(rb->index, &acm->read_urbs_free);
+	}
 
 	/* throttle device if requested by tty */
 	spin_lock_irqsave(&acm->read_lock, flags);
 	acm->throttled = acm->throttle_req;
 	if (!acm->throttled) {
 		spin_unlock_irqrestore(&acm->read_lock, flags);
-		acm_submit_read_urb(acm, rb->index, GFP_ATOMIC);
+		if ( may_resubmit ) {
+			acm_submit_read_urb(acm, rb->index, GFP_ATOMIC);
+		}
 	} else {
 		spin_unlock_irqrestore(&acm->read_lock, flags);
 	}
@@ -774,6 +849,8 @@ static void acm_tty_unthrottle(struct tty_struct *tty)
 	acm->throttle_req = 0;
 	spin_unlock_irq(&acm->read_lock);
 
+	acm_push_leftover_data_to_tty(acm);
+
 	if (was_throttled)
 		acm_submit_read_urbs(acm, GFP_KERNEL);
 }
@@ -1367,6 +1444,8 @@ made_compressed_probe:
 		struct acm_rb *rb = &(acm->read_buffers[i]);
 		struct urb *urb;
 
+		acm->remaining_data_in_read_urbs[i] = 0;
+
 		rb->base = usb_alloc_coherent(acm->dev, readsize, GFP_KERNEL,
 								&rb->dma);
 		if (!rb->base)
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index ffeb3c8..0946b0a 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -93,6 +93,9 @@ struct acm {
 	struct acm_wb wb[ACM_NW];
 	unsigned long read_urbs_free;
 	struct urb *read_urbs[ACM_NR];
+	int held_urbs[ACM_NR+1], num_held_urbs; /* fixed-size queue for not-yet-processed read urbs */
+	unsigned long remaining_data_in_read_urbs[ACM_NR]; /* amount of bytes in each held urb */
+	spinlock_t resubmit_lock;
 	struct acm_rb read_buffers[ACM_NR];
 	int rx_buflimit;
 	int rx_endpoint;
-- 
2.4.6

[toc] | [next] | [standalone]

#1188385

[ +CC: Alan, Oliver, Peter, Toby, linux-serial, linux-usb ]

On Sun, Jul 19, 2015 at 11:37:07PM +0200, Sven Brauch wrote:
> Since acm_process_read_urb does not check the return value
> of tty_insert_flip_string, it can happen that not all data
> is copied from the urb to the tty if the tty buffer
> is full and throttling does not set in quickly enough. This
> problem is very evident for devices with high data throughput;
> for a device with ~12 MB/s of data transfer, I get a few
> missing kB of data every few MB transferred randomly.

What kernel version are you using?

> To solve this problem, a check is introduced which verifies
> that indeed all data was copied from the urb to the tty buffer.
> If that is not the case, the urb is held in a queue instead
> of resubmitting it to the USB subsystem right away. When new
> data arrives or the tty is unthrottled, the queue is emptied
> again (as far as possible).
> 
> Effectively, this change will force the transmitting USB device
> to wait until the tty buffer can accept new data again, instead
> of discarding the data in this case.

The idea of adding another layer of buffering in the cdc-acm driver has
been suggested in the past but was rejected (or at least questioned).
See for example this thread:

	https://lkml.kernel.org/r/20110608164626.22bc893c@lxorguk.ukuu.org.uk

The tty buffers are quite large these days, but could possibly be bumped
further if needed to give the ldisc some more time to throttle the
device at very high speeds.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1188415

[Multipart message — attachments visible in raw view] — view raw

On 20/07/15 19:25, Johan Hovold wrote:
> What kernel version are you using?
I'm using linux 4.1.2.

> The idea of adding another layer of buffering in the cdc-acm driver has
> been suggested in the past but was rejected (or at least questioned).
> See for example this thread:
> 
> 	https://lkml.kernel.org/r/20110608164626.22bc893c@lxorguk.ukuu.org.uk
Yes, that is indeed pretty much the same problem and the same solution.
Answering to the questions brought up in that thread:

> a) Why is your setup filling 64K in the time it takes the throttle
> response to occur
As far as I understand, the throttle happens only when there's less than
128 bytes of free space in the tty buffer. Data can already be lost
before the tty even decides it should start throttling, simply because
the throttle threshold is smaller than the amount of data potentially in
each urb. Also (excuse my cluelessness) it seems that when exactly the
throttling happens depends on some scheduling "jitter" as well.
Additionally, the response of the cdc_acm driver to a throttle request
is not very prompt; it might have a queue of up to 16kB (16 urbs) pending.

> b) Do we care (is the right thing to do to lose bits anyway at
> that point)
This I cannot answer, I don't know enough about the architecture or
standards. I can just say that for my case, there's a lot of losses;
this it not an issue which happens after hours when the system is under
heavy load, it happens after just a few seconds reproducably.

> The tty buffers are quite large these days, but could possibly be bumped
> further if needed to give the ldisc some more time to throttle the
> device at very high speeds.
I do not like this solution. It will again be based on luck, and you
will still be unable to rely on the delivery guarantee made by the USB
stack (at least when using bulk).
My suggestion instead stops the host system from accepting any more data
from the device when its buffers are full, forcing the device to wait
before sending out more data (which many kinds of devices might very
well be able to do).

Also note that this patch does not introduce an extra layer of
buffering. The buffers are already there; this change just alters the
process which decides when to submit the buffers to the tty, and when to
free them for more input data from the device.

Sven

[toc] | [prev] | [next] | [standalone]

#1189747

On Tue, 2015-07-21 at 12:45 -0400, Peter Hurley wrote:
> Let me know if you need help instrumenting the tty buffers/throttling
> to help figure out what the actual problem is.
> 
> Regarding the patch itself, I have no opinion on the suitability of
> simply not resubmitting urbs. However, that is exactly how the
> throttle
> mechanism works, and the tty buffer API is specifically designed to
> allow drivers to manage flow via that interface as well (especially
> for high-throughput drivers).

Could you please expand on how this is supposed to work?
For once how does one learn that room is available again?

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1190016

On 07/22/2015 04:40 AM, Oliver Neukum wrote:
> On Tue, 2015-07-21 at 12:45 -0400, Peter Hurley wrote:
>> Let me know if you need help instrumenting the tty buffers/throttling
>> to help figure out what the actual problem is.
>>
>> Regarding the patch itself, I have no opinion on the suitability of
>> simply not resubmitting urbs. However, that is exactly how the
>> throttle
>> mechanism works, and the tty buffer API is specifically designed to
>> allow drivers to manage flow via that interface as well (especially
>> for high-throughput drivers).
> 
> Could you please expand on how this is supposed to work?
> For once how does one learn that room is available again?

There are basically 3 mechanisms for managing rx data:
1. Allocate space when the data arrives; drop data if no space is avail
   and indicate buf_overrun. This is what most drivers do.
2. Allocate space when the data arrives; try to buffer uncopied data
   and resubmit the data later. Some high-throughput drivers (in the wild)
   do this (but less so now that the tty buffer space is configurable).
3. Pre-allocate space _before_ the data arrives (with tty_buffer_request_room());
   this is applicable to subsystems which know how much data can be in-flight
   at any one time. This guarantees that when rx data arrives buffer space is
   available (since it has already been allocated).

Drivers that use method 2 typically attempt to recopy the buffered data
when either new data arrives or @ unthrottle. I've seen others use deferred
work as well.

AFAIK no driver/subsystem is using method 3 for guaranteed delivery
of in-flight data, but it seems ideally suited to usb serial.

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1190045

On Wed, 2015-07-22 at 10:30 -0400, Peter Hurley wrote:
> 3. Pre-allocate space _before_ the data arrives (with
> tty_buffer_request_room());
>    this is applicable to subsystems which know how much data can be
> in-flight
>    at any one time. This guarantees that when rx data arrives buffer
> space is
>    available (since it has already been allocated).
> 
> Drivers that use method 2 typically attempt to recopy the buffered
> data
> when either new data arrives or @ unthrottle. I've seen others use
> deferred
> work as well.
> 
> AFAIK no driver/subsystem is using method 3 for guaranteed delivery
> of in-flight data, but it seems ideally suited to usb serial.

Indeed. But flow control is still done by throttle/unthrottle, isn't it?

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1190345

[Multipart message — attachments visible in raw view] — view raw

Hi,

On 23/07/15 00:12, Peter Hurley wrote:
> The premature unthrottle actually leads to the data loss but the throttling
> with a mere 2K left is _way too late_.
Ok, yes, I think so too.

> 10ms is a _really_ long time for a cpu not to attend to a kworker.
> Which raises 2 questions:
> 1. What are the termios settings of the tty receiving input? Is it 'raw'
>    mode or typical terminal mode (icanon, echo, etc.) or something else?
In my test code, I open the tty like
  fd = open("/dev/ttyACM0", O_RDWR | O_NOCTTY | O_NONBLOCK);
I don't make any other changes to the default settings. To be honest,
I'm not sure in which mode it is operating then (I was assuming raw, but
I might be wrong?).

> 2. Are there RT threads that are hogging cpu time?
I can't see any, I think the only thing which occasionally goes to RT is
pulseaudio (but during at least some of the tests I wasn't even playing
audio, so that sounds very unplausible to me). I also cannot see a
correlation between failure rate and CPU load.

Best regards,
Sven

[toc] | [prev] | [next] | [standalone]

#1192923

Sven Brauch wrote:
> I don't make any other changes to the default settings. To be honest,
> I'm not sure in which mode it is operating then (I was assuming raw, but
> I might be wrong?).

You should explicitly set a mode if you need a particular mode,
otherwise the port might be in another mode. This sets raw. Add
error checking.

struct termios t;

tcgetattr(fd, &t);
cfmakeraw(&t);
cfsetspeed(&t, B115200);
tcsetattr(fd, 0, &t);


//Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web