PATCH: pgbench - option to build using ppoll() for larger connection counts

Started by Rady, Dougabout 8 years ago8 messages
#1Rady, Doug
radydoug@amazon.com
1 attachment(s)

[trying again for 2018-01]

This patch enables building pgbench to use ppoll() instead of select()
to allow for more than (FD_SETSIZE - 10) connections. As implemented,
when using ppoll(), the only connection limitation is system resources.

The patch has been implemented to introduce a minimal of #ifdef/#ifndef
clutter in the code.

Without this patch, one is limited to '(FD_SETSIZE - 10)’ number of connections.
Example of something that fails without this patch but works with the patch:

Without the patch:

$ pgbench -j 3000 -c 1500
invalid number of clients: "1500"

With the patch:

$ pgbench -j 3000 -c 1500
starting vacuum...end.
transaction type: <builtin: TPC-B (sort of)>
scaling factor: 2000
query mode: simple
number of clients: 1500
number of threads: 1500
number of transactions per client: 10
number of transactions actually processed: 15000/15000
latency average = 631.730 ms
tps = 2374.430587 (including connections establishing)
tps = 4206.524986 (excluding connections establishing)

doug
--
Doug Rady
Amazon Aurora, RDS PostgreSQL
radydoug@amazon.com

Attachments:

pgbench11-ppoll-v3.patchapplication/octet-stream; name=pgbench11-ppoll-v3.patchDownload
diff --git a/configure.in b/configure.in
index d9c4a50b4b..27cd84d135 100644
--- a/configure.in
+++ b/configure.in
@@ -1424,7 +1424,7 @@ PGAC_FUNC_WCSTOMBS_L
 LIBS_including_readline="$LIBS"
 LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
 
-AC_CHECK_FUNCS([cbrt clock_gettime dlopen fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate pstat pthread_is_threaded_np readlink setproctitle setsid shm_open symlink sync_file_range utime utimes wcstombs_l])
+AC_CHECK_FUNCS([cbrt clock_gettime dlopen fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setsid shm_open symlink sync_file_range utime utimes wcstombs_l])
 
 AC_REPLACE_FUNCS(fseeko)
 case $host_os in
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index bd96eae5e6..09c3e60ffb 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -47,6 +47,9 @@
 #ifdef HAVE_SYS_SELECT_H
 #include <sys/select.h>
 #endif
+#ifdef HAVE_POLL_H
+#include <poll.h>
+#endif
 
 #ifdef HAVE_SYS_RESOURCE_H
 #include <sys/resource.h>		/* for getrlimit */
@@ -56,6 +59,10 @@
 #define M_PI 3.14159265358979323846
 #endif
 
+#ifdef HAVE_PPOLL
+#define USE_PPOLL 1
+#endif
+
 #include "pgbench.h"
 
 #define ERRCODE_UNDEFINED_TABLE  "42P01"
@@ -90,6 +97,27 @@ static int	pthread_join(pthread_t th, void **thread_return);
 #define MAXCLIENTS	1024
 #endif
 
