[PATCH 1/4] Add write ring buffer for non-blocking write

[PATCH 1/4] Add write ring buffer for non-blocking write

[Zhenhua Zhang]

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

Use two layers to cache server side response data to client.
1. A fixed-length ring buffer.
2. A list of free ring buffers and a list of empty full ring buffer.

If current ring buffer is full, put it into full buffer list and fetch
a free buffer frome free list.
---
 gatchat/gatserver.c |  111 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 104 insertions(+), 7 deletions(-)

diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c
index bf7e847..6b40084 100644
--- a/gatchat/gatserver.c
+++ b/gatchat/gatserver.c
@@ -32,6 +32,9 @@
 #include "ringbuffer.h"
 #include "gatserver.h"
 
+#define MAX_BUFFER_NUM 5
+/* #define WRITE_SCHEDULER_DEBUG 1 */
+
 enum ParserState {
 	PARSER_STATE_IDLE,
 	PARSER_STATE_A,
@@ -90,17 +93,54 @@ struct _GAtServer {
 	struct v250_settings v250;		/* V.250 command setting */
 	GIOChannel *channel;			/* Server IO */
 	guint read_watch;			/* GSource read id, 0 if none */
+	guint write_watch;			/* GSource write id, 0 if none */
 	guint read_so_far;			/* Number of bytes processed */
 	GAtDisconnectFunc user_disconnect;	/* User disconnect func */
 	gpointer user_disconnect_data;		/* User disconnect data */
 	GAtDebugFunc debugf;			/* Debugging output function */
 	gpointer debug_data;			/* Data to pass to debug func */
 	struct ring_buffer *read_buf;		/* Current read buffer */
+	struct ring_buffer *write_buf;		/* Current write buffer */
+	GSList *full_list;			/* List of full ring buffer */
+	GSList *free_list;			/* List of free ring buffer */
 	guint max_read_attempts;		/* Max reads per select */
 	enum ParserState parser_state;
 	gboolean destroyed;			/* Re-entrancy guard */
 };
 
+static void g_at_server_wakeup_writer(GAtServer *server);
+
+static gboolean alloc_free_list(GAtServer *server)
+{
+	struct ring_buffer *buf;
+	guint i;
+
+	for (i = 0; i < MAX_BUFFER_NUM; i++) {
+#ifdef WRITE_SCHEDULER_DEBUG
+		buf = ring_buffer_new(4);
+#else
+		buf = ring_buffer_new(4096);
+#endif
+		if (!buf)
+			return FALSE;
+
+		server->free_list = g_slist_prepend(server->free_list, buf);
+	}
+
+	return TRUE;
+}
+
+static void send_common(GAtServer *server, const char *buf)
+{
+	gsize avail = ring_buffer_avail(server->write_buf);
+	gsize len = strlen(buf);
+
+	if (avail >= len)
+		ring_buffer_write(server->write_buf, buf, len);
+
+	g_at_server_wakeup_writer(server);
+}
+
 static void g_at_server_send_result(GAtServer *server, GAtServerResult result)
 {
 	struct v250_settings v250 = server->v250;
@@ -108,7 +148,6 @@ static void g_at_server_send_result(GAtServer *server, GAtServerResult result)
 	char buf[1024];
 	char t = v250.s3;
 	char r = v250.s4;
-	gsize wbuf;
 
 	if (v250.quiet)
 		return;
@@ -125,8 +164,7 @@ static void g_at_server_send_result(GAtServer *server, GAtServerResult result)
 	g_at_util_debug_chat(FALSE, buf, strlen(buf),
 				server->debugf, server->debug_data);
 
-	g_io_channel_write(server->channel, (char *) buf, strlen(buf),
-							&wbuf);
+	send_common(server, buf);
 }
 
 static inline gboolean is_at_command_prefix(const char c)
@@ -432,12 +470,35 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 	return TRUE;
 }
 
