[PATCH] TTY: tty flip buffer change reserve memory strategy.

[PATCH] TTY: tty flip buffer change reserve memory strategy.

[Ilya Zykov]

Currently, free flip buffer (tty->buf->free) reserve memory for further
used,
only if driver send to ldisc less 257 bytes in one time.
If driver send more, flip buffer reserve(kmalloc()) and then
free(kfree()) every chunk more 256 bytes every time,
even we have in free buffer enough space, because we can't
pass through chunk boundary in free buffer. Also we can't limit reserve
memory size. In worse case we will have in free buffer:
(256 * 256 byte chunk) = (256 * 552 ) = 141312 byte unused memory.

This patch allow reserve more than 256 bytes in one time. And have
self-regulation chunk size ability(very useful for pty).
Also we can limit reserve memory size, but then,
we will be use kmalloc()-kree() calls on intensive stream from driver.

diff -uprN last-orig/drivers/tty/tty_buffer.c
tty_buffer.patched/drivers/tty/tty_buffer.c
--- last-orig/drivers/tty/tty_buffer.c    2011-10-18 13:57:32.000000000
+0400
+++ tty_buffer.patched/drivers/tty/tty_buffer.c    2011-10-22
09:45:09.000000000 +0400
@@ -58,7 +58,7 @@ static struct tty_buffer *tty_buffer_all
 {
     struct tty_buffer *p;
 
-    if (tty->buf.memory_used + size > 65536)
+    if (tty->buf.memory_used + size > TTY_BUFFER_MAX)
         return NULL;
     p = kmalloc(sizeof(struct tty_buffer) + 2 * size, GFP_ATOMIC);
     if (p == NULL)
@@ -91,10 +91,21 @@ static void tty_buffer_free(struct tty_s
     tty->buf.memory_used -= b->size;
     WARN_ON(tty->buf.memory_used < 0);
 
-    if (b->size >= 512)
-        kfree(b);
-    else {
-        b->next = tty->buf.free;
+    tty->buf.memory_reserve += b->size;
+    b->next = NULL;
+    if (tty->buf.free != NULL) {
+        struct tty_buffer *p = tty->buf.free;
+        while (p->next != NULL)
+            p = p->next;
+        p->next = b;
+        while (tty->buf.memory_reserve > TTY_BUFFER_RESERVE ) {
+            p = tty->buf.free;
+            tty->buf.free = p->next;
+            tty->buf.memory_reserve -= p->size;
+            WARN_ON(tty->buf.memory_reserve < 0);
+            kfree(p);
+        }
+    } else {
         tty->buf.free = b;
     }
 }
@@ -175,6 +186,8 @@ static struct tty_buffer *tty_buffer_fin
             t->commit = 0;
             t->read = 0;
             tty->buf.memory_used += t->size;
+            tty->buf.memory_reserve -= t->size;
+            WARN_ON(tty->buf.memory_reserve < 0);
             return t;
         }
         tbh = &((*tbh)->next);
@@ -517,6 +530,7 @@ void tty_buffer_init(struct tty_struct *
     tty->buf.tail = NULL;
     tty->buf.free = NULL;
     tty->buf.memory_used = 0;
+    tty->buf.memory_reserve = 0;
     INIT_WORK(&tty->buf.work, flush_to_ldisc);
 }
 
diff -uprN last-orig/include/linux/tty.h
tty_buffer.patched/include/linux/tty.h
--- last-orig/include/linux/tty.h    2011-05-19 08:06:34.000000000 +0400
+++ tty_buffer.patched/include/linux/tty.h    2011-10-22
13:48:46.000000000 +0400
@@ -79,7 +79,8 @@ struct tty_buffer {
  */
 
 #define TTY_BUFFER_PAGE    (((PAGE_SIZE - sizeof(struct tty_buffer)) /
2) & ~0xFF)
-
+#define TTY_BUFFER_MAX        65536
+#define TTY_BUFFER_RESERVE    TTY_BUFFER_MAX
 
 struct tty_bufhead {
     struct work_struct work;
@@ -87,8 +88,8 @@ struct tty_bufhead {
     struct tty_buffer *head;    /* Queue head */
     struct tty_buffer *tail;    /* Active buffer */
     struct tty_buffer *free;    /* Free queue head */
-    int memory_used;        /* Buffer space used excluding
-                                free queue */
+    int memory_used;        /* Buffer space used excluding free queue */
+    int memory_reserve;        /* Free queue space */
 };
 /*
  * When a break, frame error, or parity error happens, these codes are

[PATCH] TTY: tty flip buffer change reserve memory strategy.

[Ilya Zykov]

Currently, free flip buffer (tty->buf->free) reserve memory for further used,
only if driver send to ldisc less 257 bytes in one time.
If driver send more, flip buffer reserve(kmalloc()) and then
free(kfree()) every chunk more 256 bytes every time,
even we have in free buffer enough space, because we can't
pass through chunk boundary in free buffer. Also we can't limit reserve
memory size. In worse case we will have in free buffer:
(256 * 256 byte chunk) = (256 * 552 ) = 141312 byte unused memory.

This patch allow reserve more than 256 bytes in one time. And have
self-regulation chunk size ability(very useful for pty).
Also we can limit reserve memory size, but then,
we will be use kmalloc()-kree() calls on intensive stream from the driver.

diff -uprN last-orig/drivers/tty/tty_buffer.c tty_buffer.patched/drivers/tty/tty_buffer.c
--- last-orig/drivers/tty/tty_buffer.c	2011-10-18 13:57:32.000000000 +0400
+++ tty_buffer.patched/drivers/tty/tty_buffer.c	2011-10-22 09:45:09.000000000 +0400
@@ -58,7 +58,7 @@ static struct tty_buffer *tty_buffer_all
 {
 	struct tty_buffer *p;
 
-	if (tty->buf.memory_used + size > 65536)
+	if (tty->buf.memory_used + size > TTY_BUFFER_MAX)
 		return NULL;
 	p = kmalloc(sizeof(struct tty_buffer) + 2 * size, GFP_ATOMIC);
 	if (p == NULL)
@@ -91,10 +91,21 @@ static void tty_buffer_free(struct tty_s
 	tty->buf.memory_used -= b->size;
 	WARN_ON(tty->buf.memory_used < 0);
 
-	if (b->size >= 512)
-		kfree(b);
-	else {
-		b->next = tty->buf.free;
+	tty->buf.memory_reserve += b->size;
+	b->next = NULL;
+	if (tty->buf.free != NULL) {
+		struct tty_buffer *p = tty->buf.free;
+		while (p->next != NULL)
+			p = p->next;
+		p->next = b;
+		while (tty->buf.memory_reserve > TTY_BUFFER_RESERVE ) {
+			p = tty->buf.free;
+			tty->buf.free = p->next;
+			tty->buf.memory_reserve -= p->size;
+			WARN_ON(tty->buf.memory_reserve < 0);
+			kfree(p);
+		}
+	} else {
 		tty->buf.free = b;
 	}
 }
@@ -175,6 +186,8 @@ static struct tty_buffer *tty_buffer_fin
 			t->commit = 0;
 			t->read = 0;
 			tty->buf.memory_used += t->size;
+			tty->buf.memory_reserve -= t->size;
+			WARN_ON(tty->buf.memory_reserve < 0);
 			return t;
 		}
 		tbh = &((*tbh)->next);
@@ -517,6 +530,7 @@ void tty_buffer_init(struct tty_struct *
 	tty->buf.tail = NULL;
 	tty->buf.free = NULL;
 	tty->buf.memory_used = 0;
+	tty->buf.memory_reserve = 0;
 	INIT_WORK(&tty->buf.work, flush_to_ldisc);
 }
 
diff -uprN last-orig/include/linux/tty.h tty_buffer.patched/include/linux/tty.h
--- last-orig/include/linux/tty.h	2011-05-19 08:06:34.000000000 +0400
+++ tty_buffer.patched/include/linux/tty.h	2011-10-22 13:48:46.000000000 +0400
@@ -79,7 +79,8 @@ struct tty_buffer {
  */
 
 #define TTY_BUFFER_PAGE	(((PAGE_SIZE - sizeof(struct tty_buffer)) / 2) & ~0xFF)
-
+#define TTY_BUFFER_MAX		65536
+#define TTY_BUFFER_RESERVE	TTY_BUFFER_MAX
 
 struct tty_bufhead {
 	struct work_struct work;
@@ -87,8 +88,8 @@ struct tty_bufhead {
 	struct tty_buffer *head;	/* Queue head */
 	struct tty_buffer *tail;	/* Active buffer */
 	struct tty_buffer *free;	/* Free queue head */
-	int memory_used;		/* Buffer space used excluding
-								free queue */
+	int memory_used;		/* Buffer space used excluding free queue */
+	int memory_reserve;		/* Free queue space */
 };
 /*
  * When a break, frame error, or parity error happens, these codes are

Re: [PATCH] TTY: tty flip buffer change reserve memory strategy.

[Alan Cox]

> free(kfree()) every chunk more 256 bytes every time,
> even we have in free buffer enough space, because we can't
> pass through chunk boundary in free buffer. Also we can't limit reserve
> memory size. In worse case we will have in free buffer:
> (256 * 256 byte chunk) = (256 * 552 ) = 141312 byte unused memory.

The idea is to trade off between the devices that need long linear blocks
(mostly for DMA) and devices that don't care - but many of which pass 1
or 2 bytes/irq so want a common buffer.

Most drivers should not be using tty_reserve_room in the first place.

> Also we can limit reserve memory size, but then,
> we will be use kmalloc()-kree() calls on intensive stream from the driver.

kmalloc is very fast anyway so I'd be interested in seeing the drivers
suffering and the profile traces of a real example.

I'm also not sure I agree with the maths - we chunk buffers to 256 byte
boundaries to avoid getting lots of queued buffers of different sizes. We
could in fact I think simplify the code from what we have learned using
it and simply treat the 256 byte buffer case as special rather than 256
and 512 as we do now. That would let us remove the more generic support
for free buffer sizes which is overcomplex and mean we could simply count
the number of 256 byte buffers we have to hand.

Alan

Re: [PATCH] TTY: tty flip buffer change reserve memory strategy.

[Ilya Zykov]

Thank you, for answer.

> 
> kmalloc is very fast anyway so I'd be interested in seeing the drivers
> suffering and the profile traces of a real example.
> 

Pty use chunk more 256 bytes very often. More program for communicate with
pppd use pty master - slave. If often kmalloc()-kfree() calls don't provoke
memory fragmentation and flush TLB, I agree it isn't important.


> I'm also not sure I agree with the maths - we chunk buffers to 256 byte
> boundaries to avoid getting lots of queued buffers of different sizes. We
> could in fact I think simplify the code from what we have learned using
> it and simply treat the 256 byte buffer case as special rather than 256
> and 512 as we do now. That would let us remove the more generic support
> for free buffer sizes which is overcomplex and mean we could simply count
> the number of 256 byte buffers we have to hand.

Now we can't accumulate buffer more than 256 byte. 
"if (b->size >= 512) kfree(b);" and our buffer can be only 256, 512 ... .
We can simplify tty_buffer_find() for don't look up buffer more 256 byte.

I think my patch very clear, because if stream from driver very slow we 
will be use only two 256 byte chunk, as before. But with pty, maybe, new drivers,
we could use flip buffer more effective.

Ilya.

Re: [PATCH] TTY: tty flip buffer change reserve memory strategy.

[Alan Cox]

> Pty use chunk more 256 bytes very often. More program for communicate
> with pppd use pty master - slave. If often kmalloc()-kfree() calls
> don't provoke memory fragmentation and flush TLB, I agree it isn't
> important.

I think any work of this nature ought to involve benchmarking. That
would answer the questions better. kmalloc/kfree is a smart allocator
so shouldn't cause problems, and it doesn't manipulate address space so
shouldn't cause TLB misses.

You want to make these changes, you say they are needed, but what
actually talks is real numbers.

Alan