+#ifdef USE_PPOLL
+#define SCKTWTMTHD "ppoll"
+#undef MAXCLIENTS
+#define POLL_EVENTS (POLLIN|POLLPRI|POLLRDHUP)
+#define POLL_FAIL (POLLERR|POLLHUP|POLLNVAL|POLLRDHUP)
+#define PFD_RESETEV(s) { do { if ((s)->pfdp) { (s)->pfdp->events = POLL_EVENTS; (s)->pfdp->revents = 0; } } while(0); }
+#define PFD_ZERO(s) { do { if ((s)->pfdp) { (s)->pfdp->fd = -1; (s)->pfdp->events = POLL_EVENTS; (s)->pfdp->revents = 0; } } while(0); }
+#define PFD_SETFD(s) { do { (s)->pfdp->fd = PQsocket((s)->con); } while(0); }
+#define PFD_STRUCT_POLLFD(p)	struct pollfd   (p);
+#define PFD_THREAD_FREE(t) { do { if ((t)->pfds) { pg_free((t)->pfds); (t)->pfds = NULL; } } while (0); }
+#define PFD_THREAD_INIT(t,s,n) { do { int _i; (t)->pfds = (struct pollfd *) pg_malloc0(sizeof(struct pollfd) * (n)); for (_i = 0; _i < (n); _i++) { (s)[_i].pfdp = &(t)->pfds[_i]; (s)[_i].pfdp->fd = -1; } } while (0); }
+#else
+#define SCKTWTMTHD "select"
+#define PFD_RESETEV(s)
+#define PFD_ZERO(s)
+#define PFD_SETFD(s)
+#define PFD_STRUCT_POLLFD(p)
+#define PFD_THREAD_FREE(t)
+#define PFD_THREAD_INIT(t,s,n)
+#endif
+
 #define DEFAULT_INIT_STEPS "dtgvp"	/* default -I setting */
 
 #define LOG_STEP_SECONDS	5	/* seconds between log messages */
@@ -328,6 +356,7 @@ typedef struct
 	/* per client collected stats */
 	int64		cnt;			/* client transaction count, for -t */
 	int			ecnt;			/* error count */
+	PFD_STRUCT_POLLFD(*pfdp)
 } CState;
 
 /*
@@ -348,6 +377,7 @@ typedef struct
 	instr_time	conn_time;
 	StatsData	stats;
 	int64		latency_late;	/* executed but late transactions */
+	PFD_STRUCT_POLLFD(*pfds)
 } TState;
 
 #define INVALID_THREAD		((pthread_t) 0)
@@ -465,6 +495,7 @@ static void pgbench_error(const char *fmt,...) pg_attribute_printf(1, 2);
 static void addScript(ParsedScript script);
 static void *threadRun(void *arg);
 static void setalarm(int seconds);
+static void finishCon(CState *st);
 
 
 /* callback functions for our flex lexer */
@@ -933,6 +964,7 @@ discard_response(CState *state)
 		if (res)
 			PQclear(res);
 	} while (res);
+	PFD_RESETEV(state);
 }
 
 /* qsort comparator for Variable array */
@@ -2157,6 +2189,10 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 					if (INSTR_TIME_IS_ZERO(now))
 						INSTR_TIME_SET_CURRENT(now);
 					start = now;
+					if (debug)
+						fprintf(stderr, "client %d connecting ...\n",
+							st->id);
+
 					if ((st->con = doConnect()) == NULL)
 					{
 						fprintf(stderr, "client %d aborted while establishing connection\n",
@@ -2169,6 +2205,8 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 
 					/* Reset session-local state */
 					memset(st->prepared, 0, sizeof(st->prepared));
+					PFD_ZERO(st);
+					PFD_SETFD(st);
 				}
 
 				/*
@@ -2434,8 +2472,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 
 				if (is_connect)
 				{
-					PQfinish(st->con);
-					st->con = NULL;
+					finishCon(st);
 					INSTR_TIME_SET_ZERO(now);
 				}
 
@@ -2472,11 +2509,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 				 */
 			case CSTATE_ABORTED:
 			case CSTATE_FINISHED:
-				if (st->con != NULL)
-				{
-					PQfinish(st->con);
-					st->con = NULL;
-				}
+				finishCon(st);
 				return;
 		}
 	}
@@ -2624,13 +2657,7 @@ disconnect_all(CState *state, int length)
 	int			i;
 
 	for (i = 0; i < length; i++)
