[PATCH 1/1] serial_core: Update buffer overrun statistics.

[PATCH 1/1] serial_core: Update buffer overrun statistics.

[Corbin Atkinson]

Currently, serial drivers don't report buffer overruns. When a buffer overrun
occurs, tty_insert_flip_char returns 0, and no attempt is made to insert that
same character again (i.e. it is lost). This patch reports buffer overruns via
the buf_overrun field in the port's icount structure.

Signed-off-by: Corbin Atkinson <corbin.atkinson@ni.com>
---
 drivers/tty/serial/serial_core.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 9c4c05b..59fb3ba 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2526,14 +2526,16 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
 	struct tty_struct *tty = port->state->port.tty;
 
 	if ((status & port->ignore_status_mask & ~overrun) == 0)
-		tty_insert_flip_char(tty, ch, flag);
+		if (tty_insert_flip_char(tty, ch, flag) == 0)
+			++port->icount.buf_overrun;
 
 	/*
 	 * Overrun is special.  Since it's reported immediately,
 	 * it doesn't affect the current character.
 	 */
 	if (status & ~port->ignore_status_mask & overrun)
-		tty_insert_flip_char(tty, 0, TTY_OVERRUN);
+		if (tty_insert_flip_char(tty, 0, TTY_OVERRUN) == 0)
+			++port->icount.buf_overrun;
 }
 EXPORT_SYMBOL_GPL(uart_insert_char);
 
-- 
1.7.5.4

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Alan Cox]

On Fri,  4 May 2012 12:35:10 -0500
Corbin Atkinson <corbinat@gmail.com> wrote:

> Currently, serial drivers don't report buffer overruns. When a buffer overrun
> occurs, tty_insert_flip_char returns 0, and no attempt is made to insert that
> same character again (i.e. it is lost). This patch reports buffer overruns via
> the buf_overrun field in the port's icount structure.

I think this is mostly a misunderstanding - it's always been interpreted
as logging hardware reported overrruns. The case of the software layer
losing bytes is a "doesn't happen in normal circumstances" situation. At
least it should be.

Alan

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Greg Kroah-Hartman]

On Tue, May 15, 2012 at 04:53:39PM +0100, Alan Cox wrote:
> On Fri,  4 May 2012 12:35:10 -0500
> Corbin Atkinson <corbinat@gmail.com> wrote:
> 
> > Currently, serial drivers don't report buffer overruns. When a buffer overrun
> > occurs, tty_insert_flip_char returns 0, and no attempt is made to insert that
> > same character again (i.e. it is lost). This patch reports buffer overruns via
> > the buf_overrun field in the port's icount structure.
> 
> I think this is mostly a misunderstanding - it's always been interpreted
> as logging hardware reported overrruns. The case of the software layer
> losing bytes is a "doesn't happen in normal circumstances" situation. At
> least it should be.

Hm, I applied this already, should I revert it?

thanks,

greg k-h

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Corbin]

I agree that losing bytes in the software layer wouldn't happen under
normal circumstances, but it is still important to know when it
happens. My understanding was that the overrun field in the port's
icount was for logging hardware overruns, while the buf_overrun field
should be used for software overruns. If this is not the case then is
there another method for determining when the software layer is losing
bytes?

Thanks,
Corbin

On Tue, May 15, 2012 at 10:58 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, May 15, 2012 at 04:53:39PM +0100, Alan Cox wrote:
> > On Fri,  4 May 2012 12:35:10 -0500
> > Corbin Atkinson <corbinat@gmail.com> wrote:
> >
> > > Currently, serial drivers don't report buffer overruns. When a buffer
> > > overrun
> > > occurs, tty_insert_flip_char returns 0, and no attempt is made to
> > > insert that
> > > same character again (i.e. it is lost). This patch reports buffer
> > > overruns via
> > > the buf_overrun field in the port's icount structure.
> >
> > I think this is mostly a misunderstanding - it's always been interpreted
> > as logging hardware reported overrruns. The case of the software layer
> > losing bytes is a "doesn't happen in normal circumstances" situation. At
> > least it should be.
>
> Hm, I applied this already, should I revert it?
>
> thanks,
>
> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Alan Cox]

On Tue, 15 May 2012 11:20:19 -0500
Corbin <corbinat@gmail.com> wrote:

> I agree that losing bytes in the software layer wouldn't happen under
> normal circumstances, but it is still important to know when it
> happens. My understanding was that the overrun field in the port's
> icount was for logging hardware overruns, while the buf_overrun field
> should be used for software overruns. If this is not the case then is
> there another method for determining when the software layer is losing
> bytes?

It'll occur when the machine cannot even allocate a free page of memory.
So you'll get GFP spews in the kernel log to go with it.

Alan

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Corbin]

