[PATCH] ctdb: calculate queue input buffer size correctly

[Swen Schillig]

Hey Martin

Thanks for having a look again.
Attached are the 2 patches as you suggested.

On Thu, 2018-08-09 at 16:35 +1000, Martin Schwenke wrote:
> On Thu, 26 Jul 2018 13:21:05 +0200, Swen Schillig via samba-technical
> <samba-technical at lists.samba.org> wrote:
> 
> > Please review and push if happy.
> > 
> > Thanks in advance for your support.
> 
> This looks like a good optimisation and it is now quite easy to
> understand.
> 
> Can we please make this 2 patches for the price of 1, separating out
> the switch to use MIN() to reduce num_ready when necessary?  That's a
> logic change that I think is unrelated to the optimisation.  I agree
> that the use of MIN() makes the logic clearer, so I think we should
> do
> it as a patch pre- or post- the optimisation.
> 
> With those 2 changes:
> 
> Reviewed-by: Martin Schwenke <martin at meltin.net>
> 
> I'm wondering is whether there's a way of avoiding the duplication of
> logic where the packet size is read from the buffer. One possibility
> is
> to rename the .extend element to .pkt_size and set it instead of a
> local in queue_io_read().  However, this seems messy and would
> probably
> clutter the patch for the optimisation.  So, I think we should go
> with
> duplication right now to get the optimisation in and think about it
> later.  :-)
Agree !
> 
> peace & happiness,
> martin
> 
Cheers Swen
-------------- next part --------------
From 95640d5b862fa3737120a01a0e4f99b0f4517b0d Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at vnet.ibm.com>
Date: Thu, 9 Aug 2018 09:15:36 +0200
Subject: [PATCH 1/2] ctdb: Replace calculation of bytes to read from socket by
 MIN() macro

The calculation of the bytes to read from the socket can be done easier
by the usage of the common MIN() macro.

Signed-off-by: Swen Schillig <swen at vnet.ibm.com>
---
 ctdb/common/ctdb_io.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/ctdb/common/ctdb_io.c b/ctdb/common/ctdb_io.c
index da444aca2b6..698febd0847 100644
--- a/ctdb/common/ctdb_io.c
+++ b/ctdb/common/ctdb_io.c
@@ -160,7 +160,6 @@ static void queue_io_read(struct ctdb_queue *queue)
 	int num_ready = 0;
 	ssize_t nread;
 	uint8_t *data;
-	int navail;
 
 	/* check how much data is available on the socket for immediately
 	   guaranteed nonblocking access.
@@ -196,10 +195,7 @@ static void queue_io_read(struct ctdb_queue *queue)
 		queue->buffer.extend = 0;
 	}
 
-	navail = queue->buffer.size - queue->buffer.length;
-	if (num_ready > navail) {
-		num_ready = navail;
-	}
+	num_ready = MIN(num_ready, queue->buffer.size - queue->buffer.length);
 
 	if (num_ready > 0) {
 		nread = sys_read(queue->fd,
-- 
2.17.1


From 2ec67e33a3d413aeb93b1f3739e4ac615cd1a45d Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at vnet.ibm.com>
Date: Wed, 7 Mar 2018 13:54:45 +0100
Subject: [PATCH 2/2] ctdb: calculate queue input buffer size correctly

The queue's input buffer is calculated in an iterative way.
This can result in a few back and forth jumping and
a few memory allocations and mem-free cycles.
This is very time consuming and not required, because the required
memory size can be calculated right away.

Signed-off-by: Swen Schillig <swen at vnet.ibm.com>
---
 ctdb/common/ctdb_io.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/ctdb/common/ctdb_io.c b/ctdb/common/ctdb_io.c
index 698febd0847..32d8fc753a6 100644
--- a/ctdb/common/ctdb_io.c
+++ b/ctdb/common/ctdb_io.c
@@ -43,7 +43,6 @@ struct ctdb_buffer {
 	uint8_t *data;
 	uint32_t length;
 	uint32_t size;
-	uint32_t extend;
 };
 
 struct ctdb_queue_pkt {
@@ -102,16 +101,15 @@ static void queue_process(struct ctdb_queue *queue)
 		return;
 	}
 
+	/* Did we at least read the size into the buffer */
 	pkt_size = *(uint32_t *)queue->buffer.data;
 	if (pkt_size == 0) {
 		DEBUG(DEBUG_CRIT, ("Invalid packet of length 0\n"));
 		goto failed;
 	}
 
+	/* the buffer doesn't contain the full packet, return to get the rest */
 	if (queue->buffer.length < pkt_size) {
-		if (pkt_size > queue->buffer_size) {
-			queue->buffer.extend = pkt_size;
-		}
 		return;
 	}
 
@@ -158,6 +156,7 @@ failed:
 static void queue_io_read(struct ctdb_queue *queue)
 {
 	int num_ready = 0;
+	uint32_t pkt_size;
 	ssize_t nread;
 	uint8_t *data;
 
@@ -183,18 +182,28 @@ static void queue_io_read(struct ctdb_queue *queue)
 			goto failed;
 		}
 		queue->buffer.size = queue->buffer_size;
-	} else if (queue->buffer.extend > 0) {
-		/* extending buffer */
-		data = talloc_realloc_size(queue, queue->buffer.data, queue->buffer.extend);
+		goto data_read;
+	}
+
+	if (queue->buffer.length < sizeof(pkt_size)) {
+		/* data read is not sufficient to gather message size */
+		goto data_read;
+	}
+
+	pkt_size = *(uint32_t *)queue->buffer.data;
+	if (pkt_size > queue->buffer.size) {
+		data = talloc_realloc_size(queue,
+					   queue->buffer.data,
+					   pkt_size);
 		if (data == NULL) {
-			DEBUG(DEBUG_ERR, ("read error realloc failed for %u\n", queue->buffer.extend));
+			DBG_ERR("read error realloc failed for %u\n", pkt_size);
 			goto failed;
 		}
 		queue->buffer.data = data;
-		queue->buffer.size = queue->buffer.extend;
-		queue->buffer.extend = 0;
+		queue->buffer.size = pkt_size;
 	}
 
+data_read:
 	num_ready = MIN(num_ready, queue->buffer.size - queue->buffer.length);
 
 	if (num_ready > 0) {
-- 
2.17.1