-	{
-		if (state[i].con)
-		{
-			PQfinish(state[i].con);
-			state[i].con = NULL;
-		}
-	}
+		finishCon(&state[i]);
 }
 
 /*
@@ -3949,7 +3976,11 @@ main(int argc, char **argv)
 			case 'c':
 				benchmarking_option_set = true;
 				nclients = atoi(optarg);
+#ifdef USE_PPOLL
+				if (nclients <= 0)
+#else
 				if (nclients <= 0 || nclients > MAXCLIENTS)
+#endif
 				{
 					fprintf(stderr, "invalid number of clients: \"%s\"\n",
 							optarg);
@@ -4629,6 +4660,8 @@ threadRun(void *arg)
 	StatsData	last,
 				aggs;
 
+	PFD_THREAD_INIT(thread, state, nstate);
+
 	/*
 	 * Initialize throttling rate target for all of the thread's clients.  It
 	 * might be a little more accurate to reset thread->start_time here too.
@@ -4669,8 +4702,12 @@ threadRun(void *arg)
 		/* make connections to the database */
 		for (i = 0; i < nstate; i++)
 		{
+			if (debug)
+				fprintf(stderr, "client %d connecting\n",
+					state[i].id);
 			if ((state[i].con = doConnect()) == NULL)
 				goto done;
+			PFD_SETFD(&state[i]);
 		}
 	}
 
@@ -4687,13 +4724,15 @@ threadRun(void *arg)
 	/* loop till all clients have terminated */
 	while (remains > 0)
 	{
-		fd_set		input_mask;
 		int			maxsock;	/* max socket number to be waited for */
 		int64		min_usec;
 		int64		now_usec = 0;	/* set this only if needed */
 
+#ifndef USE_PPOLL
+		fd_set		input_mask;
 		/* identify which client sockets should be checked for input */
 		FD_ZERO(&input_mask);
+#endif
 		maxsock = -1;
 		min_usec = PG_INT64_MAX;
 		for (i = 0; i < nstate; i++)
@@ -4704,8 +4743,7 @@ threadRun(void *arg)
 			{
 				/* interrupt client that has not started a transaction */
 				st->state = CSTATE_FINISHED;
-				PQfinish(st->con);
-				st->con = NULL;
+				finishCon(st);
 				remains--;
 			}
 			else if (st->state == CSTATE_SLEEP || st->state == CSTATE_THROTTLE)
@@ -4743,7 +4781,11 @@ threadRun(void *arg)
 					goto done;
 				}
 
+#ifdef USE_PPOLL
+				PFD_RESETEV(st);
+#else
 				FD_SET(sock, &input_mask);
+#endif
 				if (maxsock < sock)
 					maxsock = sock;
 			}
@@ -4791,11 +4833,19 @@ threadRun(void *arg)
 			{
 				if (maxsock != -1)
 				{
+#ifdef USE_PPOLL
+					struct timespec timeout;
+
+					timeout.tv_sec = min_usec / 1000000;
+					timeout.tv_nsec = min_usec % 1000000000;
+					nsocks = ppoll(thread->pfds, nstate, &timeout, NULL);
+#else
 					struct timeval timeout;
 
 					timeout.tv_sec = min_usec / 1000000;
 					timeout.tv_usec = min_usec % 1000000;
 					nsocks = select(maxsock + 1, &input_mask, NULL, NULL, &timeout);
+#endif
 				}
 				else			/* nothing active, simple sleep */
 				{
@@ -4804,7 +4854,11 @@ threadRun(void *arg)
 			}
 			else				/* no explicit delay, select without timeout */
 			{
+#ifdef USE_PPOLL
+				nsocks = ppoll(thread->pfds, nstate, NULL, NULL);
+#else
 				nsocks = select(maxsock + 1, &input_mask, NULL, NULL, NULL);
+#endif
 			}
 
 			if (nsocks < 0)
@@ -4815,10 +4869,11 @@ threadRun(void *arg)
 					continue;
 				}
 				/* must be something wrong */
-				fprintf(stderr, "select() failed: %s\n", strerror(errno));
+				fprintf(stderr, "%s() failed: %s\n", SCKTWTMTHD, strerror(errno));
 				goto done;
 			}
 		}
+#ifndef USE_PPOLL
 		else
 		{
 			/* min_usec == 0, i.e. something needs to be executed */
@@ -4826,6 +4881,7 @@ threadRun(void *arg)
 			/* If we didn't call select(), don't try to read any data */
 			FD_ZERO(&input_mask);
 		}