+static gboolean can_write_data(GIOChannel *channel, GIOCondition cond,
+				gpointer data)
+{
+	return FALSE;
+}
+
+static void free_ring_buffer(struct ring_buffer *buf)
+{
+	ring_buffer_free(buf);
+}
+
 static void g_at_server_cleanup(GAtServer *server)
 {
 	/* Cleanup all received data */
 	ring_buffer_free(server->read_buf);
 	server->read_buf = NULL;
 
+	/* Cleanup pending data to write */
+	ring_buffer_free(server->write_buf);
+	server->write_buf = NULL;
+
+	if (server->full_list)
+		g_slist_foreach(server->full_list, (GFunc)free_ring_buffer,
+					NULL);
+
+	if (server->free_list)
+		g_slist_foreach(server->free_list, (GFunc)free_ring_buffer,
+					NULL);
+
 	server->channel = NULL;
 }
 
@@ -446,8 +507,6 @@ static void read_watcher_destroy_notify(GAtServer *server)
 	g_at_server_cleanup(server);
 	server->read_watch = 0;
 
-	server->channel = NULL;
-
 	if (server->user_disconnect)
 		server->user_disconnect(server->user_disconnect_data);
 
@@ -455,6 +514,23 @@ static void read_watcher_destroy_notify(GAtServer *server)
 		g_free(server);
 }
 
+static void write_watcher_destroy_notify(GAtServer *server)
+{
+	server->write_watch = 0;
+}
+
+static void g_at_server_wakeup_writer(GAtServer *server)
+{
+	if (server->write_watch != 0)
+		return;
+
+	server->write_watch = g_io_add_watch_full(server->channel,
+			G_PRIORITY_DEFAULT,
+			G_IO_OUT | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+			can_write_data, server,
+			(GDestroyNotify)write_watcher_destroy_notify);
+}
+
 static void v250_settings_create(struct v250_settings *v250)
 {
 	v250->s3 = '\r';
@@ -483,11 +559,22 @@ GAtServer *g_at_server_new(GIOChannel *io)
 	v250_settings_create(&server->v250);
 	server->channel = io;
 	server->read_buf = ring_buffer_new(4096);
-	server->max_read_attempts = 3;
-
 	if (!server->read_buf)
 		goto error;
 
+#ifdef WRITE_SCHEDULER_DEBUG
+	server->write_buf = ring_buffer_new(4);
+#else
+	server->write_buf = ring_buffer_new(4096);
+#endif
+	if (!server->write_buf)
+		goto error;
+
+	if (!alloc_free_list(server))
+		goto error;
+
+	server->max_read_attempts = 3;
+
 	if (!g_at_util_setup_io(server->channel, G_IO_FLAG_NONBLOCK))
 		goto error;
 
@@ -502,6 +589,13 @@ error:
 	if (server->read_buf)
 		ring_buffer_free(server->read_buf);
 
+	if (server->write_buf)
+		ring_buffer_free(server->write_buf);
+
+	if (server->free_list)
+		g_slist_foreach(server->free_list, (GFunc)free_ring_buffer,
+					NULL);
+
 	if (server)
 		g_free(server);
 
@@ -552,6 +646,9 @@ gboolean g_at_server_shutdown(GAtServer *server)
 	server->user_disconnect = NULL;
 	server->user_disconnect_data = NULL;
 
+	if (server->write_watch)
+		g_source_remove(server->write_watch);
+
 	if (server->read_watch)
 		g_source_remove(server->read_watch);
 
-- 
1.6.6.1

[PATCH 2/4] Add handle the case when write buffer is full

[Zhenhua Zhang]

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

If current write_buf is fulled, put it into the full list and fetch
a new ring buffer from the free list.
---
 gatchat/gatserver.c |   63 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c
index 6b40084..b5ac91e 100644
--- a/gatchat/gatserver.c
+++ b/gatchat/gatserver.c
@@ -130,13 +130,70 @@ static gboolean alloc_free_list(GAtServer *server)
 	return TRUE;
 }
 
