diff mbox series

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

Message ID 20210823155715.81729-3-mikhail.sennikovskii@ionos.com
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series Reusing nfct handle for bulk ct loads | expand

Commit Message

Mikhail Sennikovsky Aug. 23, 2021, 3:57 p.m. UTC 
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(-)

Comments

Pablo Neira Ayuso Oct. 17, 2021, 1:58 p.m. UTC | #1 
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
diff mbox series

Patch

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;
 }