+#endif
 
 		/* ok, advance the state machine of each connection */
 		for (i = 0; i < nstate; i++)
@@ -4837,14 +4893,30 @@ threadRun(void *arg)
 				/* don't call doCustom unless data is available */
 				int			sock = PQsocket(st->con);
 
+#ifdef USE_PPOLL
+				if (sock < 0 && st->pfdp->revents & POLL_FAIL)
+				{
+					fprintf(stderr,
+						"ppoll() fail - errno: %d, i: %d, events: %x, %s\n",
++						errno, i,
+						((st->pfdp)->revents & POLL_FAIL),
+						PQerrorMessage(st->con));
+					goto done;
+				}
+#else
 				if (sock < 0)
 				{
 					fprintf(stderr, "invalid socket: %s",
 							PQerrorMessage(st->con));
 					goto done;
 				}
+#endif
 
+#ifdef USE_PPOLL
+				if (!(st->pfdp)->revents)
+#else
 				if (!FD_ISSET(sock, &input_mask))
+#endif
 					continue;
 			}
 			else if (st->state == CSTATE_FINISHED ||
@@ -4858,7 +4930,10 @@ threadRun(void *arg)
 
 			/* If doCustom changed client to finished state, reduce remains */
 			if (st->state == CSTATE_FINISHED || st->state == CSTATE_ABORTED)
+			{
 				remains--;
+				PFD_ZERO(st);
+			}
 		}
 
 		/* progress report is made by thread 0 for all threads */
@@ -4982,9 +5057,21 @@ done:
 		fclose(thread->logfile);
 		thread->logfile = NULL;
 	}
+	PFD_THREAD_FREE(thread);
 	return NULL;
 }
 
+static void
+finishCon(CState *st)
+{
+	if (st->con)
+	{
+		PQfinish(st->con);
+		st->con = NULL;
+	}
+	PFD_ZERO(st);
+}
+
 /*
  * Support for duration option: set timer_exceeded after so many seconds.
  */
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 84d59f12b2..21e720cfab 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -406,6 +406,9 @@
 /* Define to 1 if the assembler supports PPC's LWARX mutex hint bit. */
 #undef HAVE_PPC_LWARX_MUTEX_HINT
 
+/* Define to 1 if you have the `ppoll' function. */
+#undef HAVE_PPOLL
+
 /* Define to 1 if you have the `pstat' function. */
 #undef HAVE_PSTAT
 
#2Robert Haas
robertmhaas@gmail.com
In reply to: Rady, Doug (#1)
Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

On Tue, Nov 28, 2017 at 10:38 AM, Rady, Doug <radydoug@amazon.com> wrote:

This patch enables building pgbench to use ppoll() instead of select()
to allow for more than (FD_SETSIZE - 10) connections. As implemented,
when using ppoll(), the only connection limitation is system resources.

IIUC, ppoll() is to poll() as pselect() is to select(). Since the
existing code uses select(), why not convert to poll() rather than
ppoll()?

+#ifdef USE_PPOLL
+#define SCKTWTMTHD "ppoll"
+#undef MAXCLIENTS
+#define POLL_EVENTS (POLLIN|POLLPRI|POLLRDHUP)
+#define POLL_FAIL (POLLERR|POLLHUP|POLLNVAL|POLLRDHUP)
+#define PFD_RESETEV(s) { do { if ((s)->pfdp) { (s)->pfdp->events =
POLL_EVENTS; (s)->pfdp->revents = 0; } } while(0); }
+#define PFD_ZERO(s) { do { if ((s)->pfdp) { (s)->pfdp->fd = -1;
(s)->pfdp->events = POLL_EVENTS; (s)->pfdp->revents = 0; } } while(0);
}
+#define PFD_SETFD(s) { do { (s)->pfdp->fd = PQsocket((s)->con); } while(0); }
+#define PFD_STRUCT_POLLFD(p)    struct pollfd   (p);
+#define PFD_THREAD_FREE(t) { do { if ((t)->pfds) {
pg_free((t)->pfds); (t)->pfds = NULL; } } while (0); }
+#define PFD_THREAD_INIT(t,s,n) { do { int _i; (t)->pfds = (struct
pollfd *) pg_malloc0(sizeof(struct pollfd) * (n)); for (_i = 0; _i <
(n); _i++) { (s)[_i].pfdp = &(t)->pfds[_i]; (s)[_i].pfdp->fd = -1; } }
while (0); }
+#else
+#define SCKTWTMTHD "select"
+#define PFD_RESETEV(s)
+#define PFD_ZERO(s)
+#define PFD_SETFD(s)
+#define PFD_STRUCT_POLLFD(p)
+#define PFD_THREAD_FREE(t)
+#define PFD_THREAD_INIT(t,s,n)
+#endif