+static gboolean replace_write_buf(GAtServer *server)
+{
+	struct ring_buffer *free_buf;
+
+	/* Add current buffer into full list and replace current buffer
+	 * by a new free buffer */
+	server->full_list = g_slist_append(server->full_list,
+						server->write_buf);
+
+	/* All free lists are exhausted. Allocate more free lists */
+	if (!server->free_list) {
+		if (!alloc_free_list(server)) {
+			/* Failed so shutdown the socket */
+			g_at_server_shutdown(server);
+
+			return FALSE;
+		}
+	}
+
+	free_buf = server->free_list->data;
+
+	server->free_list = g_slist_remove(server->free_list, free_buf);
+
+	server->write_buf = free_buf;
+
+	return TRUE;
+}
+
 static void send_common(GAtServer *server, const char *buf)
 {
 	gsize avail = ring_buffer_avail(server->write_buf);
-	gsize len = strlen(buf);
+	gsize towrite = strlen(buf);
+	gsize bytes_written;
+	gsize offset = 0;
+
+	if (avail > towrite) {
+		ring_buffer_write(server->write_buf, buf, towrite);
 
-	if (avail >= len)
-		ring_buffer_write(server->write_buf, buf, len);
+		g_at_server_wakeup_writer(server);
+
+		return;
+	}
+
+	/* Write as much as we can */
+	bytes_written = ring_buffer_write(server->write_buf, buf, avail);
+	towrite -= bytes_written;
+	offset = bytes_written;
+
+	/* If current write buf is full, replace it with next free buffer */
+	if (!replace_write_buf(server))
+		return;
+
+	bytes_written = ring_buffer_write(server->write_buf, buf + offset,
+						towrite);
+	while (bytes_written < towrite) {
+		/* The next free buf is full, replace with next free one */
+		if (!replace_write_buf(server))
+			return;
+
+		towrite -= bytes_written;
+		offset += bytes_written;
+		bytes_written = ring_buffer_write(server->write_buf,
+							buf + offset, towrite);
+	}
 
 	g_at_server_wakeup_writer(server);
 }
-- 
1.6.6.1

[PATCH 3/4] Add ring_buffer_read_advance for partial write

[Zhenhua Zhang]

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

These two patches are for server side partial write.

When server writes data into non-blocking IO, we first get the
read pointer of write_buf and write data as much as we can. Then
we advance the read_ptr according to the real bytes written.
---
 gatchat/ringbuffer.c |    8 ++++++++
 gatchat/ringbuffer.h |    7 +++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/gatchat/ringbuffer.c b/gatchat/ringbuffer.c
index 42d5b68..e17c1fa 100644
--- a/gatchat/ringbuffer.c
+++ b/gatchat/ringbuffer.c
@@ -130,6 +130,14 @@ int ring_buffer_read(struct ring_buffer *buf, void *data, unsigned int len)
 	return len;
 }
 
+int ring_buffer_read_advance(struct ring_buffer *buf, unsigned int len)
+{
+	len = MIN(len, buf->in - buf->out);
+	buf->out += len;
+
+	return len;
+}
+
 int ring_buffer_drain(struct ring_buffer *buf, unsigned int len)
 {
 	len = MIN(len, buf->in - buf->out);
diff --git a/gatchat/ringbuffer.h b/gatchat/ringbuffer.h
index 826f9aa..aa4f570 100644
--- a/gatchat/ringbuffer.h
+++ b/gatchat/ringbuffer.h
@@ -93,6 +93,13 @@ int ring_buffer_read(struct ring_buffer *buf, void *data,
 			unsigned int len);
 
 /*!
+ * Advances the read counter by len, this is meant to be used with
+ * the ring_buffer_read_ptr function.  Returns the number of bytes
+ * actually advanced (the capacity of the buffer)
+ */
+int ring_buffer_read_advance(struct ring_buffer *buf, unsigned int len);
+
+/*!
  * Returns the read pointer with read offset specified by offset.  No bounds
  * checking is performed.  Be careful not to read past the end of the buffer.
  * Use the ring_buffer_len_no_wrap function, and ring_buffer_drain.
-- 
1.6.6.1

[PATCH 4/4] Add write server response into non blocking IO

[Zhenhua Zhang]

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

This and previous patch are for writing server response into
non-blocking GIOChannel when we have G_IO_OUT signal.
---
 gatchat/gatserver.c |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c
index b5ac91e..1b4ce78 100644
--- a/gatchat/gatserver.c
+++ b/gatchat/gatserver.c
@@ -530,6 +530,73 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 static gboolean can_write_data(GIOChannel *channel, GIOCondition cond,
 				gpointer data)
 {
+	GAtServer *server = data;
+	GIOError err;
+	gsize bytes_written;
+	gsize towrite;
+	struct ring_buffer *write_buf;
+	unsigned char *buf;
+	gboolean write_again = FALSE;
+#ifdef WRITE_SCHEDULER_DEBUG
+	int limiter;
+#endif
+
+	if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR))
+		return FALSE;
+
+	/* If we have full buffer, write them out first */
+	if (server->full_list)
+		write_buf = server->full_list->data;
+	else
+		write_buf = server->write_buf;
+
+	buf = ring_buffer_read_ptr(write_buf, 0);
+
+	towrite = ring_buffer_len_no_wrap(write_buf);
+	if (towrite < (gsize)ring_buffer_len(write_buf))
+		write_again = TRUE;
+
+#ifdef WRITE_SCHEDULER_DEBUG
+	limiter = towrite;
+
+	if (limiter > 5)
+		limiter = 5;
+#endif
+
+	err = g_io_channel_write(server->channel,
+			(char *)buf,
+#ifdef WRITE_SCHEDULER_DEBUG
+			limiter,
+#else
+			towrite,
+#endif
+			&bytes_written);
+
+	if (err != G_IO_ERROR_NONE) {
+		g_source_remove(server->read_watch);
+		return FALSE;
+	}
+
+	g_at_util_debug_chat(FALSE, (char *)buf, bytes_written, server->debugf,
+				server->debug_data);
+
+	ring_buffer_read_advance(write_buf, bytes_written);
+
+	if (server->full_list) {
+		/* All data in current full list is written, free it */
+		if (ring_buffer_len(write_buf) == 0) {
+			server->full_list = g_slist_remove(server->full_list,
+								write_buf);
+
+			ring_buffer_free(write_buf);
+		}
+
+		return TRUE;
+	}
+
+	if (bytes_written < towrite || write_again == TRUE)
+		return TRUE;
+
 	return FALSE;
 }
 
