[PATCHv2 0/4] Socket optimization

[PATCHv2 0/4] Socket optimization

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

These patches add socket optimization wrt to buffer size.

Andrei Emeltchenko (4):
  android/socket: Cleanup sockets on unregister
  android/socket Use 64K buffer for socket handling
  android/socket: Use heap instead of stack
  android/socket: Setup socket buffer sizes

 android/socket.c | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

-- 
1.8.3.2

[PATCHv2 1/4] android/socket: Cleanup sockets on unregister

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

This cleans up rfsock structures closing all sockets and making general cleanup
for servers and for connections. This will be called form socket unregister.
---
 android/socket.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/android/socket.c b/android/socket.c
index c9eca44..9020874 100644
--- a/android/socket.c
+++ b/android/socket.c
@@ -93,8 +93,10 @@ static struct rfcomm_sock *create_rfsock(int sock, int *hal_fd)
 	return rfsock;
 }
 
-static void cleanup_rfsock(struct rfcomm_sock *rfsock)
+static void cleanup_rfsock(gpointer data)
 {
+	struct rfcomm_sock *rfsock = data;
+
 	DBG("rfsock: %p fd %d real_sock %d chan %u",
 		rfsock, rfsock->fd, rfsock->real_sock, rfsock->channel);
 
@@ -936,5 +938,8 @@ void bt_socket_unregister(void)
 {
 	DBG("");
 
+	g_list_free_full(connections, cleanup_rfsock);
+	g_list_free_full(servers, cleanup_rfsock);
+
 	ipc_unregister(HAL_SERVICE_ID_SOCK);
 }
-- 
1.8.3.2

Re: [PATCHv2 1/4] android/socket: Cleanup sockets on unregister

[Luiz Augusto von Dentz]

Hi Andrei,

On Tue, Dec 3, 2013 at 5:51 PM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> This cleans up rfsock structures closing all sockets and making general cleanup
> for servers and for connections. This will be called form socket unregister.
> ---
>  android/socket.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/android/socket.c b/android/socket.c
> index c9eca44..9020874 100644
> --- a/android/socket.c
> +++ b/android/socket.c
> @@ -93,8 +93,10 @@ static struct rfcomm_sock *create_rfsock(int sock, int *hal_fd)
>         return rfsock;
>  }
>
> -static void cleanup_rfsock(struct rfcomm_sock *rfsock)
> +static void cleanup_rfsock(gpointer data)
>  {
> +       struct rfcomm_sock *rfsock = data;
> +
>         DBG("rfsock: %p fd %d real_sock %d chan %u",
>                 rfsock, rfsock->fd, rfsock->real_sock, rfsock->channel);
>
> @@ -936,5 +938,8 @@ void bt_socket_unregister(void)
>  {
>         DBG("");
>
> +       g_list_free_full(connections, cleanup_rfsock);
> +       g_list_free_full(servers, cleanup_rfsock);
> +
>         ipc_unregister(HAL_SERVICE_ID_SOCK);
>  }
> --
> 1.8.3.2

This one is now applied, please rebase make the changes we discussed
for the last 3.


-- 
Luiz Augusto von Dentz

[PATCHv2 2/4] android/socket Use 64K buffer for socket handling

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Make SOCKET_BUFFER define and use 0xFFFE instead of 1K.
The value 0XFFFE is what Android sends in OBEX Connect packet in
Maximum Packet Length field. Though OBEX specify meximum packet
length as 64K - 1 which is 0xFFFF.
---
 android/socket.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/android/socket.c b/android/socket.c
index 9020874..9ff9019 100644
--- a/android/socket.c
+++ b/android/socket.c
@@ -52,6 +52,8 @@
 
 #define SVC_HINT_OBEX 0x10
 
+#define SOCKET_BUFFER 0xFFFE
+
 static bdaddr_t adapter_addr;
 
 /* Simple list of RFCOMM server sockets */
@@ -487,7 +489,7 @@ static gboolean sock_stack_event_cb(GIOChannel *io, GIOCondition cond,
 								gpointer data)
 {
 	struct rfcomm_sock *rfsock = data;
-	unsigned char buf[1024];
+	unsigned char buf[SOCKET_BUFFER];
 	int len, sent;
 
 	if (cond & G_IO_HUP) {
@@ -526,7 +528,7 @@ static gboolean sock_rfcomm_event_cb(GIOChannel *io, GIOCondition cond,
 								gpointer data)
 {
 	struct rfcomm_sock *rfsock = data;
-	unsigned char buf[1024];
+	unsigned char buf[SOCKET_BUFFER];
 	int len, sent;
 
 	if (cond & G_IO_HUP) {
-- 
1.8.3.2

Re: [PATCHv2 2/4] android/socket Use 64K buffer for socket handling

[Luiz Augusto von Dentz]

Hi Andrei,

On Tue, Dec 3, 2013 at 5:51 PM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Make SOCKET_BUFFER define and use 0xFFFE instead of 1K.
> The value 0XFFFE is what Android sends in OBEX Connect packet in
> Maximum Packet Length field. Though OBEX specify meximum packet
> length as 64K - 1 which is 0xFFFF.
> ---
>  android/socket.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/android/socket.c b/android/socket.c
> index 9020874..9ff9019 100644
> --- a/android/socket.c
> +++ b/android/socket.c
> @@ -52,6 +52,8 @@
>
>  #define SVC_HINT_OBEX 0x10
>
> +#define SOCKET_BUFFER 0xFFFE
> +
>  static bdaddr_t adapter_addr;
>
>  /* Simple list of RFCOMM server sockets */
> @@ -487,7 +489,7 @@ static gboolean sock_stack_event_cb(GIOChannel *io, GIOCondition cond,
>                                                                 gpointer data)
>  {
>         struct rfcomm_sock *rfsock = data;
> -       unsigned char buf[1024];
> +       unsigned char buf[SOCKET_BUFFER];
>         int len, sent;
>
>         if (cond & G_IO_HUP) {
> @@ -526,7 +528,7 @@ static gboolean sock_rfcomm_event_cb(GIOChannel *io, GIOCondition cond,
>                                                                 gpointer data)
>  {
>         struct rfcomm_sock *rfsock = data;
> -       unsigned char buf[1024];
> +       unsigned char buf[SOCKET_BUFFER];
>         int len, sent;
>
>         if (cond & G_IO_HUP) {
> --
> 1.8.3.2

We need to be a bit more generic here, the socket HAL is not
restricted to OBEX only, also it doesn't seems you are adjusting the
buffer level of the sockets, the buffer itself is just to copy between
the sockets so we have to follow how much the sockets can
transmit/receive not the other way around.

So it seems to me that we should either set the maximum MTU size we
could use with RFCOMM (UINT16_MAX?) or read the MTU once connected
(not sure if makes sense since it is SOCK_STREAM) and then allocate
the same amount as buffer in a field in struct rfcomm_sock, also this
needs then to be set back to socketpair with
setsocketopt(SO_RCVBUF/SO_SNDBUF) so we minimize context switches and
wakeups.


-- 
Luiz Augusto von Dentz

Re: [PATCHv2 2/4] android/socket Use 64K buffer for socket handling

[Andrei Emeltchenko]

Hi Luiz,

On Thu, Dec 05, 2013 at 02:43:10PM +0200, Luiz Augusto von Dentz wrote:
> Hi Andrei,
> 
> On Tue, Dec 3, 2013 at 5:51 PM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > Make SOCKET_BUFFER define and use 0xFFFE instead of 1K.
> > The value 0XFFFE is what Android sends in OBEX Connect packet in
> > Maximum Packet Length field. Though OBEX specify meximum packet
> > length as 64K - 1 which is 0xFFFF.
> > ---
> >  android/socket.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/android/socket.c b/android/socket.c
> > index 9020874..9ff9019 100644
> > --- a/android/socket.c
> > +++ b/android/socket.c
> > @@ -52,6 +52,8 @@
> >
> >  #define SVC_HINT_OBEX 0x10
> >
> > +#define SOCKET_BUFFER 0xFFFE
> > +
> >  static bdaddr_t adapter_addr;
> >
> >  /* Simple list of RFCOMM server sockets */
> > @@ -487,7 +489,7 @@ static gboolean sock_stack_event_cb(GIOChannel *io, GIOCondition cond,
> >                                                                 gpointer data)
> >  {
> >         struct rfcomm_sock *rfsock = data;
> > -       unsigned char buf[1024];
> > +       unsigned char buf[SOCKET_BUFFER];
> >         int len, sent;
> >
> >         if (cond & G_IO_HUP) {
> > @@ -526,7 +528,7 @@ static gboolean sock_rfcomm_event_cb(GIOChannel *io, GIOCondition cond,
> >                                                                 gpointer data)
> >  {
> >         struct rfcomm_sock *rfsock = data;
> > -       unsigned char buf[1024];
> > +       unsigned char buf[SOCKET_BUFFER];
> >         int len, sent;
> >
> >         if (cond & G_IO_HUP) {
> > --
> > 1.8.3.2
> 
> We need to be a bit more generic here, the socket HAL is not
> restricted to OBEX only, also it doesn't seems you are adjusting the
> buffer level of the sockets, 

it is in the following patch

> the buffer itself is just to copy between
> the sockets so we have to follow how much the sockets can
> transmit/receive not the other way around.

the idea is to copy data to socket buffers and then sleep.

> So it seems to me that we should either set the maximum MTU size we
> could use with RFCOMM (UINT16_MAX?) or read the MTU once connected
> (not sure if makes sense since it is SOCK_STREAM) and then allocate
> the same amount as buffer in a field in struct rfcomm_sock, also this
> needs then to be set back to socketpair with
> setsocketopt(SO_RCVBUF/SO_SNDBUF) so we minimize context switches and
> wakeups.

This can actually make more switches since RFCOMM packet size is
relatively small (990 bytes in my case). Maximum RFCOMM L2CAP MTU is 1013.

This is what we had before.

Best regards 
Andrei Emeltchenko 

[PATCHv2 3/4] android/socket: Use heap instead of stack

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Keep buffer used in socket copy in heap instead of stack.
---
 android/socket.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/android/socket.c b/android/socket.c
index 9ff9019..6293b59 100644
--- a/android/socket.c
+++ b/android/socket.c
@@ -73,6 +73,8 @@ struct rfcomm_sock {
 	bdaddr_t dst;
 	uint32_t service_handle;
 
+	unsigned char *buf;
+
 	const struct profile_info *profile;
 };
 
@@ -92,6 +94,8 @@ static struct rfcomm_sock *create_rfsock(int sock, int *hal_fd)
 	*hal_fd = fds[1];
 	rfsock->real_sock = sock;
 
+	rfsock->buf = g_malloc(SOCKET_BUFFER);
+
 	return rfsock;
 }
 
@@ -123,6 +127,8 @@ static void cleanup_rfsock(gpointer data)
 	if (rfsock->service_handle)
 		bt_adapter_remove_record(rfsock->service_handle);
 
+	g_free(rfsock->buf);
+
 	g_free(rfsock);
 }
 
@@ -489,7 +495,6 @@ static gboolean sock_stack_event_cb(GIOChannel *io, GIOCondition cond,
 								gpointer data)
 {
 	struct rfcomm_sock *rfsock = data;
-	unsigned char buf[SOCKET_BUFFER];
 	int len, sent;
 
 	if (cond & G_IO_HUP) {
@@ -503,14 +508,14 @@ static gboolean sock_stack_event_cb(GIOChannel *io, GIOCondition cond,
 		goto fail;
 	}
 
-	len = read(rfsock->fd, buf, sizeof(buf));
+	len = read(rfsock->fd, rfsock->buf, SOCKET_BUFFER);
 	if (len <= 0) {
 		error("read(): %s", strerror(errno));
 		/* Read again */
 		return TRUE;
 	}
 
-	sent = try_write_all(rfsock->real_sock, buf, len);
+	sent = try_write_all(rfsock->real_sock, rfsock->buf, len);
 	if (sent < 0) {
 		error("write(): %s", strerror(errno));
 		goto fail;
@@ -528,7 +533,6 @@ static gboolean sock_rfcomm_event_cb(GIOChannel *io, GIOCondition cond,
 								gpointer data)
 {
 	struct rfcomm_sock *rfsock = data;
-	unsigned char buf[SOCKET_BUFFER];
 	int len, sent;
 
 	if (cond & G_IO_HUP) {
@@ -542,14 +546,14 @@ static gboolean sock_rfcomm_event_cb(GIOChannel *io, GIOCondition cond,
 		goto fail;
 	}
 
-	len = read(rfsock->real_sock, buf, sizeof(buf));
+	len = read(rfsock->real_sock, rfsock->buf, SOCKET_BUFFER);
 	if (len <= 0) {
 		error("read(): %s", strerror(errno));
 		/* Read again */
 		return TRUE;
 	}
 
-	sent = try_write_all(rfsock->fd, buf, len);
+	sent = try_write_all(rfsock->fd, rfsock->buf, len);
 	if (sent < 0) {
 		error("write(): %s", strerror(errno));
 		goto fail;
-- 
1.8.3.2

[PATCHv2 4/4] android/socket: Setup socket buffer sizes

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Set socket buffer sizes for socketpair's end and real RFCOMM socket
equal to SOCKET_BUFFER.
---
 android/socket.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/android/socket.c b/android/socket.c
index 6293b59..3898767 100644
--- a/android/socket.c
+++ b/android/socket.c
@@ -82,6 +82,9 @@ static struct rfcomm_sock *create_rfsock(int sock, int *hal_fd)
 {
 	int fds[2] = {-1, -1};
 	struct rfcomm_sock *rfsock;
+	socklen_t len = sizeof(int);
+	int size = SOCKET_BUFFER;
+	int i;
 
 	if (socketpair(AF_LOCAL, SOCK_STREAM, 0, fds) < 0) {
 		error("socketpair(): %s", strerror(errno));
@@ -94,6 +97,15 @@ static struct rfcomm_sock *create_rfsock(int sock, int *hal_fd)
 	*hal_fd = fds[1];
 	rfsock->real_sock = sock;
 
+	for (i = 0; i < 2; i++) {
+		if (setsockopt(fds[i], SOL_SOCKET, SO_SNDBUF, &size, len) < 0)
+			warn("setsockopt() SO_SNDBUF fd: %d %s", fds[i],
+							strerror(errno));
+		if (setsockopt(fds[i], SOL_SOCKET, SO_RCVBUF, &size, len) < 0)
+			warn("setsockopt() SO_RCVBUF fd: %d %s", fds[i],
+							strerror(errno));
+	}
+
 	rfsock->buf = g_malloc(SOCKET_BUFFER);
 
 	return rfsock;
-- 
1.8.3.2