I find this an impenetrable mess. I am not going to commit a macro
with a ten-letter name that contains neither punctuation marks nor
vowels, nor one that is a single 200+ character that embeds a malloc
and a loop call but no comments. While I agree with the general goal
of avoiding introducing lots of #ifdefs, and while I think that the
introduction of finishCon() seems like a possibly-useful step in that
direction, I don't think that having a bunch of long, complex,
comment-free macros like this actually improves code clarity. Better
to have some embedded #ifdefs than this.

+                    if (debug)
+                        fprintf(stderr, "client %d connecting ...\n",
+                            st->id);
+

This is unrelated to the purpose of the patch. Please don't mix in
unrelated changes.

+            if (debug)
+                fprintf(stderr, "client %d connecting\n",
+                    state[i].id);

Ditto.

IMHO, this looks like a broadly reasonable thing to do. I had no idea
that FD_SETSIZE was ever set to a value much less than the number of
FDs the system could handle -- but if it is, then this seems like a
reasonable fix, once we agree on exactly how the details should look.

I don't remember off-hand whether you've done testing to see how
poll()/ppoll() performs compares to select(), but that might also be
something to check.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#3Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Rady, Doug (#1)
Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Hello,

This patch enables building pgbench to use ppoll() instead of select()
to allow for more than (FD_SETSIZE - 10) connections. As implemented,
when using ppoll(), the only connection limitation is system resources.

I'm fine with allowing more clients through ppoll, as large
multi/many/whatever-core hardware becomes more common and is expected to
grow.

However, I'm at odds with the how and the ppoll/select compatibility layer
offered by the patch, and the implied maintenance cost just to understand
what is happening while reading the code cluttered with possibly empty
macros and ifdef stuff. There are many ifdef, many strange/astute macros,
significant dissymetry in the code, which reduce maintainability
noticeably.

I think that it should abstract cleanly the two implementations into a set
of functions with the same signatures, even if some arguments are unsused,
and use these functions without ifdef stuff later. Maybe some macros too,
ok, but not too much.

"SCKTWTMTHD"... WTH? I do not want to know what it means. I'm sure a
better name (if something without vowels can be a name of anything in
English...) can be found. Names must be chosen with great care.

MAXCLIENTS: could be set to -1 when there is no limit, and tested to avoid
one ifdef.

I must admit that I do not see the benefit of "{ do { ... } while(0); }"
compared to "{ ... }". Maybe it has to do with compiler warnings.

The patch has been implemented to introduce a minimal of #ifdef/#ifndef
clutter in the code.

I think that it could be much more minimal than that. I would aim at
having just one.

#ifdef USE_PPOLL
specific functions definitions
some macros
#else /* SELECT */
idem for select
#endif

Ok, it may not be the best solution everywhere, but it should be
significantly reduce compared to the current state.

ISTM that ifdef which breaks the code structure should be avoided, eg