-- 
1.6.6.1

Re: [PATCH 3/4] Add ring_buffer_read_advance for partial write

[Denis Kenzior]

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

Hi Zhenhua,

> These two patches are for server side partial write.
> 
> When server writes data into non-blocking IO, we first get the
> read pointer of write_buf and write data as much as we can. Then
> we advance the read_ptr according to the real bytes written.
> ---
>  gatchat/ringbuffer.c |    8 ++++++++
>  gatchat/ringbuffer.h |    7 +++++++
>  2 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/gatchat/ringbuffer.c b/gatchat/ringbuffer.c
> index 42d5b68..e17c1fa 100644
> --- a/gatchat/ringbuffer.c
> +++ b/gatchat/ringbuffer.c
> @@ -130,6 +130,14 @@ int ring_buffer_read(struct ring_buffer *buf, void
>  *data, unsigned int len) return len;
>  }
> 
> +int ring_buffer_read_advance(struct ring_buffer *buf, unsigned int len)
> +{
> +	len = MIN(len, buf->in - buf->out);
> +	buf->out += len;
> +
> +	return len;
> +}
> +

ring_buffer_drain already does this, that is the one you want to use.

Regards,
-Denis

Re: [PATCH 1/4] Add write ring buffer for non-blocking write

[Gustavo F. Padovan]

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

* Zhenhua Zhang <zhenhua.zhang@intel.com> [2010-02-10 16:13:40 +0800]:

