[PATCH 0/2] Reusing nfct handle for bulk ct loads

[PATCH 0/2] Reusing nfct handle for bulk ct loads

[Mikhail Sennikovsky]

Hi Pablo & all,

I've finally got my hands on doing some more sophisticated testing
of the bulk ct operations functionality (-R option), and revealed 
a way to makethe ct load operations to be almost twice as fast 
by reusing the same nfct handleinstead of recreating it for each
entry.
This becomes essential for bulk loads of big amounts of entries,
where the overall speedup reaches values of seconds.

The two patches posted here are the tests/conntrack/bulk-load-stress.sh 
script that could be used for stress testing the bulk operations
and the patch improving the bulk ct load performance.

(Btw the script also reveals that bulk deletion of big amount
of entries specified individually is really slow,
but it's another story which could be addressed later if needed.)

Any feedback/suggestions for the patches are welcome.

Regards,
Mikhail


Mikhail Sennikovsky (2):
  tests/conntrack: script for stress-testing ct load
  conntrack: use same nfct handle for all entries

 src/conntrack.c                     | 160 +++++++++++++++++-----------
 tests/conntrack/bulk-load-stress.sh | 152 ++++++++++++++++++++++++++
 2 files changed, 247 insertions(+), 65 deletions(-)
 create mode 100755 tests/conntrack/bulk-load-stress.sh

-- 
2.25.1

[PATCH 1/2] tests/conntrack: script for stress-testing ct load

[Mikhail Sennikovsky]

The tests/conntrack/bulk-load-stress.sh is intended to be used for
stress-testing the bulk load of ct entries from a file (-R option).

Script usage detail is given by the ./bulk-load-stress.sh -h

Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@ionos.com>
---
 tests/conntrack/bulk-load-stress.sh | 152 ++++++++++++++++++++++++++++
 1 file changed, 152 insertions(+)
 create mode 100755 tests/conntrack/bulk-load-stress.sh

diff --git a/tests/conntrack/bulk-load-stress.sh b/tests/conntrack/bulk-load-stress.sh
new file mode 100755
index 0000000..de7bd8d
--- /dev/null
+++ b/tests/conntrack/bulk-load-stress.sh
@@ -0,0 +1,152 @@
+#!/bin/bash
+
+DEFAULT_CT="../../src/conntrack"
+DEFAULT_SPORT_COUNT=0xffff
+DEFAULT_DPORT_COUNT=0x2
+DEFAULT_TMP_FILE="./ct_data.txt"
+DEFAULT_CT_ZONE=123
+DEFAULT_GEN_ONLY=0
+DEFAULT_CLEANUP_INDIVIDUAL=0
+
+
+CT=$DEFAULT_CT
+SPORT_COUNT=$DEFAULT_SPORT_COUNT
+DPORT_COUNT=$DEFAULT_DPORT_COUNT
+TMP_FILE=$DEFAULT_TMP_FILE
+CT_ZONE=$DEFAULT_CT_ZONE
+GEN_ONLY=$DEFAULT_GEN_ONLY
+CLEANUP_INDIVIDUAL=$DEFAULT_CLEANUP_INDIVIDUAL
+
+
+print_help()
+{
+  me=$(basename "$0")
+
+  echo "Script for stress-testing bulk ct entries load (-R option)"
+  echo ""
+  echo "Usage: $me [options]"
+  echo ""
+  echo "Where options can be:"
+  echo ""
+  echo "-dpc <dst_port_count> -  number of destination port values."
+  echo "                         Default is ${DEFAULT_DPORT_COUNT}."
+  echo ""
+  echo "-spc <src_port_count> -  number of source port values."
+  echo "                         Default is ${DEFAULT_SPORT_COUNT}."
+  echo ""
+  echo "-ct <ct_tool_path>    -  path to the conntrack tool."
+  echo "                         Default is ${DEFAULT_CT}."
+  echo ""
+  echo "-z <ct_zone>          -  ct zone to be used."
+  echo "                         Default is ${DEFAULT_CT_ZONE}."
+  echo ""
+  echo "-f <tmp_file_name>    -  tmp file to be used to generate the ct data to."
+  echo "                         Default is ${DEFAULT_TMP_FILE}."
+  echo ""
+  echo "-g                    -  Generate tmp file and exit."
+  echo ""
+  echo "-h                    -  Print this help and exit."
+}
+
+
+while [ $# -gt 0 ]
+do
+  case "$1" in
+    -spc)  SPORT_COUNT=${2:-}
+      if [ -z "$SPORT_COUNT" ]
+      then
+        echo "Source port must be specified!"
+        print_help
+        exit 1
+      fi
+      shift
+      ;;
+    -dpc)  DPORT_COUNT=${2:-}
+      if [ -z "$DPORT_COUNT" ]
+      then
+        echo "Destination port must be specified!"
+        print_help
+        exit 1
+      fi
+      shift
+      ;;
+    -ct)   CT=${2:-}
+      if [ -z "$CT" ]
+      then
+        echo "conntrack path must be specified!"
+        print_help
+        exit 1
+      fi
+      shift
+      ;;
+    -z)    CT_ZONE=${2:-}
+      if [ -z "$CT_ZONE" ]
+      then
+        echo "ct zone must be specified!"
+        print_help
+        exit 1
+      fi
+      shift
+      ;;
+    -f)    TMP_FILE=${2:-}
+      if [ -z "$TMP_FILE" ]
+      then
+        echo "Tmp file must be specified!"
+        print_help
+        exit 1
+      fi
+      shift
+      ;;
+    -g)    GEN_ONLY=1
+      ;;
+    -ci)   CLEANUP_INDIVIDUAL=1
+      ;;
+    -h)    print_help
+      exit 1
+      ;;
+    *)     echo "Unknown paramerer \"$1\""
+      print_help
+      exit 1
+      ;;
+  esac
+  shift
+done
+
+
+function ct_data_gen()
+{
+  for (( d = 1; d <= $DPORT_COUNT; d++ )) do
+    for (( s = 1; s <= $SPORT_COUNT; s++ )) do
+      echo "-I -w $CT_ZONE -s 1.1.1.1 -d 2.2.2.2 -p tcp --sport ${s} --dport ${d} --state LISTEN -u SEEN_REPLY -t 50"
+    done
+  done
+}
+
+ct_data_gen > $TMP_FILE
+
+NUM_ENTRIES=$(cat ${TMP_FILE} | wc -l)
+
+echo "File ${TMP_FILE} is generated, number of entries: ${NUM_ENTRIES}."
+
+if [ "$GEN_ONLY" -eq "1" ]; then
+  exit 0
+fi
+
+echo "Loading ${NUM_ENTRIES} entries from ${TMP_FILE} .."
+sudo time -p ${CT} -R $TMP_FILE
+
+if [ "$CLEANUP_INDIVIDUAL" -eq "1" ]; then
+  sed -i -e "s/-I/-D/g" -e "s/-t 50//g" $TMP_FILE
+
+  NUM_ENTRIES=$(cat ${TMP_FILE} | wc -l)
+
+  echo "File ${TMP_FILE} is updated, number of entries: ${NUM_ENTRIES}."
+
+  echo "Cleaning ${NUM_ENTRIES} entries from ${TMP_FILE} .."
+  sudo time -p ${CT} -R $TMP_FILE
+fi
+
+
+echo "Cleaning up zone ${CT_ZONE}.."
+sudo time -p ${CT} -D -w $CT_ZONE > /dev/null
+rm $TMP_FILE
-- 
2.25.1

