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

Groups > linux.kernel > #1251935 > unrolled thread

Re: [PATCH] Fix data loss in cdc-acm

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] Fix data loss in cdc-acm Peter Hurley <peter@hurleysoftware.com> - 2015-10-20 20:20 +0200
    Re: [PATCH] Fix data loss in cdc-acm Oliver Neukum <oneukum@suse.com> - 2015-10-21 12:20 +0200
      Re: [PATCH] Fix data loss in cdc-acm Peter Hurley <peter@hurleysoftware.com> - 2015-10-21 14:10 +0200
        Re: [PATCH] Fix data loss in cdc-acm Peter Hurley <peter@hurleysoftware.com> - 2015-10-21 17:00 +0200

#1251935 — Re: [PATCH] Fix data loss in cdc-acm

On 08/05/2015 01:36 PM, Peter Hurley wrote:
> On 07/22/2015 06:53 PM, Sven Brauch wrote:
>> 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.
> 
> I haven't forgotten about this problem and I still plan to look into why
> the long delay, particularly focusing on the scheduling latency from
> tty_flip_buffer_push() => flush_to_ldisc().

I made some changes wrt which CPU is scheduled for the flush_to_ldisc()
input worker, which should reduce the kworker latency. The changes are
on the tty-next branch of Greg's tty.git tree; please test at your earliest
convenience.


>>> 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?).

ECHO is on by default and the cdc-acm driver does not implement the
put_char() and flush_chars() tty driver methods, which made the problem
_way worse_, since every echoed char is sent as it's own URB.

Since echoing is performed by the input worker, this seems the likely
cause for the significant delay in ldisc processing.

Echo processing should probably be performed by a separate kworker rather
than by the input kworker.

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] | [next] | [standalone]

#1252678

On Tue, 2015-10-20 at 14:16 -0400, Peter Hurley wrote:
> ECHO is on by default and the cdc-acm driver does not implement the
> put_char() and flush_chars() tty driver methods, which made the
> problem
> _way worse_, since every echoed char is sent as it's own URB.

That can be fixed. How can I test this? Will the tty layer use write()
and put_char()?

	Regards
		Oliver

From ac75a2c9ea67ec22c704a6bcaa30e6fc415d64d3 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Wed, 21 Oct 2015 12:10:07 +0200
Subject: [PATCH] cdc-acm: implement put_char() and flush_chars()

This should cut down latencies and waste if the tty layer writes single bytes.

Signed-off-by: Oliver Neukum >oneukum@suse.com>
---
 drivers/usb/class/cdc-acm.c | 58 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/class/cdc-acm.h |  1 +
 2 files changed, 59 insertions(+)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index b30e742..262f179 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -725,6 +725,62 @@ static int acm_tty_write(struct tty_struct *tty,
 	return count;
 }
 