> Use two layers to cache server side response data to client.
> 1. A fixed-length ring buffer.
> 2. A list of free ring buffers and a list of empty full ring buffer.
> 
> If current ring buffer is full, put it into full buffer list and fetch
> a free buffer frome free list.
> ---
>  gatchat/gatserver.c |  111 +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 104 insertions(+), 7 deletions(-)
> 
> diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c
> index bf7e847..6b40084 100644
> --- a/gatchat/gatserver.c
> +++ b/gatchat/gatserver.c
> @@ -32,6 +32,9 @@
>  #include "ringbuffer.h"
>  #include "gatserver.h"
>  
> +#define MAX_BUFFER_NUM 5
> +/* #define WRITE_SCHEDULER_DEBUG 1 */
> +
>  enum ParserState {
>  	PARSER_STATE_IDLE,
>  	PARSER_STATE_A,
> @@ -90,17 +93,54 @@ struct _GAtServer {
>  	struct v250_settings v250;		/* V.250 command setting */
>  	GIOChannel *channel;			/* Server IO */
>  	guint read_watch;			/* GSource read id, 0 if none */
> +	guint write_watch;			/* GSource write id, 0 if none */
>  	guint read_so_far;			/* Number of bytes processed */
>  	GAtDisconnectFunc user_disconnect;	/* User disconnect func */
>  	gpointer user_disconnect_data;		/* User disconnect data */
>  	GAtDebugFunc debugf;			/* Debugging output function */
>  	gpointer debug_data;			/* Data to pass to debug func */
>  	struct ring_buffer *read_buf;		/* Current read buffer */
> +	struct ring_buffer *write_buf;		/* Current write buffer */
> +	GSList *full_list;			/* List of full ring buffer */
> +	GSList *free_list;			/* List of free ring buffer */
>  	guint max_read_attempts;		/* Max reads per select */
>  	enum ParserState parser_state;
>  	gboolean destroyed;			/* Re-entrancy guard */
>  };
>  
> +static void g_at_server_wakeup_writer(GAtServer *server);
> +
> +static gboolean alloc_free_list(GAtServer *server)
> +{
> +	struct ring_buffer *buf;
> +	guint i;
> +
> +	for (i = 0; i < MAX_BUFFER_NUM; i++) {
> +#ifdef WRITE_SCHEDULER_DEBUG
> +		buf = ring_buffer_new(4);
> +#else
> +		buf = ring_buffer_new(4096);
> +#endif

Why not a macro to define buf size? So you don't need to use ifdef
everytime

#ifdef WRITE_SCHEDULER_DEBUG
		BUF_SIZE = 4
#else
		BUF_SIZE = 4096	
#endif

> +		if (!buf)
> +			return FALSE;
> +
> +		server->free_list = g_slist_prepend(server->free_list, buf);
> +	}
> +
> +	return TRUE;
> +}
> +
> +static void send_common(GAtServer *server, const char *buf)
> +{
> +	gsize avail = ring_buffer_avail(server->write_buf);
> +	gsize len = strlen(buf);
> +
> +	if (avail >= len)
> +		ring_buffer_write(server->write_buf, buf, len);
> +
> +	g_at_server_wakeup_writer(server);
> +}
> +
>  static void g_at_server_send_result(GAtServer *server, GAtServerResult result)
>  {
>  	struct v250_settings v250 = server->v250;
> @@ -108,7 +148,6 @@ static void g_at_server_send_result(GAtServer *server, GAtServerResult result)
>  	char buf[1024];
>  	char t = v250.s3;
>  	char r = v250.s4;
> -	gsize wbuf;
>  
>  	if (v250.quiet)
>  		return;
> @@ -125,8 +164,7 @@ static void g_at_server_send_result(GAtServer *server, GAtServerResult result)
>  	g_at_util_debug_chat(FALSE, buf, strlen(buf),
>  				server->debugf, server->debug_data);
>  
> -	g_io_channel_write(server->channel, (char *) buf, strlen(buf),
> -							&wbuf);
> +	send_common(server, buf);
>  }
>  
>  static inline gboolean is_at_command_prefix(const char c)
> @@ -432,12 +470,35 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
>  	return TRUE;
>  }
>  
> +static gboolean can_write_data(GIOChannel *channel, GIOCondition cond,
> +				gpointer data)
> +{
> +	return FALSE;
> +}
> +
> +static void free_ring_buffer(struct ring_buffer *buf)
> +{
> +	ring_buffer_free(buf);
> +}

This seems pointless to me.

