Skip to contentSkip to conversation

TTY: tty flip buffer change reserve memory strategy.

PATCH In review Ilya Zykov 3 participants 4 messages Oct 23, 2011 – Nov 1, 2011 TTY layer and serial drivers

lore
Share & tools
lore

Ilya Zykov

author
#0 lore raw

PATCH 1/1 TTY: tty flip buffer change reserve memory strategy.

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.

drivers/tty/tty_buffer.c +19 -5
diff -uprN last-orig/drivers/tty/tty_buffer.c tty_buffer.patched/drivers/tty/tty_buffer.c
@@ -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);
 }
 
include/linux/tty.h +4 -3
diff -uprN last-orig/include/linux/tty.h tty_buffer.patched/include/linux/tty.h
@@ -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
#1 lore raw
Quoted 5 lines

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.

Quoted 2 lines

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

author
#2 lore raw

Thank you, for answer.

Quoted 4 lines

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.

Quoted 7 lines

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.

#3 lore raw
Quoted 4 lines

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