#ifdef ...
if (...)
#else
if (...)
#endif
{
// condition was true...

It is unclear to me why the input_mask is declared in the threadRun
function (so is thread specific) but pfds is in each thread struct (so is
also thread specific, but differently). The same logic should be used for
both, which ever is best. Not sure that "pfds" is the right name. If the
two variables means the same thing, they should have the same name,
although possibly different types.

$ pgbench -j 3000 -c 1500

Probably you intended 1500 threads for 3000 clients.

--
Fabien.

#4Robert Haas
robertmhaas@gmail.com
In reply to: Fabien COELHO (#3)
Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

On Wed, Nov 29, 2017 at 3:40 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

ever is best. Not sure that "pfds" is the right name. If the two variables
means the same thing, they should have the same name, although possibly
different types.

Although I agree with a good bit of what you say here, I don't agree
with that. If the member used by ppoll() (or just poll()) has a
different name than the one used for select(), it's much easier to,
say, grep for everyplace where the field you care about is used. If
you use the same name for different things, that doesn't work.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#5Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Robert Haas (#4)
Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Hello Robert,

ever is best. Not sure that "pfds" is the right name. If the two variables
means the same thing, they should have the same name, although possibly
different types.

Although I agree with a good bit of what you say here, I don't agree
with that. If the member used by ppoll() (or just poll()) has a
different name than the one used for select(), it's much easier to,
say, grep for everyplace where the field you care about is used. If
you use the same name for different things, that doesn't work.

I'm not sure it is incompatible.

My point is consistent with my other advice which is to hide the stuff in
functions with identical (or compatible) signatures, so that the only
place where it would differ would be in the functions, where greping would
work.

#ifdef USE_POLL
// pool specific stuff
#define SOME_TYPE v1_type (or typedef)
void do_stuff(v1_type * stuff) { ... }
...
#else /* default to SELECT */
// select specific stuff
#define SOME_TYPE v2_type (idem)
void do_stuff(v2_type * stuff) { ... }
...
#endif

Then later the code is not specific to poll or select, eg:

SOME_TYPE mask;
do_stuff(mask);
do_other_stuff(...);
if (is_ready(mask, ...)) { ... }

--
Fabien.

#6Robert Haas
robertmhaas@gmail.com
In reply to: Fabien COELHO (#5)
Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

On Wed, Nov 29, 2017 at 8:10 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

My point is consistent with my other advice which is to hide the stuff in
functions with identical (or compatible) signatures, so that the only place
where it would differ would be in the functions, where greping would work.

#ifdef USE_POLL
// pool specific stuff
#define SOME_TYPE v1_type (or typedef)
void do_stuff(v1_type * stuff) { ... }
...
#else /* default to SELECT */
// select specific stuff
#define SOME_TYPE v2_type (idem)
void do_stuff(v2_type * stuff) { ... }
...
#endif

Then later the code is not specific to poll or select, eg:

SOME_TYPE mask;
do_stuff(mask);
do_other_stuff(...);
if (is_ready(mask, ...)) { ... }

Yeah, that sort of style would be OK with me. But I wouldn't like:

struct blah {
#ifdef FOO
int doohicky;
#else
char *doohicky;
};

...because now any place in the code where you see "doohicky" you
don't immediately know whether it's an int or a char *

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#7Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Robert Haas (#6)
Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

[...] Yeah, that sort of style would be OK with me. But I wouldn't
like:

struct blah {
#ifdef FOO
int doohicky;
#else
char *doohicky;
};

Indeed. Me neither.

--
Fabien.

#8Stephen Frost
sfrost@snowman.net
In reply to: Rady, Doug (#1)
Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Doug,

* Rady, Doug (radydoug@amazon.com) wrote:

This patch enables building pgbench to use ppoll() instead of select()
to allow for more than (FD_SETSIZE - 10) connections. As implemented,
when using ppoll(), the only connection limitation is system resources.

Looks like this patch had some good feedback back when it was posted,
but we haven't seen any update to it based on that feedback and we're
now nearly a week into the January commitfest.

In order to keep the commitfest moving and to give you the best
opportunity at having the patch be reviewed and committed during this
cycle, I'd suggest trying to find some time soon to incorporate the
recommendations provided and post an updated patch for review.

Thanks!

Stephen