Maybe I don't have a complete understanding of what is happening, but
it appears that the software buffer is actually artificially limited
to 65536 bytes in the tty_buffer_alloc function in tty_buffer.c. I
wrote a quick application to test this. The test opens two 8250 ports
on my computer and sends 69632 (68Kib) of data from one port to the
other. After the write, the test performs a read from the other port,
which only receives 69376 bytes. I believe the extra space over 64KiB
is coming from the line discipline (n_tty.c), but the test still shows
data being lost. Neither the overrun or buf_overrun field in the
port's icount structure were incremented during the test.

Here is the section of code I'm referencing from tty_buffer.c. This
code was pulled from linux-next.
static struct tty_buffer *tty_buffer_alloc(struct tty_struct *tty, size_t size)
{
	struct tty_buffer *p;

	if (tty->buf.memory_used + size > 65536)
		return NULL;
	p = kmalloc(sizeof(struct tty_buffer) + 2 * size, GFP_ATOMIC);
	if (p == NULL)
		return NULL;
	p->used = 0;
	p->size = size;
	p->next = NULL;
	p->commit = 0;
	p->read = 0;
	p->char_buf_ptr = (char *)(p->data);
	p->flag_buf_ptr = (unsigned char *)p->char_buf_ptr + size;
	tty->buf.memory_used += size;
	return p;
}

Corbin


On Tue, May 15, 2012 at 11:27 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Tue, 15 May 2012 11:20:19 -0500
> Corbin <corbinat@gmail.com> wrote:
>
>> I agree that losing bytes in the software layer wouldn't happen under
>> normal circumstances, but it is still important to know when it
>> happens. My understanding was that the overrun field in the port's
>> icount was for logging hardware overruns, while the buf_overrun field
>> should be used for software overruns. If this is not the case then is
>> there another method for determining when the software layer is losing
>> bytes?
>
> It'll occur when the machine cannot even allocate a free page of memory.
> So you'll get GFP spews in the kernel log to go with it.
>
> Alan

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Jason Smith]

On Tue, May 15, 2012 at 1:41 PM, Corbin <corbinat@gmail.com> wrote:
> Maybe I don't have a complete understanding of what is happening, but
> it appears that the software buffer is actually artificially limited
> to 65536 bytes in the tty_buffer_alloc function in tty_buffer.c. I

...

> On Tue, May 15, 2012 at 11:27 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> 
wrote:
>>
>> It'll occur when the machine cannot even allocate a free page of 
memory.
>> So you'll get GFP spews in the kernel log to go with it.

It seems to me that it would be undesirable to allow an ignored TTY device 
to consume all memory on the system.

It makes sense that the TTY layer would limit it to 64K, which is what 
Corbin observed in his test, but then there should be a mechanism to 
report that data had been discarded.

-Jason Smith

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Alan Cox]

> it appears that the software buffer is actually artificially limited
> to 65536 bytes in the tty_buffer_alloc function in tty_buffer.c. I

Yes although you shouldn't be able to hit this with flow control
enabled. Without flow control we didn't overrun at that level.

it's really a semantics question - do we count stuff dumped
intentionally without flow control as an overrun.

I'm not fundamentally opposed to the idea but it is a change of meaning.

Other views ?

Alan

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Corbin]

On Tue, May 15, 2012 at 4:08 PM, Alan Cox <alan@linux.intel.com> wrote:
>> it appears that the software buffer is actually artificially limited
>> to 65536 bytes in the tty_buffer_alloc function in tty_buffer.c. I
>
> Yes although you shouldn't be able to hit this with flow control
> enabled. Without flow control we didn't overrun at that level.
>
> it's really a semantics question - do we count stuff dumped
> intentionally without flow control as an overrun.
>
> I'm not fundamentally opposed to the idea but it is a change of meaning.
>
> Other views ?
>
> Alan

While I agree on normal use cases this won't be hit due to flow
control, I think it is still important to somehow know data was lost
in the case where flow control isn't used. Calling it a buffer overrun
was the way I thought was most fitting. I'm not sure what the
alternatives would be.

On a side note, If this isn't considered a buffer overrun I'm also not
too sure what the buf_overrun field of the port's icount struct is
for. While the "overrun" field is incremented in most drivers to
indicate a hardware overrun, the "buf_overrun" field isn't really
used. It appears that at least one of the few drivers using
buf_overrun (cyclades.c) is interpreting it in the same way that I am,
incrementing it when tty_buffer_request_room returns 0.

Thanks again,

Corbin

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Jason Smith]