> +
>  static void g_at_server_cleanup(GAtServer *server)
>  {
>  	/* Cleanup all received data */
>  	ring_buffer_free(server->read_buf);
>  	server->read_buf = NULL;
>  
> +	/* Cleanup pending data to write */
> +	ring_buffer_free(server->write_buf);
> +	server->write_buf = NULL;
> +
> +	if (server->full_list)
> +		g_slist_foreach(server->full_list, (GFunc)free_ring_buffer,
> +					NULL);
> +
> +	if (server->free_list)
> +		g_slist_foreach(server->free_list, (GFunc)free_ring_buffer,
> +					NULL);
> +
>  	server->channel = NULL;
>  }
>  
> @@ -446,8 +507,6 @@ static void read_watcher_destroy_notify(GAtServer *server)
>  	g_at_server_cleanup(server);
>  	server->read_watch = 0;
>  
> -	server->channel = NULL;
> -
>  	if (server->user_disconnect)
>  		server->user_disconnect(server->user_disconnect_data);
>  
> @@ -455,6 +514,23 @@ static void read_watcher_destroy_notify(GAtServer *server)
>  		g_free(server);
>  }
>  
> +static void write_watcher_destroy_notify(GAtServer *server)
> +{
> +	server->write_watch = 0;
> +}
> +
> +static void g_at_server_wakeup_writer(GAtServer *server)
> +{
> +	if (server->write_watch != 0)
> +		return;
> +
> +	server->write_watch = g_io_add_watch_full(server->channel,
> +			G_PRIORITY_DEFAULT,
> +			G_IO_OUT | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
> +			can_write_data, server,
> +			(GDestroyNotify)write_watcher_destroy_notify);
> +}
> +
>  static void v250_settings_create(struct v250_settings *v250)
>  {
>  	v250->s3 = '\r';
> @@ -483,11 +559,22 @@ GAtServer *g_at_server_new(GIOChannel *io)
>  	v250_settings_create(&server->v250);
>  	server->channel = io;
>  	server->read_buf = ring_buffer_new(4096);
> -	server->max_read_attempts = 3;
> -
>  	if (!server->read_buf)
>  		goto error;
>  
> +#ifdef WRITE_SCHEDULER_DEBUG
> +	server->write_buf = ring_buffer_new(4);
> +#else
> +	server->write_buf = ring_buffer_new(4096);
> +#endif
> +	if (!server->write_buf)
> +		goto error;
> +
> +	if (!alloc_free_list(server))
> +		goto error;
> +
> +	server->max_read_attempts = 3;
> +
>  	if (!g_at_util_setup_io(server->channel, G_IO_FLAG_NONBLOCK))
>  		goto error;
>  
> @@ -502,6 +589,13 @@ error:
>  	if (server->read_buf)
>  		ring_buffer_free(server->read_buf);
>  
> +	if (server->write_buf)
> +		ring_buffer_free(server->write_buf);
> +
> +	if (server->free_list)
> +		g_slist_foreach(server->free_list, (GFunc)free_ring_buffer,
> +					NULL);
> +
>  	if (server)
>  		g_free(server);
>  
> @@ -552,6 +646,9 @@ gboolean g_at_server_shutdown(GAtServer *server)
>  	server->user_disconnect = NULL;
>  	server->user_disconnect_data = NULL;
>  
> +	if (server->write_watch)
> +		g_source_remove(server->write_watch);
> +
>  	if (server->read_watch)
>  		g_source_remove(server->read_watch);
>  
> -- 
> 1.6.6.1
> 
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> http://lists.ofono.org/listinfo/ofono

-- 
Gustavo F. Padovan
http://padovan.org

ProFUSION embedded systems - http://profusion.mobi

Re: [PATCH 1/4] Add write ring buffer for non-blocking write

[Zhenhua Zhang]

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

Hi Padovan,

On 02/11/2010 01:23 AM, Gustavo F. Padovan wrote:
> * Zhenhua Zhang<zhenhua.zhang@intel.com>  [2010-02-10 16:13:40 +0800]:
>> +
>> +	for (i = 0; i<  MAX_BUFFER_NUM; i++) {
>> +#ifdef WRITE_SCHEDULER_DEBUG
>> +		buf = ring_buffer_new(4);
>> +#else
>> +		buf = ring_buffer_new(4096);
>> +#endif
>
> Why not a macro to define buf size? So you don't need to use ifdef
> everytime
>
> #ifdef WRITE_SCHEDULER_DEBUG
> 		BUF_SIZE = 4
> #else
> 		BUF_SIZE = 4096	
> #endif

Good suggestion. We can use a macro to define the buffer size.

>> +		if (!buf)
>> +			return FALSE;
>> +
>> +		server->free_list = g_slist_prepend(server->free_list, buf);
>> +	}
>> +
>> +	return TRUE;
>> +}
>> +
>> +static void free_ring_buffer(struct ring_buffer *buf)
>> +{
>> +	ring_buffer_free(buf);
>> +}
>
> This seems pointless to me.