+static void acm_tty_flush_chars(struct tty_struct *tty)
+{
+	struct acm *acm = tty->driver_data;
+	struct acm_wb *cur = acm->putbuffer;
+	int err;
+	unsigned long flags;
+
+	acm->putbuffer = NULL;
+	err = usb_autopm_get_interface_async(acm->control);
+	spin_lock_irqsave(&acm->write_lock, flags);
+	if (err < 0) {
+		cur->use = 0;
+		goto out;
+	}
+
+	if (acm->susp_count) {
+		usb_anchor_urb(cur->urb, &acm->delayed);
+		goto out;
+	}
+
+	acm_start_wb(acm, cur);
+out:
+	spin_unlock_irqrestore(&acm->write_lock, flags);
+	return;
+}
+
+static int acm_tty_put_char(struct tty_struct *tty, unsigned char ch)
+{
+	struct acm *acm = tty->driver_data;
+	struct acm_wb *cur;
+	int wbn;
+	unsigned long flags;
+
+overflow:
+	cur = acm->putbuffer;
+	if (!cur) {
+		spin_lock_irqsave(&acm->write_lock, flags);
+		wbn = acm_wb_alloc(acm);
+		if (wbn >= 0) {
+			cur = &acm->wb[wbn];
+			acm->putbuffer = cur;
+		}
+		spin_unlock_irqrestore(&acm->write_lock, flags);
+		if (!cur)
+			return 0;
+	}
+
+	if (cur->len == acm->writesize) {
+		acm_tty_flush_chars(tty);
+		goto overflow;
+	}
+
+	cur->buf[cur->len++] = ch;
+	return 1;
+}
+
 static int acm_tty_write_room(struct tty_struct *tty)
 {
 	struct acm *acm = tty->driver_data;
@@ -1888,6 +1944,8 @@ static const struct tty_operations acm_ops = {
 	.cleanup =		acm_tty_cleanup,
 	.hangup =		acm_tty_hangup,
 	.write =		acm_tty_write,
+	.put_char =		acm_tty_put_char,
+	.flush_chars =		acm_tty_flush_chars,
 	.write_room =		acm_tty_write_room,
 	.ioctl =		acm_tty_ioctl,
 	.throttle =		acm_tty_throttle,
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index dd9af38..648a6f7 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -94,6 +94,7 @@ struct acm {
 	unsigned long read_urbs_free;
 	struct urb *read_urbs[ACM_NR];
 	struct acm_rb read_buffers[ACM_NR];
+	struct acm_wb *putbuffer;			/* for acm_tty_put_char() */
 	int rx_buflimit;
 	int rx_endpoint;
 	spinlock_t read_lock;
-- 
2.1.4



--
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]

#1252766

On 10/21/2015 06:12 AM, Oliver Neukum wrote:
> On Tue, 2015-10-20 at 14:16 -0400, Peter Hurley wrote:
>> ECHO is on by default and the cdc-acm driver does not implement the
>> put_char() and flush_chars() tty driver methods, which made the
>> problem
>> _way worse_, since every echoed char is sent as it's own URB.
> 
> That can be fixed. How can I test this?

I'm working on getting my tty tests organized for upstreaming, but it's
not there yet. Let me see if I can get you a standalone test for
accuracy, at least.

> Will the tty layer use write() and put_char()?

Echoing uses put_char() + flush_chars() for echoing, if the driver
supports it; otherwise, it uses write().

Be aware that while the N_TTY line discipline ensures that put_char()
and write() usage will not be interleaved without flush_chars(), other
line disciplines might not, so at least be sure that driver won't
crash if that happens.

Regards,
Peter Hurley


> From ac75a2c9ea67ec22c704a6bcaa30e6fc415d64d3 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.com>
> Date: Wed, 21 Oct 2015 12:10:07 +0200
> Subject: [PATCH] cdc-acm: implement put_char() and flush_chars()
> 
> This should cut down latencies and waste if the tty layer writes single bytes.
> 
> Signed-off-by: Oliver Neukum >oneukum@suse.com>
> ---
>  drivers/usb/class/cdc-acm.c | 58 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/class/cdc-acm.h |  1 +
>  2 files changed, 59 insertions(+)
> 
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index b30e742..262f179 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -725,6 +725,62 @@ static int acm_tty_write(struct tty_struct *tty,
>  	return count;
>  }
>  
> +static void acm_tty_flush_chars(struct tty_struct *tty)
> +{
> +	struct acm *acm = tty->driver_data;
> +	struct acm_wb *cur = acm->putbuffer;
> +	int err;
> +	unsigned long flags;
> +
> +	acm->putbuffer = NULL;
> +	err = usb_autopm_get_interface_async(acm->control);
> +	spin_lock_irqsave(&acm->write_lock, flags);
> +	if (err < 0) {
> +		cur->use = 0;
> +		goto out;
> +	}
> +
> +	if (acm->susp_count) {
> +		usb_anchor_urb(cur->urb, &acm->delayed);
> +		goto out;
> +	}
> +
> +	acm_start_wb(acm, cur);
> +out:
> +	spin_unlock_irqrestore(&acm->write_lock, flags);
> +	return;
> +}
> +
> +static int acm_tty_put_char(struct tty_struct *tty, unsigned char ch)
> +{
> +	struct acm *acm = tty->driver_data;
> +	struct acm_wb *cur;
> +	int wbn;
> +	unsigned long flags;
> +
> +overflow:
> +	cur = acm->putbuffer;
> +	if (!cur) {
> +		spin_lock_irqsave(&acm->write_lock, flags);
> +		wbn = acm_wb_alloc(acm);
> +		if (wbn >= 0) {
> +			cur = &acm->wb[wbn];
> +			acm->putbuffer = cur;
> +		}
> +		spin_unlock_irqrestore(&acm->write_lock, flags);
> +		if (!cur)
> +			return 0;
> +	}
> +
> +	if (cur->len == acm->writesize) {
> +		acm_tty_flush_chars(tty);
> +		goto overflow;
> +	}
> +
> +	cur->buf[cur->len++] = ch;
> +	return 1;
> +}
> +
>  static int acm_tty_write_room(struct tty_struct *tty)
>  {
>  	struct acm *acm = tty->driver_data;
> @@ -1888,6 +1944,8 @@ static const struct tty_operations acm_ops = {
>  	.cleanup =		acm_tty_cleanup,
>  	.hangup =		acm_tty_hangup,
>  	.write =		acm_tty_write,
> +	.put_char =		acm_tty_put_char,
> +	.flush_chars =		acm_tty_flush_chars,
>  	.write_room =		acm_tty_write_room,
>  	.ioctl =		acm_tty_ioctl,
>  	.throttle =		acm_tty_throttle,
> diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
> index dd9af38..648a6f7 100644
> --- a/drivers/usb/class/cdc-acm.h
> +++ b/drivers/usb/class/cdc-acm.h
> @@ -94,6 +94,7 @@ struct acm {
>  	unsigned long read_urbs_free;
>  	struct urb *read_urbs[ACM_NR];
>  	struct acm_rb read_buffers[ACM_NR];
> +	struct acm_wb *putbuffer;			/* for acm_tty_put_char() */
>  	int rx_buflimit;
>  	int rx_endpoint;
>  	spinlock_t read_lock;
> 

--
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]

#1252911

On 10/21/2015 08:09 AM, Peter Hurley wrote:
> Be aware that while the N_TTY line discipline ensures that put_char()
> and write() usage will not be interleaved without flush_chars(), other
> line disciplines might not, so at least be sure that driver won't
> crash if that happens.

Sorry, I take that back: certain echoes are output via write() in
OPOST mode, specifically for O_ONLCR and XTABS.
--
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