[PATCH] PPP: Optimize write buffer management

[PATCH] PPP: Optimize write buffer management

[Patrick Porlan]

[-- Attachment #1: Type: text/plain, Size: 9327 bytes --]

Replace modulo operations in ringbuffer.c by masking operations.
That's possible because the size of the ring buffers is always
a power of two, and yields a small performance improvement.

More importantly, extend the write buffer handling in gathdlc.c
to minimize stalling and process switching during large PPP transfers.
The single write buffer is replaced by a circular queue of buffers,
allowing for much larger emission windows without hugely impacting
memory consumption. This reduces the time required to send 50 MB
between a couple of local PPP interfaces on my laptop from 53s to 2.6s.
---
 gatchat/gathdlc.c    |   85 +++++++++++++++++++++++++++++++++++++-------------
 gatchat/ringbuffer.c |   14 +++++---
 2 files changed, 71 insertions(+), 28 deletions(-)

diff --git a/gatchat/gathdlc.c b/gatchat/gathdlc.c
index dd11043..9cc6a74 100644
--- a/gatchat/gathdlc.c
+++ b/gatchat/gathdlc.c
@@ -37,7 +37,9 @@
 #include "gatio.h"
 #include "gathdlc.h"
 
-#define BUFFER_SIZE 2048
+#define BUFFER_SIZE	4096
+#define MAX_BUFFERS	64	/* Maximum number of in-flight write buffers: this needs to be a power of 2 */
+#define HDLC_OVERHEAD	256	/* Rough estimate of HDLC protocol overhead for a frame */
 
 #define HDLC_FLAG	0x7e	/* Flag sequence */
 #define HDLC_ESCAPE	0x7d	/* Asynchronous control escape */
@@ -51,7 +53,9 @@
 struct _GAtHDLC {
 	gint ref_count;
 	GAtIO *io;
-	struct ring_buffer *write_buffer;
+	struct ring_buffer *wbuf_ring[MAX_BUFFERS]; /* Array of pointers to circular write buffers - itself managed as a ring */
+	gint wbuf_head; /* First unsent buffer index */
+	gint wbuf_tail; /* Last unsent buffer index */
 	unsigned char *decode_buffer;
 	guint decode_offset;
 	guint16 decode_fcs;
@@ -190,6 +194,7 @@ GAtHDLC *g_at_hdlc_new_from_io(GAtIO *io)
 {
 	GAtHDLC *hdlc;
 	unsigned char *buf;
+	struct ring_buffer* write_buffer;
 
 	if (io == NULL)
 		return NULL;
@@ -207,16 +212,19 @@ GAtHDLC *g_at_hdlc_new_from_io(GAtIO *io)
 	hdlc->xmit_accm[3] = 0x60000000; /* 0x7d, 0x7e */
 	hdlc->recv_accm = ~0U;
 
-	hdlc->write_buffer = ring_buffer_new(BUFFER_SIZE * 2);
-	if (!hdlc->write_buffer)
+	write_buffer = ring_buffer_new(BUFFER_SIZE);
+	if (!write_buffer)
 		goto error;
 
+	hdlc->wbuf_head = hdlc->wbuf_tail = 0;
+	hdlc->wbuf_ring[0] = write_buffer;
+
 	/* Write an initial 0x7e as wakeup character */
-	buf = ring_buffer_write_ptr(hdlc->write_buffer, 0);
+	buf = ring_buffer_write_ptr(write_buffer, 0);
 	*buf = HDLC_FLAG;
-	ring_buffer_write_advance(hdlc->write_buffer, 1);
+	ring_buffer_write_advance(write_buffer, 1);
 
-	hdlc->decode_buffer = g_try_malloc(BUFFER_SIZE * 2);
+	hdlc->decode_buffer = g_try_malloc(BUFFER_SIZE);
 	if (!hdlc->decode_buffer)
 		goto error;
 
@@ -228,8 +236,8 @@ GAtHDLC *g_at_hdlc_new_from_io(GAtIO *io)
 	return hdlc;
 
 error:
-	if (hdlc->write_buffer)
-		ring_buffer_free(hdlc->write_buffer);
+	if (write_buffer)
+	    ring_buffer_free(write_buffer);
 
 	if (hdlc->decode_buffer)
 		g_free(hdlc->decode_buffer);
@@ -266,6 +274,8 @@ GAtHDLC *g_at_hdlc_ref(GAtHDLC *hdlc)
 
 void g_at_hdlc_unref(GAtHDLC *hdlc)
 {
+	int i;
+
 	if (hdlc == NULL)
 		return;
 
@@ -280,7 +290,11 @@ void g_at_hdlc_unref(GAtHDLC *hdlc)
 	g_at_io_unref(hdlc->io);
 	hdlc->io = NULL;
 
-	ring_buffer_free(hdlc->write_buffer);
+	for (i = hdlc->wbuf_head; i != hdlc->wbuf_tail; i = (i+1) % MAX_BUFFERS)
+	    ring_buffer_free(hdlc->wbuf_ring[i]);
+
+	ring_buffer_free(hdlc->wbuf_ring[i]);
+
 	g_free(hdlc->decode_buffer);
 
 	if (hdlc->in_read_handler)
@@ -314,17 +328,26 @@ static gboolean can_write_data(gpointer data)
 	unsigned int len;
 	unsigned char *buf;
 	gsize bytes_written;
+	struct ring_buffer* write_buffer = hdlc->wbuf_ring[hdlc->wbuf_head];
 
-	len = ring_buffer_len_no_wrap(hdlc->write_buffer);
-	buf = ring_buffer_read_ptr(hdlc->write_buffer, 0);
+	len = ring_buffer_len_no_wrap(write_buffer);
+	buf = ring_buffer_read_ptr(write_buffer, 0);
 
 	bytes_written = g_at_io_write(hdlc->io, (gchar *) buf, len);
 	hdlc_record(hdlc->record_fd, FALSE, buf, bytes_written);
-	ring_buffer_drain(hdlc->write_buffer, bytes_written);
+	ring_buffer_drain(write_buffer, bytes_written);
 
-	if (ring_buffer_len(hdlc->write_buffer) > 0)
+	if (ring_buffer_len(write_buffer) > 0)
 		return TRUE;
 
+	/* We're done with this buffer : adjust head if there is at least another buffer in flight */
+	if (hdlc->wbuf_head != hdlc->wbuf_tail) {
+		ring_buffer_free(write_buffer);
+		hdlc->wbuf_head = (hdlc->wbuf_head + 1) % MAX_BUFFERS;
+
+		return TRUE;
+	}
+
 	return FALSE;
 }
 
@@ -356,19 +379,37 @@ GAtIO *g_at_hdlc_get_io(GAtHDLC *hdlc)
 
 gboolean g_at_hdlc_send(GAtHDLC *hdlc, const unsigned char *data, gsize size)
 {
-	unsigned int avail = ring_buffer_avail(hdlc->write_buffer);
-	unsigned int wrap = ring_buffer_avail_no_wrap(hdlc->write_buffer);
-	unsigned char *buf = ring_buffer_write_ptr(hdlc->write_buffer, 0);
+	struct ring_buffer* write_buffer = hdlc->wbuf_ring[hdlc->wbuf_tail];
+	unsigned int avail = ring_buffer_avail(write_buffer);
+	unsigned int wrap = ring_buffer_avail_no_wrap(write_buffer);
+	unsigned char *buf;
 	unsigned char tail[2];
 	unsigned int i = 0;
 	guint16 fcs = HDLC_INITFCS;
 	gboolean escape = FALSE;
 	gsize pos = 0;
 
-	if (avail < size)
-		return FALSE;
+	if (avail < size + HDLC_OVERHEAD) {
+		/* Switch to a new buffer */
+
+		if ((hdlc->wbuf_tail + 1) % MAX_BUFFERS == hdlc->wbuf_head)
+			return FALSE;	/* Ring full */
+
+		write_buffer = ring_buffer_new(BUFFER_SIZE);
+
+		if (!write_buffer)
+			return FALSE;
+
+		hdlc->wbuf_tail = (hdlc->wbuf_tail + 1) % MAX_BUFFERS;
+		hdlc->wbuf_ring[hdlc->wbuf_tail] = write_buffer;
+
+		avail = ring_buffer_avail(write_buffer);
+		wrap = ring_buffer_avail_no_wrap(write_buffer);
+		buf = ring_buffer_write_ptr(write_buffer, 0);
+	}
 
 	i = 0;
+	buf = ring_buffer_write_ptr(write_buffer, 0);
 
 	while (pos < avail && i < size) {
 		if (escape == TRUE) {
@@ -387,7 +428,7 @@ gboolean g_at_hdlc_send(GAtHDLC *hdlc, const unsigned char *data, gsize size)
 		pos++;
 
 		if (pos == wrap)
-			buf = ring_buffer_write_ptr(hdlc->write_buffer, pos);
+			buf = ring_buffer_write_ptr(write_buffer, pos);
 	}
 
 	if (i < size)
@@ -414,7 +455,7 @@ gboolean g_at_hdlc_send(GAtHDLC *hdlc, const unsigned char *data, gsize size)
 		pos++;
 
 		if (pos == wrap)
-			buf = ring_buffer_write_ptr(hdlc->write_buffer, pos);
+		    buf = ring_buffer_write_ptr(write_buffer, pos);
 	}
 
 	if (i < sizeof(tail))
@@ -426,7 +467,7 @@ gboolean g_at_hdlc_send(GAtHDLC *hdlc, const unsigned char *data, gsize size)
 	*buf = HDLC_FLAG;
 	pos++;
 
-	ring_buffer_write_advance(hdlc->write_buffer, pos);
+	ring_buffer_write_advance(write_buffer, pos);
 
 	g_at_io_set_write_handler(hdlc->io, can_write_data, hdlc);
 
diff --git a/gatchat/ringbuffer.c b/gatchat/ringbuffer.c
index becd3f8..27be3a8 100644
--- a/gatchat/ringbuffer.c
+++ b/gatchat/ringbuffer.c
@@ -34,6 +34,7 @@
 struct ring_buffer {
 	unsigned char *buffer;
 	unsigned int size;
+	unsigned int mask;
 	unsigned int in;
 	unsigned int out;
 };
@@ -61,6 +62,7 @@ struct ring_buffer *ring_buffer_new(unsigned int size)
 	}
 
 	buffer->size = real_size;
+	buffer->mask = real_size - 1;
 	buffer->in = 0;
 	buffer->out = 0;
 
@@ -78,7 +80,7 @@ int ring_buffer_write(struct ring_buffer *buf, const void *data,
 	len = MIN(len, buf->size - buf->in + buf->out);
 
 	/* Determine how much to write before wrapping */
-	offset = buf->in % buf->size;
+	offset = buf->in & buf->mask;
 	end = MIN(len, buf->size - offset);
 	memcpy(buf->buffer+offset, d, end);
 
@@ -93,12 +95,12 @@ int ring_buffer_write(struct ring_buffer *buf, const void *data,
 unsigned char *ring_buffer_write_ptr(struct ring_buffer *buf,
 					unsigned int offset)
 {
-	return buf->buffer + (buf->in + offset) % buf->size;
+	return buf->buffer + ((buf->in + offset) & buf->mask);
 }
 
 int ring_buffer_avail_no_wrap(struct ring_buffer *buf)
 {
-	unsigned int offset = buf->in % buf->size;
+	unsigned int offset = buf->in & buf->mask;
 	unsigned int len = buf->size - buf->in + buf->out;
 
 	return MIN(len, buf->size - offset);
@@ -121,7 +123,7 @@ int ring_buffer_read(struct ring_buffer *buf, void *data, unsigned int len)
 	len = MIN(len, buf->in - buf->out);
 
 	/* Grab data from buffer starting at offset until the end */
-	offset = buf->out % buf->size;
+	offset = buf->out & buf->mask;
 	end = MIN(len, buf->size - offset);
 	memcpy(d, buf->buffer + offset, end);
 
@@ -150,7 +152,7 @@ int ring_buffer_drain(struct ring_buffer *buf, unsigned int len)
 
 int ring_buffer_len_no_wrap(struct ring_buffer *buf)
 {
-	unsigned int offset = buf->out % buf->size;
+	unsigned int offset = buf->out & buf->mask;
 	unsigned int len = buf->in - buf->out;
 
 	return MIN(len, buf->size - offset);
@@ -159,7 +161,7 @@ int ring_buffer_len_no_wrap(struct ring_buffer *buf)
 unsigned char *ring_buffer_read_ptr(struct ring_buffer *buf,
 					unsigned int offset)
 {
-	return buf->buffer + (buf->out + offset) % buf->size;
+	return buf->buffer + ((buf->out + offset) & buf->mask);
 }
 
 int ring_buffer_len(struct ring_buffer *buf)
-- 
1.7.1

Re: [PATCH] PPP: Optimize write buffer management

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 11545 bytes --]

Hi Patrick,

On 03/01/2011 09:07 AM, Patrick Porlan wrote:
> Replace modulo operations in ringbuffer.c by masking operations.
> That's possible because the size of the ring buffers is always
> a power of two, and yields a small performance improvement.
> 
> More importantly, extend the write buffer handling in gathdlc.c
> to minimize stalling and process switching during large PPP transfers.
> The single write buffer is replaced by a circular queue of buffers,
> allowing for much larger emission windows without hugely impacting
> memory consumption. This reduces the time required to send 50 MB
> between a couple of local PPP interfaces on my laptop from 53s to 2.6s.

This is a great description, but right after reading it one should
realize that this patch is better broken down into two.  The first patch
addressing the ringbuffer performance improvements and the second one
dealing with PPP/HDLC queuing.  In fact, if there were two patches, then
ringbuffer changes could go in right away.

> ---
>  gatchat/gathdlc.c    |   85 +++++++++++++++++++++++++++++++++++++-------------
>  gatchat/ringbuffer.c |   14 +++++---
>  2 files changed, 71 insertions(+), 28 deletions(-)
> 
> diff --git a/gatchat/gathdlc.c b/gatchat/gathdlc.c
> index dd11043..9cc6a74 100644
> --- a/gatchat/gathdlc.c
> +++ b/gatchat/gathdlc.c
> @@ -37,7 +37,9 @@
>  #include "gatio.h"
>  #include "gathdlc.h"
>  
> -#define BUFFER_SIZE 2048
> +#define BUFFER_SIZE	4096
> +#define MAX_BUFFERS	64	/* Maximum number of in-flight write buffers: this needs to be a power of 2 */
> +#define HDLC_OVERHEAD	256	/* Rough estimate of HDLC protocol overhead for a frame */
>  

Please try to avoid going over 80 characters a line.  Sometimes it is
unavoidable, but lets try very hard to avoid it.

>  #define HDLC_FLAG	0x7e	/* Flag sequence */
>  #define HDLC_ESCAPE	0x7d	/* Asynchronous control escape */
> @@ -51,7 +53,9 @@
>  struct _GAtHDLC {
>  	gint ref_count;
>  	GAtIO *io;
> -	struct ring_buffer *write_buffer;
> +	struct ring_buffer *wbuf_ring[MAX_BUFFERS]; /* Array of pointers to circular write buffers - itself managed as a ring */
> +	gint wbuf_head; /* First unsent buffer index */
> +	gint wbuf_tail; /* Last unsent buffer index */

Is there a particular reason why you chose to use a ring buffer of ring
buffers? A simple GQueue might be much easier to understand.  If you are
worried about 'infinite queuing' then a simple counter might help to
alleviate that.

>  	unsigned char *decode_buffer;
>  	guint decode_offset;
>  	guint16 decode_fcs;
> @@ -190,6 +194,7 @@ GAtHDLC *g_at_hdlc_new_from_io(GAtIO *io)
>  {
>  	GAtHDLC *hdlc;
>  	unsigned char *buf;
> +	struct ring_buffer* write_buffer;
>  
>  	if (io == NULL)
>  		return NULL;
> @@ -207,16 +212,19 @@ GAtHDLC *g_at_hdlc_new_from_io(GAtIO *io)
>  	hdlc->xmit_accm[3] = 0x60000000; /* 0x7d, 0x7e */
>  	hdlc->recv_accm = ~0U;
>  
> -	hdlc->write_buffer = ring_buffer_new(BUFFER_SIZE * 2);
> -	if (!hdlc->write_buffer)
> +	write_buffer = ring_buffer_new(BUFFER_SIZE);
> +	if (!write_buffer)

You change BUFFER_SIZE from 2048 to 4096 and remove the multiplication
here.  It is a good idea to send these types of changes in a separate
patch for two reasons:
	- Logically they should be separate
	- It is much easier to review for correctness outside the context of a
large patch

>  		goto error;
>  
> +	hdlc->wbuf_head = hdlc->wbuf_tail = 0;
> +	hdlc->wbuf_ring[0] = write_buffer;
> +
>  	/* Write an initial 0x7e as wakeup character */
> -	buf = ring_buffer_write_ptr(hdlc->write_buffer, 0);
> +	buf = ring_buffer_write_ptr(write_buffer, 0);
>  	*buf = HDLC_FLAG;
> -	ring_buffer_write_advance(hdlc->write_buffer, 1);
> +	ring_buffer_write_advance(write_buffer, 1);
>  
> -	hdlc->decode_buffer = g_try_malloc(BUFFER_SIZE * 2);
> +	hdlc->decode_buffer = g_try_malloc(BUFFER_SIZE);
>  	if (!hdlc->decode_buffer)
>  		goto error;
>  
> @@ -228,8 +236,8 @@ GAtHDLC *g_at_hdlc_new_from_io(GAtIO *io)
>  	return hdlc;
>  
>  error:
> -	if (hdlc->write_buffer)
> -		ring_buffer_free(hdlc->write_buffer);
> +	if (write_buffer)
> +	    ring_buffer_free(write_buffer);

Please watch out for space / tab indentation mixups.  You have one here

>  
>  	if (hdlc->decode_buffer)
>  		g_free(hdlc->decode_buffer);
> @@ -266,6 +274,8 @@ GAtHDLC *g_at_hdlc_ref(GAtHDLC *hdlc)
>  
>  void g_at_hdlc_unref(GAtHDLC *hdlc)
>  {
> +	int i;
> +
>  	if (hdlc == NULL)
>  		return;
>  
> @@ -280,7 +290,11 @@ void g_at_hdlc_unref(GAtHDLC *hdlc)
>  	g_at_io_unref(hdlc->io);
>  	hdlc->io = NULL;
>  
> -	ring_buffer_free(hdlc->write_buffer);
> +	for (i = hdlc->wbuf_head; i != hdlc->wbuf_tail; i = (i+1) % MAX_BUFFERS)
> +	    ring_buffer_free(hdlc->wbuf_ring[i]);

And here as well

> +
> +	ring_buffer_free(hdlc->wbuf_ring[i]);
> +
>  	g_free(hdlc->decode_buffer);
>  
>  	if (hdlc->in_read_handler)
> @@ -314,17 +328,26 @@ static gboolean can_write_data(gpointer data)
>  	unsigned int len;
>  	unsigned char *buf;
>  	gsize bytes_written;
> +	struct ring_buffer* write_buffer = hdlc->wbuf_ring[hdlc->wbuf_head];
>  
> -	len = ring_buffer_len_no_wrap(hdlc->write_buffer);
> -	buf = ring_buffer_read_ptr(hdlc->write_buffer, 0);
> +	len = ring_buffer_len_no_wrap(write_buffer);
> +	buf = ring_buffer_read_ptr(write_buffer, 0);
>  
>  	bytes_written = g_at_io_write(hdlc->io, (gchar *) buf, len);
>  	hdlc_record(hdlc->record_fd, FALSE, buf, bytes_written);
> -	ring_buffer_drain(hdlc->write_buffer, bytes_written);
> +	ring_buffer_drain(write_buffer, bytes_written);
>  
> -	if (ring_buffer_len(hdlc->write_buffer) > 0)
> +	if (ring_buffer_len(write_buffer) > 0)
>  		return TRUE;
>  
> +	/* We're done with this buffer : adjust head if there is at least another buffer in flight */
> +	if (hdlc->wbuf_head != hdlc->wbuf_tail) {
> +		ring_buffer_free(write_buffer);
> +		hdlc->wbuf_head = (hdlc->wbuf_head + 1) % MAX_BUFFERS;
> +
> +		return TRUE;
> +	}
> +
>  	return FALSE;
>  }
>  
> @@ -356,19 +379,37 @@ GAtIO *g_at_hdlc_get_io(GAtHDLC *hdlc)
>  
>  gboolean g_at_hdlc_send(GAtHDLC *hdlc, const unsigned char *data, gsize size)
>  {
> -	unsigned int avail = ring_buffer_avail(hdlc->write_buffer);
> -	unsigned int wrap = ring_buffer_avail_no_wrap(hdlc->write_buffer);
> -	unsigned char *buf = ring_buffer_write_ptr(hdlc->write_buffer, 0);
> +	struct ring_buffer* write_buffer = hdlc->wbuf_ring[hdlc->wbuf_tail];
> +	unsigned int avail = ring_buffer_avail(write_buffer);
> +	unsigned int wrap = ring_buffer_avail_no_wrap(write_buffer);
> +	unsigned char *buf;
>  	unsigned char tail[2];
>  	unsigned int i = 0;
>  	guint16 fcs = HDLC_INITFCS;
>  	gboolean escape = FALSE;
>  	gsize pos = 0;
>  
> -	if (avail < size)
> -		return FALSE;
> +	if (avail < size + HDLC_OVERHEAD) {

So I think we have to be a bit careful here.  HDLC framing can in theory
(if you're maximally unlucky) result in doubling of the data size once
it is framed.  This means that we might have enough space in the current
buffer according to this estimate, but still exceed it once the actual
framing is performed.  If so, then we have to drop the frame.

There are two possibilities:
	- Retry again with a full buffer this time
	- Always pick a buffer if we have less than 2x size available

> +		/* Switch to a new buffer */
> +
> +		if ((hdlc->wbuf_tail + 1) % MAX_BUFFERS == hdlc->wbuf_head)
> +			return FALSE;	/* Ring full */
> +
> +		write_buffer = ring_buffer_new(BUFFER_SIZE);
> +
> +		if (!write_buffer)
> +			return FALSE;
> +
> +		hdlc->wbuf_tail = (hdlc->wbuf_tail + 1) % MAX_BUFFERS;
> +		hdlc->wbuf_ring[hdlc->wbuf_tail] = write_buffer;
> +
> +		avail = ring_buffer_avail(write_buffer);
> +		wrap = ring_buffer_avail_no_wrap(write_buffer);
> +		buf = ring_buffer_write_ptr(write_buffer, 0);
> +	}
>  
>  	i = 0;
> +	buf = ring_buffer_write_ptr(write_buffer, 0);
>  
>  	while (pos < avail && i < size) {
>  		if (escape == TRUE) {
> @@ -387,7 +428,7 @@ gboolean g_at_hdlc_send(GAtHDLC *hdlc, const unsigned char *data, gsize size)
>  		pos++;
>  
>  		if (pos == wrap)
> -			buf = ring_buffer_write_ptr(hdlc->write_buffer, pos);
> +			buf = ring_buffer_write_ptr(write_buffer, pos);
>  	}
>  
>  	if (i < size)
> @@ -414,7 +455,7 @@ gboolean g_at_hdlc_send(GAtHDLC *hdlc, const unsigned char *data, gsize size)
>  		pos++;
>  
>  		if (pos == wrap)
> -			buf = ring_buffer_write_ptr(hdlc->write_buffer, pos);
> +		    buf = ring_buffer_write_ptr(write_buffer, pos);
>  	}
>  
>  	if (i < sizeof(tail))
> @@ -426,7 +467,7 @@ gboolean g_at_hdlc_send(GAtHDLC *hdlc, const unsigned char *data, gsize size)
>  	*buf = HDLC_FLAG;
>  	pos++;
>  
> -	ring_buffer_write_advance(hdlc->write_buffer, pos);
> +	ring_buffer_write_advance(write_buffer, pos);
>  
>  	g_at_io_set_write_handler(hdlc->io, can_write_data, hdlc);
>  
> diff --git a/gatchat/ringbuffer.c b/gatchat/ringbuffer.c
> index becd3f8..27be3a8 100644
> --- a/gatchat/ringbuffer.c
> +++ b/gatchat/ringbuffer.c
> @@ -34,6 +34,7 @@
>  struct ring_buffer {
>  	unsigned char *buffer;
>  	unsigned int size;
> +	unsigned int mask;
>  	unsigned int in;
>  	unsigned int out;
>  };
> @@ -61,6 +62,7 @@ struct ring_buffer *ring_buffer_new(unsigned int size)
>  	}
>  
>  	buffer->size = real_size;
> +	buffer->mask = real_size - 1;
>  	buffer->in = 0;
>  	buffer->out = 0;
>  
> @@ -78,7 +80,7 @@ int ring_buffer_write(struct ring_buffer *buf, const void *data,
>  	len = MIN(len, buf->size - buf->in + buf->out);
>  
>  	/* Determine how much to write before wrapping */
> -	offset = buf->in % buf->size;
> +	offset = buf->in & buf->mask;
>  	end = MIN(len, buf->size - offset);
>  	memcpy(buf->buffer+offset, d, end);
>  
> @@ -93,12 +95,12 @@ int ring_buffer_write(struct ring_buffer *buf, const void *data,
>  unsigned char *ring_buffer_write_ptr(struct ring_buffer *buf,
>  					unsigned int offset)
>  {
> -	return buf->buffer + (buf->in + offset) % buf->size;
> +	return buf->buffer + ((buf->in + offset) & buf->mask);
>  }
>  
>  int ring_buffer_avail_no_wrap(struct ring_buffer *buf)
>  {
> -	unsigned int offset = buf->in % buf->size;
> +	unsigned int offset = buf->in & buf->mask;
>  	unsigned int len = buf->size - buf->in + buf->out;
>  
>  	return MIN(len, buf->size - offset);
> @@ -121,7 +123,7 @@ int ring_buffer_read(struct ring_buffer *buf, void *data, unsigned int len)
>  	len = MIN(len, buf->in - buf->out);
>  
>  	/* Grab data from buffer starting at offset until the end */
> -	offset = buf->out % buf->size;
> +	offset = buf->out & buf->mask;
>  	end = MIN(len, buf->size - offset);
>  	memcpy(d, buf->buffer + offset, end);
>  
> @@ -150,7 +152,7 @@ int ring_buffer_drain(struct ring_buffer *buf, unsigned int len)
>  
>  int ring_buffer_len_no_wrap(struct ring_buffer *buf)
>  {
> -	unsigned int offset = buf->out % buf->size;
> +	unsigned int offset = buf->out & buf->mask;
>  	unsigned int len = buf->in - buf->out;
>  
>  	return MIN(len, buf->size - offset);
> @@ -159,7 +161,7 @@ int ring_buffer_len_no_wrap(struct ring_buffer *buf)
>  unsigned char *ring_buffer_read_ptr(struct ring_buffer *buf,
>  					unsigned int offset)
>  {
> -	return buf->buffer + (buf->out + offset) % buf->size;
> +	return buf->buffer + ((buf->out + offset) & buf->mask);
>  }
>  
>  int ring_buffer_len(struct ring_buffer *buf)

Regards,
-Denis

Re: [PATCH] PPP: Optimize write buffer management

[Patrick Porlan]

[-- Attachment #1: Type: text/plain, Size: 2207 bytes --]

Hi Denis,

On Tue, 2011-03-01 at 21:47 -0600, Denis Kenzior wrote:
> This is a great description, but right after reading it one should
> realize that this patch is better broken down into two.  The first patch
> addressing the ringbuffer performance improvements and the second one
> dealing with PPP/HDLC queuing.  In fact, if there were two patches, then
> ringbuffer changes could go in right away.

Fair enough ; I'll separate the changes.


> Please try to avoid going over 80 characters a line.  Sometimes it is
> unavoidable, but lets try very hard to avoid it.

Understood.


> Is there a particular reason why you chose to use a ring buffer of ring
> buffers? A simple GQueue might be much easier to understand.  If you are
> worried about 'infinite queuing' then a simple counter might help to
> alleviate that.

Well, this implementation opens the door to further optimizations, such
as reusing cells (buffers) to avoid frequent allocation/deallocation.


> You change BUFFER_SIZE from 2048 to 4096 and remove the multiplication
> here.  It is a good idea to send these types of changes in a separate
> patch for two reasons:
> 	- Logically they should be separate
> 	- It is much easier to review for correctness outside the context of a
> large patch

Got it. By the way, why is it written this way? Is it to stress that we
want space for two frames?


> Please watch out for space / tab indentation mixups. 

OK. I'll fix that.


> So I think we have to be a bit careful here.  HDLC framing can in theory
> (if you're maximally unlucky) result in doubling of the data size once
> it is framed.  This means that we might have enough space in the current
> buffer according to this estimate, but still exceed it once the actual
> framing is performed.  If so, then we have to drop the frame.
> 
> There are two possibilities:
> 	- Retry again with a full buffer this time
> 	- Always pick a buffer if we have less than 2x size available
> 

Thanks for the insight. Picking a new buffer may be best. That will
cause more buffer "run away" though, strengthening the case for better
buffer recycling strategies.

Regards,

-- Patrick

patch to fix BMC #13679 Huawei EM770: ofonod crash when disable/enable 3G technology

[Xu, Martin]

[-- Attachment #1: Type: text/plain, Size: 120 bytes --]

Looks like my smtp server has some issue currently. 
So send the patch through attachment.
Sorry for inconvenient.


[-- Attachment #2: 0001-Fix-huawei_disconnect-function-issue.patch --]
[-- Type: application/octet-stream, Size: 4960 bytes --]

From db83d90a1e48d99e2d83afa5db21cefda3427014 Mon Sep 17 00:00:00 2001
From: Martin Xu <martin.xu@intel.com>
Date: Wed, 2 Mar 2011 16:41:59 +0800
Subject: [PATCH] Fix huawei_disconnect function issue

Close the chat initiatively, huawei_disconnect should not be called
to recreate chat and gprs context. And the function needs to check modem
online state before recreate gprs context.

also refer commit 39382730d7758b093ca6271f4e9dea875fa04b3a
---
 plugins/huawei.c |   66 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/plugins/huawei.c b/plugins/huawei.c
index 6f05677..5e2ee90 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -78,6 +78,7 @@ struct huawei_data {
 	struct ofono_gprs *gprs;
 	struct ofono_gprs_context *gc;
 	gboolean voice;
+	gboolean online;
 	gboolean ndis;
 	guint sim_poll_timeout;
 	guint sim_poll_count;
@@ -97,6 +98,7 @@ static int huawei_probe(struct ofono_modem *modem)
 	if (data == NULL)
 		return -ENOMEM;
 
+	data->online = FALSE;
 	ofono_modem_set_data(modem, data);
 
 	return 0;
@@ -111,6 +113,11 @@ static void huawei_remove(struct ofono_modem *modem)
 	ofono_modem_set_data(modem, NULL);
 
 	if (data->modem)
+		/*
+		 * huawei_disconnect should not be called when the chat
+		 * is closed initiatively
+		 */
+		g_at_chat_set_disconnect_function(data->modem, NULL, NULL);
 		g_at_chat_unref(data->modem);
 
 	g_at_chat_unref(data->pcui);
@@ -470,10 +477,7 @@ static void huawei_disconnect(gpointer user_data)
 	struct ofono_modem *modem = user_data;
 	struct huawei_data *data = ofono_modem_get_data(modem);
 
-	DBG("");
-
-	if (data->gc)
-		ofono_gprs_context_remove(data->gc);
+	DBG("data->online %d", data->online);
 
 	g_at_chat_unref(data->modem);
 	data->modem = NULL;
@@ -485,6 +489,13 @@ static void huawei_disconnect(gpointer user_data)
 	g_at_chat_set_disconnect_function(data->modem,
 						huawei_disconnect, modem);
 
+	/* reopen GPRS context channel only at online state */
+	if (data->online == FALSE)
+		return;
+
+	if (data->gc)
+		ofono_gprs_context_remove(data->gc);
+
 	if (data->sim_state == HUAWEI_SIM_STATE_VALID ||
 			data->sim_state == HUAWEI_SIM_STATE_INVALID_CS) {
 		ofono_info("Reopened GPRS context channel");
@@ -515,6 +526,11 @@ static int huawei_enable(struct ofono_modem *modem)
 
 	data->pcui = open_device(modem, "Pcui", "PCUI: ");
 	if (data->pcui == NULL) {
+		/*
+		 * huawei_disconnect should not be called when the chat
+		 * is closed initiatively
+		 */
+		g_at_chat_set_disconnect_function(data->modem, NULL, NULL);
 		g_at_chat_unref(data->modem);
 		data->modem = NULL;
 		return -EIO;
@@ -564,6 +580,11 @@ static int huawei_disable(struct ofono_modem *modem)
 	if (data->modem) {
 		g_at_chat_cancel_all(data->modem);
 		g_at_chat_unregister_all(data->modem);
+		/*
+		 * huawei_disconnect should not be called when the chat
+		 * is closed initiatively
+		 */
+		g_at_chat_set_disconnect_function(data->modem, NULL, NULL);
 		g_at_chat_unref(data->modem);
 		data->modem = NULL;
 	}
@@ -579,15 +600,27 @@ static int huawei_disable(struct ofono_modem *modem)
 	return -EINPROGRESS;
 }
 
+struct huawei_cb_data {
+	struct cb_data *cbd;
+	ofono_bool_t online;
+	struct ofono_modem *modem;
+};
+
 static void set_online_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
-	struct cb_data *cbd = user_data;
-	ofono_modem_online_cb_t cb = cbd->cb;
+	struct huawei_cb_data *online_cbd = user_data;
+	ofono_modem_online_cb_t cb = online_cbd->cbd->cb;
+	struct huawei_data *data = ofono_modem_get_data(online_cbd->modem);
 
-	if (ok)
-		CALLBACK_WITH_SUCCESS(cb, cbd->data);
+	if (ok) {
+		data->online = online_cbd->online;
+
+		CALLBACK_WITH_SUCCESS(cb, online_cbd->cbd->data);
+
+		g_free(online_cbd);
+	}
 	else
-		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		CALLBACK_WITH_FAILURE(cb, online_cbd->cbd->data);
 }
 
 static void huawei_set_online(struct ofono_modem *modem, ofono_bool_t online,
@@ -595,21 +628,26 @@ static void huawei_set_online(struct ofono_modem *modem, ofono_bool_t online,
 {
 	struct huawei_data *data = ofono_modem_get_data(modem);
 	GAtChat *chat = data->pcui;
-	struct cb_data *cbd = cb_data_new(cb, user_data);
+	struct huawei_cb_data *online_cbd;
 	char const *command = online ? "AT+CFUN=1" : "AT+CFUN=5";
 
 	DBG("modem %p %s", modem, online ? "online" : "offline");
 
-	if (cbd == NULL)
+	online_cbd = g_try_new0(struct huawei_cb_data, 1);
+	online_cbd->cbd = cb_data_new(cb, user_data);
+	if (online_cbd->cbd == NULL)
 		goto error;
+	online_cbd->online = online;
+	online_cbd->modem = modem;
 
-	if (g_at_chat_send(chat, command, NULL, set_online_cb, cbd, g_free))
+	if (g_at_chat_send(chat, command, NULL, set_online_cb,
+						online_cbd, g_free))
 		return;
 
 error:
-	g_free(cbd);
+	g_free(online_cbd);
 
-	CALLBACK_WITH_FAILURE(cb, cbd->data);
+	CALLBACK_WITH_FAILURE(cb, user_data);
 }
 
 static void huawei_pre_sim(struct ofono_modem *modem)
-- 
1.6.1.3

Re: [PATCH] PPP: Optimize write buffer management

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2139 bytes --]

Hi Patrick,

>> Is there a particular reason why you chose to use a ring buffer of ring
>> buffers? A simple GQueue might be much easier to understand.  If you are
>> worried about 'infinite queuing' then a simple counter might help to
>> alleviate that.
> 
> Well, this implementation opens the door to further optimizations, such
> as reusing cells (buffers) to avoid frequent allocation/deallocation.
> 

I think I know where you're going with this, but I'm unconvinced the
circular buffer of buffers is buying you anything over the much simpler
to understand queue of buffers approach.

> 
>> You change BUFFER_SIZE from 2048 to 4096 and remove the multiplication
>> here.  It is a good idea to send these types of changes in a separate
>> patch for two reasons:
>> 	- Logically they should be separate
>> 	- It is much easier to review for correctness outside the context of a
>> large patch
> 
> Got it. By the way, why is it written this way? Is it to stress that we
> want space for two frames?
>

You'd have to ask Marcel, but yes I think this was the intent.

>> So I think we have to be a bit careful here.  HDLC framing can in theory
>> (if you're maximally unlucky) result in doubling of the data size once
>> it is framed.  This means that we might have enough space in the current
>> buffer according to this estimate, but still exceed it once the actual
>> framing is performed.  If so, then we have to drop the frame.
>>
>> There are two possibilities:
>> 	- Retry again with a full buffer this time
>> 	- Always pick a buffer if we have less than 2x size available
>>
> 
> Thanks for the insight. Picking a new buffer may be best. That will
> cause more buffer "run away" though, strengthening the case for better
> buffer recycling strategies.
> 

Yes, we definitely want to have some sort of buffer pool management
strategy to share a pool of buffers between different entities (e.g.
GAtMux, GAtServer, GAtHDLC).  Have you looked at g_slice* inside glib
yet?  I think we can take advantage of the fact that most of our ring
buffers are sized at 4k.

Regards,
-Denis

Re: patch to fix BMC #13679 Huawei EM770: ofonod crash when disable/enable 3G technology

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 5616 bytes --]

Hi Martin,

On 03/02/2011 03:59 AM, Xu, Martin wrote:
> Looks like my smtp server has some issue currently. 
> So send the patch through attachment.
> Sorry for inconvenient.
> 

> diff --git a/plugins/huawei.c b/plugins/huawei.c
> index 6f05677..5e2ee90 100644
> --- a/plugins/huawei.c
> +++ b/plugins/huawei.c
> @@ -78,6 +78,7 @@ struct huawei_data {
>  	struct ofono_gprs *gprs;
>  	struct ofono_gprs_context *gc;
>  	gboolean voice;
> +	gboolean online;

I don't think you really need this one, you can use the presence of gprs
atom instead.

>  	gboolean ndis;
>  	guint sim_poll_timeout;
>  	guint sim_poll_count;
> @@ -97,6 +98,7 @@ static int huawei_probe(struct ofono_modem *modem)
>  	if (data == NULL)
>  		return -ENOMEM;
>  
> +	data->online = FALSE;
>  	ofono_modem_set_data(modem, data);
>  
>  	return 0;
> @@ -111,6 +113,11 @@ static void huawei_remove(struct ofono_modem *modem)
>  	ofono_modem_set_data(modem, NULL);
>  
>  	if (data->modem)
> +		/*
> +		 * huawei_disconnect should not be called when the chat
> +		 * is closed initiatively
> +		 */
> +		g_at_chat_set_disconnect_function(data->modem, NULL, NULL);
>  		g_at_chat_unref(data->modem);

Are you missing some parentheses around the if statement now?

Setting the disconnect function here seems fishy.  We automatically set
it to NULL when GAtIO is unref-ed.  Seems to me g_at_chat_unref should
be enough.

>  
>  	g_at_chat_unref(data->pcui);
> @@ -470,10 +477,7 @@ static void huawei_disconnect(gpointer user_data)
>  	struct ofono_modem *modem = user_data;
>  	struct huawei_data *data = ofono_modem_get_data(modem);
>  
> -	DBG("");
> -
> -	if (data->gc)
> -		ofono_gprs_context_remove(data->gc);
> +	DBG("data->online %d", data->online);
>  
>  	g_at_chat_unref(data->modem);
>  	data->modem = NULL;
> @@ -485,6 +489,13 @@ static void huawei_disconnect(gpointer user_data)
>  	g_at_chat_set_disconnect_function(data->modem,
>  						huawei_disconnect, modem);
>  
> +	/* reopen GPRS context channel only at online state */
> +	if (data->online == FALSE)
> +		return;
> +
> +	if (data->gc)
> +		ofono_gprs_context_remove(data->gc);
> +
>  	if (data->sim_state == HUAWEI_SIM_STATE_VALID ||
>  			data->sim_state == HUAWEI_SIM_STATE_INVALID_CS) {
>  		ofono_info("Reopened GPRS context channel");
> @@ -515,6 +526,11 @@ static int huawei_enable(struct ofono_modem *modem)
>  
>  	data->pcui = open_device(modem, "Pcui", "PCUI: ");
>  	if (data->pcui == NULL) {
> +		/*
> +		 * huawei_disconnect should not be called when the chat
> +		 * is closed initiatively
> +		 */
> +		g_at_chat_set_disconnect_function(data->modem, NULL, NULL);
>  		g_at_chat_unref(data->modem);
>  		data->modem = NULL;
>  		return -EIO;
> @@ -564,6 +580,11 @@ static int huawei_disable(struct ofono_modem *modem)
>  	if (data->modem) {
>  		g_at_chat_cancel_all(data->modem);
>  		g_at_chat_unregister_all(data->modem);
> +		/*
> +		 * huawei_disconnect should not be called when the chat
> +		 * is closed initiatively
> +		 */
> +		g_at_chat_set_disconnect_function(data->modem, NULL, NULL);
>  		g_at_chat_unref(data->modem);
>  		data->modem = NULL;
>  	}
> @@ -579,15 +600,27 @@ static int huawei_disable(struct ofono_modem *modem)
>  	return -EINPROGRESS;
>  }
>  
> +struct huawei_cb_data {
> +	struct cb_data *cbd;
> +	ofono_bool_t online;
> +	struct ofono_modem *modem;
> +};
> +
>  static void set_online_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  {
> -	struct cb_data *cbd = user_data;
> -	ofono_modem_online_cb_t cb = cbd->cb;
> +	struct huawei_cb_data *online_cbd = user_data;
> +	ofono_modem_online_cb_t cb = online_cbd->cbd->cb;
> +	struct huawei_data *data = ofono_modem_get_data(online_cbd->modem);
>  
> -	if (ok)
> -		CALLBACK_WITH_SUCCESS(cb, cbd->data);
> +	if (ok) {
> +		data->online = online_cbd->online;
> +
> +		CALLBACK_WITH_SUCCESS(cb, online_cbd->cbd->data);
> +
> +		g_free(online_cbd);
> +	}
>  	else
> -		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		CALLBACK_WITH_FAILURE(cb, online_cbd->cbd->data);
>  }

All of this can be better accomplished by using two separate callbacks.
 One for online and one for offline.

I suspect all you need to do is unref data->modem, set data->gc and
data->gprs to NULL in the offline callback prior to calling back into
the core.

>  
>  static void huawei_set_online(struct ofono_modem *modem, ofono_bool_t online,
> @@ -595,21 +628,26 @@ static void huawei_set_online(struct ofono_modem *modem, ofono_bool_t online,
>  {
>  	struct huawei_data *data = ofono_modem_get_data(modem);
>  	GAtChat *chat = data->pcui;
> -	struct cb_data *cbd = cb_data_new(cb, user_data);
> +	struct huawei_cb_data *online_cbd;
>  	char const *command = online ? "AT+CFUN=1" : "AT+CFUN=5";
>  
>  	DBG("modem %p %s", modem, online ? "online" : "offline");
>  
> -	if (cbd == NULL)
> +	online_cbd = g_try_new0(struct huawei_cb_data, 1);
> +	online_cbd->cbd = cb_data_new(cb, user_data);
> +	if (online_cbd->cbd == NULL)
>  		goto error;
> +	online_cbd->online = online;
> +	online_cbd->modem = modem;
>  
> -	if (g_at_chat_send(chat, command, NULL, set_online_cb, cbd, g_free))
> +	if (g_at_chat_send(chat, command, NULL, set_online_cb,
> +						online_cbd, g_free))
>  		return;
>  
>  error:
> -	g_free(cbd);
> +	g_free(online_cbd);
>  
> -	CALLBACK_WITH_FAILURE(cb, cbd->data);
> +	CALLBACK_WITH_FAILURE(cb, user_data);
>  }
>  
>  static void huawei_pre_sim(struct ofono_modem *modem)
> -- 1.6.1.3 

Regards,
-Denis

RE: patch to fix BMC #13679 Huawei EM770: ofonod crash when disable/enable 3G technology

[Xu, Martin]

[-- Attachment #1: Type: text/plain, Size: 4803 bytes --]

Hi Denis:

> >  	struct ofono_gprs *gprs;
> >  	struct ofono_gprs_context *gc;
> >  	gboolean voice;
> > +	gboolean online;
> 
> I don't think you really need this one, you can use the presence of gprs
> atom instead.
Gprs atom is not reliable here. Because gprs will be flushed when entering offline mode
> 
> >  	gboolean ndis;
> >  	guint sim_poll_timeout;
> >  	guint sim_poll_count;
> > @@ -97,6 +98,7 @@ static int huawei_probe(struct ofono_modem *modem)
> >  	if (data == NULL)
> >  		return -ENOMEM;
> >
> > +	data->online = FALSE;
> >  	ofono_modem_set_data(modem, data);
> >
> >  	return 0;
> > @@ -111,6 +113,11 @@ static void huawei_remove(struct ofono_modem
> *modem)
> >  	ofono_modem_set_data(modem, NULL);
> >
> >  	if (data->modem)
> > +		/*
> > +		 * huawei_disconnect should not be called when the chat
> > +		 * is closed initiatively
> > +		 */
> > +		g_at_chat_set_disconnect_function(data->modem, NULL, NULL);
> >  		g_at_chat_unref(data->modem);
> 
> Are you missing some parentheses around the if statement now?
> 
> Setting the disconnect function here seems fishy.  We automatically set
> it to NULL when GAtIO is unref-ed.  Seems to me g_at_chat_unref should
> be enough.
You are quite right. 
> 
> >
> >  	g_at_chat_unref(data->pcui);
> > @@ -470,10 +477,7 @@ static void huawei_disconnect(gpointer user_data)
> >  	struct ofono_modem *modem = user_data;
> >  	struct huawei_data *data = ofono_modem_get_data(modem);
> >
> > -	DBG("");
> > -
> > -	if (data->gc)
> > -		ofono_gprs_context_remove(data->gc);
> > +	DBG("data->online %d", data->online);
> >
> >  	g_at_chat_unref(data->modem);
> >  	data->modem = NULL;
> > @@ -485,6 +489,13 @@ static void huawei_disconnect(gpointer user_data)
> >  	g_at_chat_set_disconnect_function(data->modem,
> >  						huawei_disconnect, modem);
> >
> > +	/* reopen GPRS context channel only at online state */
> > +	if (data->online == FALSE)
> > +		return;
> > +
> > +	if (data->gc)
> > +		ofono_gprs_context_remove(data->gc);
> > +
> >  	if (data->sim_state == HUAWEI_SIM_STATE_VALID ||
> >  			data->sim_state == HUAWEI_SIM_STATE_INVALID_CS) {
> >  		ofono_info("Reopened GPRS context channel");
> > @@ -515,6 +526,11 @@ static int huawei_enable(struct ofono_modem
> *modem)
> >
> >  	data->pcui = open_device(modem, "Pcui", "PCUI: ");
> >  	if (data->pcui == NULL) {
> > +		/*
> > +		 * huawei_disconnect should not be called when the chat
> > +		 * is closed initiatively
> > +		 */
> > +		g_at_chat_set_disconnect_function(data->modem, NULL, NULL);
> >  		g_at_chat_unref(data->modem);
> >  		data->modem = NULL;
> >  		return -EIO;
> > @@ -564,6 +580,11 @@ static int huawei_disable(struct ofono_modem
> *modem)
> >  	if (data->modem) {
> >  		g_at_chat_cancel_all(data->modem);
> >  		g_at_chat_unregister_all(data->modem);
> > +		/*
> > +		 * huawei_disconnect should not be called when the chat
> > +		 * is closed initiatively
> > +		 */
> > +		g_at_chat_set_disconnect_function(data->modem, NULL, NULL);
> >  		g_at_chat_unref(data->modem);
> >  		data->modem = NULL;
> >  	}
> > @@ -579,15 +600,27 @@ static int huawei_disable(struct ofono_modem
> *modem)
> >  	return -EINPROGRESS;
> >  }
> >
> > +struct huawei_cb_data {
> > +	struct cb_data *cbd;
> > +	ofono_bool_t online;
> > +	struct ofono_modem *modem;
> > +};
> > +
> >  static void set_online_cb(gboolean ok, GAtResult *result, gpointer
> user_data)
> >  {
> > -	struct cb_data *cbd = user_data;
> > -	ofono_modem_online_cb_t cb = cbd->cb;
> > +	struct huawei_cb_data *online_cbd = user_data;
> > +	ofono_modem_online_cb_t cb = online_cbd->cbd->cb;
> > +	struct huawei_data *data =
> ofono_modem_get_data(online_cbd->modem);
> >
> > -	if (ok)
> > -		CALLBACK_WITH_SUCCESS(cb, cbd->data);
> > +	if (ok) {
> > +		data->online = online_cbd->online;
> > +
> > +		CALLBACK_WITH_SUCCESS(cb, online_cbd->cbd->data);
> > +
> > +		g_free(online_cbd);
> > +	}
> >  	else
> > -		CALLBACK_WITH_FAILURE(cb, cbd->data);
> > +		CALLBACK_WITH_FAILURE(cb, online_cbd->cbd->data);
> >  }
> 
> All of this can be better accomplished by using two separate callbacks.
>  One for online and one for offline.
agree

> 
> I suspect all you need to do is unref data->modem, set data->gc and
> data->gprs to NULL in the offline callback prior to calling back into
> the core.
Logically, call back only needs to set offline states
It is Huawei_disconnect's duty to decide the operation. 
In theory, Huawei_disconnect may be called at any situation when io error happens.
(I observed two place, one is at ppp_disconnect the other is at offline mode CFUN=5.)
So the function must take action according to the states. 
I will resent the updated patch

Re: patch to fix BMC #13679 Huawei EM770: ofonod crash when disable/enable 3G technology

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]

Hi Martin,

On 03/04/2011 12:53 AM, Xu, Martin wrote:
> Hi Denis:
> 
>>>  	struct ofono_gprs *gprs;
>>>  	struct ofono_gprs_context *gc;
>>>  	gboolean voice;
>>> +	gboolean online;
>>
>> I don't think you really need this one, you can use the presence of gprs
>> atom instead.
> Gprs atom is not reliable here. Because gprs will be flushed when entering offline mode

Both gprs and gprs-contexts will be flushed.  So you should be setting
them to NULL when going offline.  How is this not reliable?

>>
>> I suspect all you need to do is unref data->modem, set data->gc and
>> data->gprs to NULL in the offline callback prior to calling back into
>> the core.
> Logically, call back only needs to set offline states
> It is Huawei_disconnect's duty to decide the operation. 
> In theory, Huawei_disconnect may be called at any situation when io error happens.
> (I observed two place, one is at ppp_disconnect the other is at offline mode CFUN=5.)
> So the function must take action according to the states. 
> I will resent the updated patch

Sure, but what you're trying to prevent is keeping pointers around to
atoms which have gone away.  One easy way to do that would be to simply
set them to NULL when going offline.

You can also install atom watches, but that might not be worth the effort.

Regards,
-Denis

Re: [PATCH] PPP: Optimize write buffer management

[Patrick Porlan]

[-- Attachment #1: Type: text/plain, Size: 1786 bytes --]

Hi Denis,

On Wed, 2011-03-02 at 09:28 -0600, Denis Kenzior wrote:

> >> Is there a particular reason why you chose to use a ring buffer of ring
> >> buffers? A simple GQueue might be much easier to understand.  If you are
> >> worried about 'infinite queuing' then a simple counter might help to
> >> alleviate that.

I just sent a new version of my patch that does it (forgot to tag it as
v2, sorry).


> >> You change BUFFER_SIZE from 2048 to 4096 and remove the multiplication
> >> here.

Having a constant named BUFFER_SIZE whose value is half a buffer size
did not seem right though. I reinstantated the multiplication, but at
the macro definition level.


> >> So I think we have to be a bit careful here.  HDLC framing can in theory
> >> (if you're maximally unlucky) result in doubling of the data size once
> >> it is framed.  This means that we might have enough space in the current
> >> buffer according to this estimate, but still exceed it once the actual
> >> framing is performed.  If so, then we have to drop the frame.

I checked with random data, and get some frames dropped with a 128
margin, but none with 256. Using a much larger margin might be
detrimental to memory usage, so I chose to leave it like this. We can
address this in a further patch if you wish.


> Yes, we definitely want to have some sort of buffer pool management
> strategy to share a pool of buffers between different entities (e.g.
> GAtMux, GAtServer, GAtHDLC).  Have you looked at g_slice* inside glib
> yet?  I think we can take advantage of the fact that most of our ring
> buffers are sized at 4k.

It looks like this allocator should be used in ringbuffer.c. Let's
discuss that and follow up with another patch.

Regards,

-- Patrick