Re: [PATCH 1/2] tests/conntrack: script for stress-testing ct load

[Pablo Neira Ayuso]

On Mon, Aug 23, 2021 at 05:57:14PM +0200, Mikhail Sennikovsky wrote:
> The tests/conntrack/bulk-load-stress.sh is intended to be used for
> stress-testing the bulk load of ct entries from a file (-R option).
> 
> Script usage detail is given by the ./bulk-load-stress.sh -h

Applied with a few edits, thanks.

[PATCH 2/2] conntrack: use same nfct handle for all entries

[Mikhail Sennikovsky]

For bulk ct entry loads (with -R option) reusing the same nftc handle
for all entries results in ~two-time reduction of entries creation
time. This becomes signifficant when loading tens of thouthand of
entries.
E.g. in the tests performed with the tests/conntrack/bulk-load-stress.sh
the time used for loading of 131070 ct entries (2 * 0xffff)
was 1.05s when this single nfct handle adjustment and 1.88s w/o it .

Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@ionos.com>
---
 src/conntrack.c | 160 ++++++++++++++++++++++++++++--------------------
 1 file changed, 95 insertions(+), 65 deletions(-)

diff --git a/src/conntrack.c b/src/conntrack.c
index ef7f604..7a86420 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -105,6 +105,59 @@ struct ct_tmpl {
 
 static struct ct_tmpl *cur_tmpl;
 
+
+struct ct_ctx {
+	struct nfct_handle *cth;
+	struct nfct_handle *ith;
+	uint8_t cur_subsys_id;
+	unsigned cur_subscriptions;
+} ct_ctx;
+
+static void ct_ctx_term(void)
+{
+	if (ct_ctx.cth)
+		nfct_close(ct_ctx.cth);
+	if (ct_ctx.ith)
+		nfct_close(ct_ctx.ith);
+	memset(&ct_ctx, 0, sizeof(ct_ctx));
+}
+
+static inline struct nfct_handle* _ct_ctx_get(struct nfct_handle **ph,
+		uint8_t subsys_id, unsigned subscriptions)
+{
+	struct nfct_handle *h = *ph;
+
+	if (!ct_ctx.cur_subsys_id) {
+		ct_ctx.cur_subsys_id = subsys_id;
+		ct_ctx.cur_subscriptions = subscriptions;
+	} else if (ct_ctx.cur_subsys_id != subsys_id
+			|| ct_ctx.cur_subscriptions != subscriptions) {
+		exit_error(OTHER_PROBLEM, "handler parameters mismatch!");
+	}
+
+	if (!h) {
+		h = nfct_open(subsys_id, subscriptions);
+		if (!h)
+			exit_error(OTHER_PROBLEM, "Can't open handler");
+		*ph = h;
+	}
+
+	return h;
+}
+
+static struct nfct_handle* ct_ctx_get_cth(
+		uint8_t subsys_id, unsigned subscriptions)
+{
+	return _ct_ctx_get(&ct_ctx.cth, subsys_id, subscriptions);
+}
+
+static struct nfct_handle* ct_ctx_get_ith(
+		uint8_t subsys_id, unsigned subscriptions)
+{
+	return _ct_ctx_get(&ct_ctx.ith, subsys_id, subscriptions);
+}
+
+
 struct ct_cmd {
 	struct list_head list;
 	unsigned int	command;
@@ -605,7 +658,6 @@ static const char usage_parameters[] =
 
 #define OPTION_OFFSET 256
 
-static struct nfct_handle *cth, *ith;
 static struct option *opts = original_opts;
 static unsigned int global_option_offset = 0;
 
@@ -1739,7 +1791,7 @@ exp_event_sighandler(int s)
 
 	fprintf(stderr, "%s v%s (conntrack-tools): ", PROGNAME, VERSION);
 	fprintf(stderr, "%d expectation events have been shown.\n", counter);
-	nfct_close(cth);
+	ct_ctx_term();
 	exit(0);
 }
 
@@ -2046,7 +2098,7 @@ static int delete_cb(enum nf_conntrack_msg_type type,
 	if (nfct_filter(cmd, ct, cur_tmpl))
 		return NFCT_CB_CONTINUE;
 
-	res = nfct_query(ith, NFCT_Q_DESTROY, ct);
+	res = nfct_query(ct_ctx.ith, NFCT_Q_DESTROY, ct);
 	if (res < 0)
 		exit_error(OTHER_PROBLEM,
 			   "Operation failed: %s",
@@ -2230,19 +2282,19 @@ static int update_cb(enum nf_conntrack_msg_type type,
 		return NFCT_CB_CONTINUE;
 	}
 
-	res = nfct_query(ith, NFCT_Q_UPDATE, tmp);
+	res = nfct_query(ct_ctx.ith, NFCT_Q_UPDATE, tmp);
 	if (res < 0)
 		fprintf(stderr,
 			   "Operation failed: %s\n",
 			   err2str(errno, CT_UPDATE));
-	nfct_callback_register(ith, NFCT_T_ALL, print_cb, NULL);
+	nfct_callback_register(ct_ctx.ith, NFCT_T_ALL, print_cb, NULL);
 
-	res = nfct_query(ith, NFCT_Q_GET, tmp);
+	res = nfct_query(ct_ctx.ith, NFCT_Q_GET, tmp);
 	if (res < 0) {
 		nfct_destroy(tmp);
 		/* the entry has vanish in middle of the update */
 		if (errno == ENOENT) {
-			nfct_callback_unregister(ith);
+			nfct_callback_unregister(ct_ctx.ith);
 			return NFCT_CB_CONTINUE;
 		}
 		exit_error(OTHER_PROBLEM,
@@ -2250,7 +2302,7 @@ static int update_cb(enum nf_conntrack_msg_type type,
 			   err2str(errno, CT_UPDATE));
 	}
 	nfct_destroy(tmp);
-	nfct_callback_unregister(ith);
+	nfct_callback_unregister(ct_ctx.ith);
 
 	counter++;
 
@@ -3178,6 +3230,7 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
 
 static int do_command_ct(const char *progname, struct ct_cmd *cmd)
 {
+	struct nfct_handle *cth;
 	struct nfct_filter_dump *filter_dump;
 	int res = 0;
 
@@ -3205,9 +3258,7 @@ static int do_command_ct(const char *progname, struct ct_cmd *cmd)
 			break;
 		}
 
-		cth = nfct_open(CONNTRACK, 0);
-		if (!cth)
-			exit_error(OTHER_PROBLEM, "Can't open handler");
+		cth = ct_ctx_get_cth(CONNTRACK, 0);
 
 		if (cmd->options & CT_COMPARISON &&
 		    cmd->options & CT_OPT_ZERO)
@@ -3248,17 +3299,15 @@ static int do_command_ct(const char *progname, struct ct_cmd *cmd)
 			fflush(stdout);
 		}
 
-		nfct_close(cth);
 		break;
 
 	case EXP_LIST:
-		cth = nfct_open(EXPECT, 0);
+		cth = ct_ctx_get_cth(EXPECT, 0);
 		if (!cth)
 			exit_error(OTHER_PROBLEM, "Can't open handler");
 
 		nfexp_callback_register(cth, NFCT_T_ALL, dump_exp_cb, NULL);
 		res = nfexp_query(cth, NFCT_Q_DUMP, &cmd->family);
-		nfct_close(cth);
 
 		if (dump_xml_header_done == 0) {
 			printf("</expect>\n");
@@ -3279,14 +3328,11 @@ static int do_command_ct(const char *progname, struct ct_cmd *cmd)
 			nfct_set_attr(cmd->tmpl.ct, ATTR_CONNLABELS,
 					xnfct_bitmask_clone(cmd->tmpl.label_modify));
 
-		cth = nfct_open(CONNTRACK, 0);
-		if (!cth)
-			exit_error(OTHER_PROBLEM, "Can't open handler");
+		cth = ct_ctx_get_cth(CONNTRACK, 0);
 
 		res = nfct_query(cth, NFCT_Q_CREATE, cmd->tmpl.ct);
 		if (res != -1)
 			counter++;
-		nfct_close(cth);
 		break;
 
 	case EXP_CREATE:
@@ -3294,35 +3340,41 @@ static int do_command_ct(const char *progname, struct ct_cmd *cmd)
 		nfexp_set_attr(cmd->tmpl.exp, ATTR_EXP_EXPECTED, cmd->tmpl.exptuple);
 		nfexp_set_attr(cmd->tmpl.exp, ATTR_EXP_MASK, cmd->tmpl.mask);
 
-		cth = nfct_open(EXPECT, 0);
-		if (!cth)
-			exit_error(OTHER_PROBLEM, "Can't open handler");
+		cth = ct_ctx_get_cth(EXPECT, 0);
 
 		res = nfexp_query(cth, NFCT_Q_CREATE, cmd->tmpl.exp);
-		nfct_close(cth);
 		break;
 
 	case CT_UPDATE:
-		cth = nfct_open(CONNTRACK, 0);
-		/* internal handler for delete_cb, otherwise we hit EILSEQ */
-		ith = nfct_open(CONNTRACK, 0);
-		if (!cth || !ith)
-			exit_error(OTHER_PROBLEM, "Can't open handler");
+		cth = ct_ctx_get_cth(CONNTRACK, 0);
+		/*
+		 * Internal handler for update_cb, otherwise we hit EILSEQ.
+		 *
+		 * Don't need a return value here, just need it to be initialized
+		 * to be used in the update_cb.
+		 * In case the ith handle creation fails, the ct_ctx_get_ith will do
+		 * exit_error internally.
+		 */
+		ct_ctx_get_ith(CONNTRACK, 0);
 
 		nfct_filter_init(cmd);
 
 		nfct_callback_register(cth, NFCT_T_ALL, update_cb, cmd);
 
 		res = nfct_query(cth, NFCT_Q_DUMP, &cmd->family);
-		nfct_close(ith);
-		nfct_close(cth);
 		break;
 
 	case CT_DELETE:
-		cth = nfct_open(CONNTRACK, 0);
-		ith = nfct_open(CONNTRACK, 0);
-		if (!cth || !ith)
-			exit_error(OTHER_PROBLEM, "Can't open handler");
+		cth = ct_ctx_get_cth(CONNTRACK, 0);
+		/*
+		 * Internal handler for delete_cb, otherwise we hit EILSEQ.
+		 *
+		 * Don't need a return value here, just need it to be initialized
+		 * to be used in the delete_cb.
+		 * In case the ith handle creation fails, the ct_ctx_get_ith will do
+		 * exit_error internally.
+		 */
+		ct_ctx_get_ith(CONNTRACK, 0);
 
 		nfct_filter_init(cmd);
 
@@ -3345,59 +3397,42 @@ static int do_command_ct(const char *progname, struct ct_cmd *cmd)
 
 		nfct_filter_dump_destroy(filter_dump);
 
-		nfct_close(ith);
-		nfct_close(cth);
 		break;
 
 	case EXP_DELETE:
 		nfexp_set_attr(cmd->tmpl.exp, ATTR_EXP_EXPECTED, cmd->tmpl.ct);
 
-		cth = nfct_open(EXPECT, 0);
-		if (!cth)
-			exit_error(OTHER_PROBLEM, "Can't open handler");
+		cth = ct_ctx_get_cth(EXPECT, 0);
 
 		res = nfexp_query(cth, NFCT_Q_DESTROY, cmd->tmpl.exp);
-		nfct_close(cth);
 		break;
 
 	case CT_GET:
-		cth = nfct_open(CONNTRACK, 0);
-		if (!cth)
-			exit_error(OTHER_PROBLEM, "Can't open handler");
+		cth = ct_ctx_get_cth(CONNTRACK, 0);
 
 		nfct_callback_register(cth, NFCT_T_ALL, dump_cb, cmd);
 		res = nfct_query(cth, NFCT_Q_GET, cmd->tmpl.ct);
-		nfct_close(cth);
 		break;
 
 	case EXP_GET:
 		nfexp_set_attr(cmd->tmpl.exp, ATTR_EXP_MASTER, cmd->tmpl.ct);
 
-		cth = nfct_open(EXPECT, 0);
-		if (!cth)
-			exit_error(OTHER_PROBLEM, "Can't open handler");
+		cth = ct_ctx_get_cth(EXPECT, 0);
 
 		nfexp_callback_register(cth, NFCT_T_ALL, dump_exp_cb, NULL);
 		res = nfexp_query(cth, NFCT_Q_GET, cmd->tmpl.exp);
-		nfct_close(cth);
 		break;
 
 	case CT_FLUSH:
-		cth = nfct_open(CONNTRACK, 0);
-		if (!cth)
-			exit_error(OTHER_PROBLEM, "Can't open handler");
+		cth = ct_ctx_get_cth(CONNTRACK, 0);
 		res = nfct_query(cth, NFCT_Q_FLUSH_FILTER, &cmd->family);
-		nfct_close(cth);
 		fprintf(stderr, "%s v%s (conntrack-tools): ",PROGNAME,VERSION);
 		fprintf(stderr,"connection tracking table has been emptied.\n");
 		break;
 
 	case EXP_FLUSH:
-		cth = nfct_open(EXPECT, 0);
-		if (!cth)
-			exit_error(OTHER_PROBLEM, "Can't open handler");
+		cth = ct_ctx_get_cth(EXPECT, 0);
 		res = nfexp_query(cth, NFCT_Q_FLUSH, &cmd->family);
-		nfct_close(cth);
 		fprintf(stderr, "%s v%s (conntrack-tools): ",PROGNAME,VERSION);
 		fprintf(stderr,"expectation table has been emptied.\n");
 		break;
@@ -3486,21 +3521,18 @@ static int do_command_ct(const char *progname, struct ct_cmd *cmd)
 			if (cmd->event_mask & CT_EVENT_F_DEL)
 				nl_events |= NF_NETLINK_CONNTRACK_EXP_DESTROY;
 
-			cth = nfct_open(CONNTRACK, nl_events);
+			cth = ct_ctx_get_cth(CONNTRACK, nl_events);
 		} else {
-			cth = nfct_open(EXPECT,
+			cth = ct_ctx_get_cth(EXPECT,
 					NF_NETLINK_CONNTRACK_EXP_NEW |
 					NF_NETLINK_CONNTRACK_EXP_UPDATE |
 					NF_NETLINK_CONNTRACK_EXP_DESTROY);
 		}
 
-		if (!cth)
-			exit_error(OTHER_PROBLEM, "Can't open handler");
 		signal(SIGINT, exp_event_sighandler);
 		signal(SIGTERM, exp_event_sighandler);
 		nfexp_callback_register(cth, NFCT_T_ALL, event_exp_cb, NULL);
 		res = nfexp_catch(cth);
-		nfct_close(cth);
 		break;
 	case CT_COUNT:
 		/* If we fail with netlink, fall back to /proc to ensure
@@ -3538,13 +3570,9 @@ try_proc_count:
 		break;
 	}
 	case EXP_COUNT:
-		cth = nfct_open(EXPECT, 0);
-		if (!cth)
-			exit_error(OTHER_PROBLEM, "Can't open handler");
-
+		cth = ct_ctx_get_cth(EXPECT, 0);
 		nfexp_callback_register(cth, NFCT_T_ALL, count_exp_cb, NULL);
 		res = nfexp_query(cth, NFCT_Q_DUMP, &cmd->family);
-		nfct_close(cth);
 		printf("%d\n", counter);
 		break;
 	case CT_STATS:
@@ -3840,5 +3868,7 @@ int main(int argc, char *argv[])
 		free(cmd);
 	}
 
+	ct_ctx_term();
+
 	return res < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
 }
-- 
2.25.1

Re: [PATCH 2/2] conntrack: use same nfct handle for all entries

[Pablo Neira Ayuso]

Hi Mikhail,

On Mon, Aug 23, 2021 at 05:57:15PM +0200, Mikhail Sennikovsky wrote:
> For bulk ct entry loads (with -R option) reusing the same nftc handle
> for all entries results in ~two-time reduction of entries creation
> time. This becomes signifficant when loading tens of thouthand of
> entries.

This is showing on of the limitations of the original API, I started
sketching a patch to update this code to use libmnl, I'd rather follow
this path.

Would you have the time to take it over and look into it?

> E.g. in the tests performed with the tests/conntrack/bulk-load-stress.sh
> the time used for loading of 131070 ct entries (2 * 0xffff)
> was 1.05s when this single nfct handle adjustment and 1.88s w/o it .

What is making things go faster? What is introduding the extra
overhead?

Thanks