It just remind me to call ring_buffer_free. Yes. I can remove it and 
call ring_buffer_free in g_slist_foreach() directly.

>> +
>>   static void g_at_server_cleanup(GAtServer *server)
>>   {
>>   	/* Cleanup all received data */
>>   	ring_buffer_free(server->read_buf);
>>   	server->read_buf = NULL;
>>
>> +	/* Cleanup pending data to write */
>> +	ring_buffer_free(server->write_buf);
>> +	server->write_buf = NULL;
>> +
>> +	if (server->full_list)
>> +		g_slist_foreach(server->full_list, (GFunc)free_ring_buffer,
>> +					NULL);
>> +
>> +	if (server->free_list)
>> +		g_slist_foreach(server->free_list, (GFunc)free_ring_buffer,
>> +					NULL);
>> +
>>   	server->channel = NULL;
>>   }
>>
>> @@ -446,8 +507,6 @@ static void read_watcher_destroy_notify(GAtServer *server)
>>   	g_at_server_cleanup(server);
>>   	server->read_watch = 0;
>>
>> -	server->channel = NULL;
>> -
>>   	if (server->user_disconnect)
>>   		server->user_disconnect(server->user_disconnect_data);
>>
>> @@ -455,6 +514,23 @@ static void read_watcher_destroy_notify(GAtServer *server)
>>   		g_free(server);
>>   }
>>
>> +static void write_watcher_destroy_notify(GAtServer *server)
>> +{
>> +	server->write_watch = 0;
>> +}
>> +
>> +static void g_at_server_wakeup_writer(GAtServer *server)
>> +{
>> +	if (server->write_watch != 0)
>> +		return;
>> +
>> +	server->write_watch = g_io_add_watch_full(server->channel,
>> +			G_PRIORITY_DEFAULT,
>> +			G_IO_OUT | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
>> +			can_write_data, server,
>> +			(GDestroyNotify)write_watcher_destroy_notify);
>> +}
>> +
>>   static void v250_settings_create(struct v250_settings *v250)
>>   {
>>   	v250->s3 = '\r';
>> @@ -483,11 +559,22 @@ GAtServer *g_at_server_new(GIOChannel *io)
>>   	v250_settings_create(&server->v250);
>>   	server->channel = io;
>>   	server->read_buf = ring_buffer_new(4096);
>> -	server->max_read_attempts = 3;
>> -
>>   	if (!server->read_buf)
>>   		goto error;
>>
>> +#ifdef WRITE_SCHEDULER_DEBUG
>> +	server->write_buf = ring_buffer_new(4);
>> +#else
>> +	server->write_buf = ring_buffer_new(4096);
>> +#endif
>> +	if (!server->write_buf)
>> +		goto error;
>> +
>> +	if (!alloc_free_list(server))
>> +		goto error;
>> +
>> +	server->max_read_attempts = 3;
>> +
>>   	if (!g_at_util_setup_io(server->channel, G_IO_FLAG_NONBLOCK))
>>   		goto error;
>>
>> @@ -502,6 +589,13 @@ error:
>>   	if (server->read_buf)
>>   		ring_buffer_free(server->read_buf);
>>
>> +	if (server->write_buf)
>> +		ring_buffer_free(server->write_buf);
>> +
>> +	if (server->free_list)
>> +		g_slist_foreach(server->free_list, (GFunc)free_ring_buffer,
>> +					NULL);
>> +
>>   	if (server)
>>   		g_free(server);
>>
>> @@ -552,6 +646,9 @@ gboolean g_at_server_shutdown(GAtServer *server)
>>   	server->user_disconnect = NULL;
>>   	server->user_disconnect_data = NULL;
>>
>> +	if (server->write_watch)
>> +		g_source_remove(server->write_watch);
>> +
>>   	if (server->read_watch)
>>   		g_source_remove(server->read_watch);
>>
>> --
>> 1.6.6.1
>>
>> _______________________________________________
>> ofono mailing list
>> ofono(a)ofono.org
>> http://lists.ofono.org/listinfo/ofono
>