On Tue, May 15, 2012 at 5:46 PM, Corbin <corbinat@gmail.com> wrote:
> On Tue, May 15, 2012 at 4:08 PM, Alan Cox <alan@linux.intel.com> wrote:
>>> it appears that the software buffer is actually artificially limited
>>> to 65536 bytes in the tty_buffer_alloc function in tty_buffer.c. I
>>
>> Yes although you shouldn't be able to hit this with flow control
>> enabled. Without flow control we didn't overrun at that level.
>>
>> it's really a semantics question - do we count stuff dumped
>> intentionally without flow control as an overrun.
>>
>> I'm not fundamentally opposed to the idea but it is a change of 
meaning.
>>
>> Other views ?
>>
>> Alan
>
> While I agree on normal use cases this won't be hit due to flow
> control, I think it is still important to somehow know data was lost
> in the case where flow control isn't used. Calling it a buffer overrun
> was the way I thought was most fitting. I'm not sure what the
> alternatives would be.
>
> On a side note, If this isn't considered a buffer overrun I'm also not
> too sure what the buf_overrun field of the port's icount struct is
> for. While the "overrun" field is incremented in most drivers to
> indicate a hardware overrun, the "buf_overrun" field isn't really
> used. It appears that at least one of the few drivers using
> buf_overrun (cyclades.c) is interpreting it in the same way that I am,
> incrementing it when tty_buffer_request_room returns 0.


Has anyone had a chance to think about this anymore? We would really like 
the capability to recognize that data has been discarded, even if it is 
because no flow control is used and the user is ignoring the port. There 
are cases, such as with RS-485, where there is no real flow control 
mechanism is available, so we should not depend on flow-control as always 
being an option. As Corbin mentioned, there are some other drivers using 
buf_overrun (as opposed to overrun, which indicates FIFO overruns) for 
this purpose, but we don't know if this was the original intention of this 
field.

-Jason

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Alan Cox]

> Has anyone had a chance to think about this anymore? We would really like 
> the capability to recognize that data has been discarded, even if it is 
> because no flow control is used and the user is ignoring the port. There 
> are cases, such as with RS-485, where there is no real flow control 
> mechanism is available, so we should not depend on flow-control as always 
> being an option. As Corbin mentioned, there are some other drivers using 
> buf_overrun (as opposed to overrun, which indicates FIFO overruns) for 
> this purpose, but we don't know if this was the original intention of this 
> field.

I was hopng Jiri would chip in.

If you think its important and want to argue for it, I'm not going to
fight against it.

Alan

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Corbin]

On Mon, May 21, 2012 at 11:05 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> I was hopng Jiri would chip in.

Jiri,

What do you think of my patch?

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Jiri Slaby]

On 05/22/2012 10:31 PM, Corbin wrote:
> On Mon, May 21, 2012 at 11:05 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> I was hopng Jiri would chip in.
> 
> Jiri,
> 
> What do you think of my patch?

That it is perfectly concealed.

(I'm not subscribed to linux-serial and cannot find it anywhere. A link
would be appreciated...)

thanks,
-- 
js
suse labs

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Corbin]

On Tue, May 22, 2012 at 3:38 PM, Jiri Slaby <jslaby@suse.cz> wrote:

> That it is perfectly concealed.
>
> (I'm not subscribed to linux-serial and cannot find it anywhere. A link
> would be appreciated...)

Oh, sorry about that. Here is an archived version of the email thread:
http://www.spinics.net/lists/linux-serial/msg06104.html
I'll also copy and paste my patch here:

Currently, serial drivers don't report buffer overruns. When a buffer overrun
occurs, tty_insert_flip_char returns 0, and no attempt is made to insert that
same character again (i.e. it is lost). This patch reports buffer overruns via
the buf_overrun field in the port's icount structure.

Signed-off-by: Corbin Atkinson <corbin.atkinson@xxxxxx>
---
 drivers/tty/serial/serial_core.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 9c4c05b..59fb3ba 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2526,14 +2526,16 @@ void uart_insert_char(struct uart_port *port,
unsigned int status,
 	struct tty_struct *tty = port->state->port.tty;

 	if ((status & port->ignore_status_mask & ~overrun) == 0)
-		tty_insert_flip_char(tty, ch, flag);
+		if (tty_insert_flip_char(tty, ch, flag) == 0)
+			++port->icount.buf_overrun;

 	/*
 	 * Overrun is special.  Since it's reported immediately,
 	 * it doesn't affect the current character.
 	 */
 	if (status & ~port->ignore_status_mask & overrun)
-		tty_insert_flip_char(tty, 0, TTY_OVERRUN);
+		if (tty_insert_flip_char(tty, 0, TTY_OVERRUN) == 0)
+			++port->icount.buf_overrun;
 }
 EXPORT_SYMBOL_GPL(uart_insert_char);

-- 
1.7.5.4

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Corbin]

Has anyone been able to give this any more thought? If no one has any
objection I would still like to see this get added.

Thanks,
Corbin

Re: [PATCH 1/1] serial_core: Update buffer overrun statistics.

[Alan Cox]

On Wed, 6 Jun 2012 17:34:45 -0500
Corbin <corbinat@gmail.com> wrote:

> Has anyone been able to give this any more thought? If no one has any
> objection I would still like to see this get added.

Nobody seems to care too much so I'm ok on that basis.