[nft PATCH 0/5] Reduce cache overhead a bit

[nft PATCH 0/5] Reduce cache overhead a bit

[Phil Sutter]

Comparing performance of various commands with equivalent iptables ones
I noticed that nftables fetches data from kernel it doesn't need in some
cases. For instance, listing one table was slowed down by a large other
table.

Since there is already code to filter data added to cache, make use of
that and craft GET requests to kernel a bit further so it returns only
what is needed.

This series is not entirely complete, e.g. objects are still fetched as
before. It rather converts some low hanging fruits.

Phil Sutter (5):
  cache: Filter tables on kernel side
  cache: Filter rule list on kernel side
  cache: Filter chain list on kernel side
  cache: Filter set list on server side
  cache: Support filtering for a specific flowtable

 include/cache.h                               |   1 +
 include/mnl.h                                 |  14 +-
 include/netlink.h                             |   3 +-
 src/cache.c                                   | 188 ++++++++++--------
 src/mnl.c                                     |  91 +++++++--
 src/netlink.c                                 |  15 +-
 tests/shell/testcases/listing/0020flowtable_0 |  51 ++++-
 7 files changed, 247 insertions(+), 116 deletions(-)

-- 
2.33.0

[nft PATCH 1/5] cache: Filter tables on kernel side

[Phil Sutter]

Instead of requesting a dump of all tables and filtering the data in
user space, construct a non-dump request if filter contains a table so
kernel returns only that single table.

This should improve nft performance in rulesets with many tables
present.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/mnl.h     |  2 +-
 include/netlink.h |  3 ++-
 src/cache.c       |  9 +--------
 src/mnl.c         | 20 ++++++++++++++++++--
 src/netlink.c     | 12 ++++++++++--
 5 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/include/mnl.h b/include/mnl.h
index 68ec80cd22821..344030f306940 100644
--- a/include/mnl.h
+++ b/include/mnl.h
@@ -50,7 +50,7 @@ int mnl_nft_table_add(struct netlink_ctx *ctx, struct cmd *cmd,
 int mnl_nft_table_del(struct netlink_ctx *ctx, struct cmd *cmd);
 
 struct nftnl_table_list *mnl_nft_table_dump(struct netlink_ctx *ctx,
-					    int family);
+					    int family, const char *table);
 
 int mnl_nft_set_add(struct netlink_ctx *ctx, struct cmd *cmd,
 		    unsigned int flags);
diff --git a/include/netlink.h b/include/netlink.h
index a692edcdb5bf2..0e439061e3800 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -135,7 +135,8 @@ extern int netlink_list_chains(struct netlink_ctx *ctx, const struct handle *h);
 extern struct chain *netlink_delinearize_chain(struct netlink_ctx *ctx,
 					       const struct nftnl_chain *nlc);
 
-extern int netlink_list_tables(struct netlink_ctx *ctx, const struct handle *h);
+extern int netlink_list_tables(struct netlink_ctx *ctx, const struct handle *h,
+			       const struct nft_cache_filter *filter);
 extern struct table *netlink_delinearize_table(struct netlink_ctx *ctx,
 					       const struct nftnl_table *nlt);
 
diff --git a/src/cache.c b/src/cache.c
index 6d20716d73110..66da2b3475732 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -772,19 +772,12 @@ static int cache_init_tables(struct netlink_ctx *ctx, struct handle *h,
 	struct table *table, *next;
 	int ret;
 
-	ret = netlink_list_tables(ctx, h);
+	ret = netlink_list_tables(ctx, h, filter);
 	if (ret < 0)
 		return -1;
 
 	list_for_each_entry_safe(table, next, &ctx->list, list) {
 		list_del(&table->list);
-
-		if (filter && filter->list.table &&
-		    (filter->list.family != table->handle.family ||
-		     strcmp(filter->list.table, table->handle.table.name))) {
-			table_free(table);
-			continue;
-		}
 		table_cache_add(table, cache);
 	}
 
diff --git a/src/mnl.c b/src/mnl.c
index 23348e1393bce..edd951186d3a7 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -1016,10 +1016,12 @@ err_free:
 }
 
 struct nftnl_table_list *mnl_nft_table_dump(struct netlink_ctx *ctx,
-					    int family)
+					    int family, const char *table)
 {
 	char buf[MNL_SOCKET_BUFFER_SIZE];
 	struct nftnl_table_list *nlt_list;
+	struct nftnl_table *nlt = NULL;
+	int flags = NLM_F_DUMP;
 	struct nlmsghdr *nlh;
 	int ret;
 
@@ -1027,8 +1029,22 @@ struct nftnl_table_list *mnl_nft_table_dump(struct netlink_ctx *ctx,
 	if (nlt_list == NULL)
 		return NULL;
 
+	if (table) {
+		nlt = nftnl_table_alloc();
+		if (!nlt)
+			memory_allocation_error();
+
+		nftnl_table_set_u32(nlt, NFTNL_TABLE_FAMILY, family);
+		nftnl_table_set_str(nlt, NFTNL_TABLE_NAME, table);
+		flags = NLM_F_ACK;
+	}
+
 	nlh = nftnl_nlmsg_build_hdr(buf, NFT_MSG_GETTABLE, family,
-				    NLM_F_DUMP, ctx->seqnum);
+				    flags, ctx->seqnum);
+	if (nlt) {
+		nftnl_table_nlmsg_build_payload(nlh, nlt);
+		nftnl_table_free(nlt);
+	}
 
 	ret = nft_mnl_talk(ctx, nlh, nlh->nlmsg_len, table_cb, nlt_list);
 	if (ret < 0)
diff --git a/src/netlink.c b/src/netlink.c
index ab90d0c05acaf..f74c0383a0db3 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -664,11 +664,19 @@ static int list_table_cb(struct nftnl_table *nlt, void *arg)
 	return 0;
 }
 
-int netlink_list_tables(struct netlink_ctx *ctx, const struct handle *h)
+int netlink_list_tables(struct netlink_ctx *ctx, const struct handle *h,
+			const struct nft_cache_filter *filter)
 {
 	struct nftnl_table_list *table_cache;
+	uint32_t family = h->family;
+	const char *table = NULL;
 
-	table_cache = mnl_nft_table_dump(ctx, h->family);
+	if (filter) {
+		family = filter->list.family;
+		table = filter->list.table;
+	}
+
+	table_cache = mnl_nft_table_dump(ctx, family, table);
 	if (table_cache == NULL) {
 		if (errno == EINTR)
 			return -1;
-- 
2.33.0

[nft PATCH 2/5] cache: Filter rule list on kernel side

[Phil Sutter]

Instead of fetching all existing rules in kernel's ruleset and filtering
in user space, add payload to the dump request specifying the table and
chain to filter for.

Since list_rule_cb() no longer needs the filter, pass only netlink_ctx
to the callback and drop struct rule_cache_dump_ctx.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/mnl.h |  4 ++--
 src/cache.c   | 23 +++--------------------
 src/mnl.c     | 21 +++++++++++++++++++--
 3 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/include/mnl.h b/include/mnl.h
index 344030f306940..19faa651fdb91 100644
--- a/include/mnl.h
+++ b/include/mnl.h
@@ -33,8 +33,8 @@ int mnl_nft_rule_add(struct netlink_ctx *ctx, struct cmd *cmd,
 int mnl_nft_rule_del(struct netlink_ctx *ctx, struct cmd *cmd);
 int mnl_nft_rule_replace(struct netlink_ctx *ctx, struct cmd *cmd);
 
-struct nftnl_rule_list *mnl_nft_rule_dump(struct netlink_ctx *ctx,
-					  int family);
+struct nftnl_rule_list *mnl_nft_rule_dump(struct netlink_ctx *ctx, int family,
+					  const struct nft_cache_filter *filter);
 
 int mnl_nft_chain_add(struct netlink_ctx *ctx, struct cmd *cmd,
 		      unsigned int flags);
diff --git a/src/cache.c b/src/cache.c
index 66da2b3475732..484efdb93862b 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -441,16 +441,9 @@ struct chain *chain_cache_find(const struct table *table, const char *name)
 	return NULL;
 }
 
-struct rule_cache_dump_ctx {
-	struct netlink_ctx	*nlctx;
-	const struct nft_cache_filter *filter;
-};
-
 static int list_rule_cb(struct nftnl_rule *nlr, void *data)
 {
-	struct rule_cache_dump_ctx *rule_ctx = data;
-	const struct nft_cache_filter *filter = rule_ctx->filter;
-	struct netlink_ctx *ctx = rule_ctx->nlctx;
+	struct netlink_ctx *ctx = data;
 	const struct handle *h = ctx->data;
 	const char *table, *chain;
 	struct rule *rule;
@@ -465,12 +458,6 @@ static int list_rule_cb(struct nftnl_rule *nlr, void *data)
 	    (h->chain.name && strcmp(chain, h->chain.name) != 0))
 		return 0;
 
-	if (filter && filter->list.table && filter->list.chain &&
-	    (filter->list.family != family ||
-	     strcmp(filter->list.table, table) ||
-	     strcmp(filter->list.chain, chain)))
-		return 0;
-
 	netlink_dump_rule(nlr, ctx);
 	rule = netlink_delinearize_rule(ctx, nlr);
 	list_add_tail(&rule->list, &ctx->list);
@@ -481,13 +468,9 @@ static int list_rule_cb(struct nftnl_rule *nlr, void *data)
 static int rule_cache_init(struct netlink_ctx *ctx, const struct handle *h,
 			   const struct nft_cache_filter *filter)
 {
-	struct rule_cache_dump_ctx rule_ctx = {
-		.nlctx = ctx,
-		.filter = filter,
-	};
 	struct nftnl_rule_list *rule_cache;
 
-	rule_cache = mnl_nft_rule_dump(ctx, h->family);
+	rule_cache = mnl_nft_rule_dump(ctx, h->family, filter);
 	if (rule_cache == NULL) {
 		if (errno == EINTR)
 			return -1;
@@ -496,7 +479,7 @@ static int rule_cache_init(struct netlink_ctx *ctx, const struct handle *h,
 	}
 
 	ctx->data = h;
-	nftnl_rule_list_foreach(rule_cache, list_rule_cb, &rule_ctx);
+	nftnl_rule_list_foreach(rule_cache, list_rule_cb, ctx);
 	nftnl_rule_list_free(rule_cache);
 	return 0;
 }
diff --git a/src/mnl.c b/src/mnl.c
index edd951186d3a7..35fff92a180c7 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -653,20 +653,37 @@ err_free:
 	return MNL_CB_OK;
 }
 
-struct nftnl_rule_list *mnl_nft_rule_dump(struct netlink_ctx *ctx,
-					  int family)
+struct nftnl_rule_list *mnl_nft_rule_dump(struct netlink_ctx *ctx, int family,
+					  const struct nft_cache_filter *filter)
 {
 	char buf[MNL_SOCKET_BUFFER_SIZE];
 	struct nftnl_rule_list *nlr_list;
+	struct nftnl_rule *nlr = NULL;
 	struct nlmsghdr *nlh;
 	int ret;
 
+	if (filter && filter->list.table) {
+		nlr = nftnl_rule_alloc();
+		if (!nlr)
+			memory_allocation_error();
+
+		nftnl_rule_set_str(nlr, NFTNL_RULE_TABLE,
+				   filter->list.table);
+		if (filter->list.chain)
+			nftnl_rule_set_str(nlr, NFTNL_RULE_CHAIN,
+					   filter->list.chain);
+	}
+
 	nlr_list = nftnl_rule_list_alloc();
 	if (nlr_list == NULL)
 		memory_allocation_error();
 
 	nlh = nftnl_nlmsg_build_hdr(buf, NFT_MSG_GETRULE, family,
 				    NLM_F_DUMP, ctx->seqnum);
+	if (nlr) {
+		nftnl_rule_nlmsg_build_payload(nlh, nlr);
+		nftnl_rule_free(nlr);
+	}
 
 	ret = nft_mnl_talk(ctx, nlh, nlh->nlmsg_len, rule_cb, nlr_list);
 	if (ret < 0)
-- 
2.33.0

[nft PATCH 3/5] cache: Filter chain list on kernel side

[Phil Sutter]

When operating on a specific chain, add payload to NFT_MSG_GETCHAIN so
kernel returns only relevant data. Since in non-dump mode kernel returns
error if the requested item was not found, ignore errors then - chain
not found is handled anyway and kernel returns just EINVAL so no point
in checking what exactly was returned.

While being at it, improve code in chain_cache_cb() a bit:
- Check chain's family first, it is a less expensive check than
  comparing table names.
- Do not extract chain name of uninteresting chains.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/mnl.h |  3 ++-
 src/cache.c   | 38 +++++++++++++++++++-------------------
 src/mnl.c     | 21 ++++++++++++++++++---
 3 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/include/mnl.h b/include/mnl.h
index 19faa651fdb91..9d54aac876dc1 100644
--- a/include/mnl.h
+++ b/include/mnl.h
@@ -43,7 +43,8 @@ int mnl_nft_chain_rename(struct netlink_ctx *ctx, const struct cmd *cmd,
 			 const struct chain *chain);
 
 struct nftnl_chain_list *mnl_nft_chain_dump(struct netlink_ctx *ctx,
-					    int family);
+					    int family, const char *table,
+					    const char *chain);
 
 int mnl_nft_table_add(struct netlink_ctx *ctx, struct cmd *cmd,
 		      unsigned int flags);
diff --git a/src/cache.c b/src/cache.c
index 484efdb93862b..3ecea38b550d4 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -342,31 +342,23 @@ struct table *table_cache_find(const struct cache *cache,
 struct chain_cache_dump_ctx {
 	struct netlink_ctx	*nlctx;
 	struct table		*table;
-	const struct nft_cache_filter *filter;
 };
 
 static int chain_cache_cb(struct nftnl_chain *nlc, void *arg)
 {
 	struct chain_cache_dump_ctx *ctx = arg;
-	const struct nft_cache_filter *filter = ctx->filter;
 	const char *chain_name, *table_name;
 	uint32_t hash, family;
 	struct chain *chain;
 
 	table_name = nftnl_chain_get_str(nlc, NFTNL_CHAIN_TABLE);
-	chain_name = nftnl_chain_get_str(nlc, NFTNL_CHAIN_NAME);
 	family = nftnl_chain_get_u32(nlc, NFTNL_CHAIN_FAMILY);
 
-	if (strcmp(table_name, ctx->table->handle.table.name) ||
-	    family != ctx->table->handle.family)
-		return 0;
-
-	if (filter && filter->list.table && filter->list.chain &&
-	    (filter->list.family != family ||
-	     strcmp(filter->list.table, table_name) ||
-	     strcmp(filter->list.chain, chain_name)))
+	if (family != ctx->table->handle.family ||
+	    strcmp(table_name, ctx->table->handle.table.name))
 		return 0;
 
+	chain_name = nftnl_chain_get_str(nlc, NFTNL_CHAIN_NAME);
 	hash = djb_hash(chain_name) % NFT_CACHE_HSIZE;
 	chain = netlink_delinearize_chain(ctx->nlctx, nlc);
 
@@ -383,25 +375,33 @@ static int chain_cache_cb(struct nftnl_chain *nlc, void *arg)
 }
 
 static int chain_cache_init(struct netlink_ctx *ctx, struct table *table,
-			    struct nftnl_chain_list *chain_list,
-			    const struct nft_cache_filter *filter)
+			    struct nftnl_chain_list *chain_list)
 {
 	struct chain_cache_dump_ctx dump_ctx = {
 		.nlctx	= ctx,
 		.table	= table,
-		.filter	= filter,
 	};
 	nftnl_chain_list_foreach(chain_list, chain_cache_cb, &dump_ctx);
 
 	return 0;
 }
 
-static struct nftnl_chain_list *chain_cache_dump(struct netlink_ctx *ctx,
-						 int *err)
+static struct nftnl_chain_list *
+chain_cache_dump(struct netlink_ctx *ctx,
+		 const struct nft_cache_filter *filter, int *err)
 {
 	struct nftnl_chain_list *chain_list;
+	const char *table = NULL;
+	const char *chain = NULL;
+	int family = AF_UNSPEC;
+
+	if (filter && filter->list.table && filter->list.chain) {
+		family = filter->list.family;
+		table = filter->list.table;
+		chain = filter->list.chain;
+	}
 
-	chain_list = mnl_nft_chain_dump(ctx, AF_UNSPEC);
+	chain_list = mnl_nft_chain_dump(ctx, family, table, chain);
 	if (chain_list == NULL) {
 		if (errno == EINTR) {
 			*err = -1;
@@ -781,7 +781,7 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags,
 	int ret = 0;
 
 	if (flags & NFT_CACHE_CHAIN_BIT) {
-		chain_list = chain_cache_dump(ctx, &ret);
+		chain_list = chain_cache_dump(ctx, filter, &ret);
 		if (!chain_list)
 			return -1;
 	}
@@ -834,7 +834,7 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags,
 			}
 		}
 		if (flags & NFT_CACHE_CHAIN_BIT) {
-			ret = chain_cache_init(ctx, table, chain_list, filter);
+			ret = chain_cache_init(ctx, table, chain_list);
 			if (ret < 0) {
 				ret = -1;
 				goto cache_fails;
diff --git a/src/mnl.c b/src/mnl.c
index 35fff92a180c7..03484c03d9ff8 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -906,10 +906,12 @@ err_free:
 }
 
 struct nftnl_chain_list *mnl_nft_chain_dump(struct netlink_ctx *ctx,
-					    int family)
+					    int family, const char *table,
+					    const char *chain)
 {
 	char buf[MNL_SOCKET_BUFFER_SIZE];
 	struct nftnl_chain_list *nlc_list;
+	struct nftnl_chain *nlc = NULL;
 	struct nlmsghdr *nlh;
 	int ret;
 
@@ -917,11 +919,24 @@ struct nftnl_chain_list *mnl_nft_chain_dump(struct netlink_ctx *ctx,
 	if (nlc_list == NULL)
 		memory_allocation_error();
 
+	if (table && chain) {
+		nlc = nftnl_chain_alloc();
+		if (!nlc)
+			memory_allocation_error();
+
+		nftnl_chain_set_str(nlc, NFTNL_CHAIN_TABLE, table);
+		nftnl_chain_set_str(nlc, NFTNL_CHAIN_NAME, chain);
+	}
+
 	nlh = nftnl_nlmsg_build_hdr(buf, NFT_MSG_GETCHAIN, family,
-				    NLM_F_DUMP, ctx->seqnum);
+				    nlc ? NLM_F_ACK : NLM_F_DUMP, ctx->seqnum);
+	if (nlc) {
+		nftnl_chain_nlmsg_build_payload(nlh, nlc);
+		nftnl_chain_free(nlc);
+	}
 
 	ret = nft_mnl_talk(ctx, nlh, nlh->nlmsg_len, chain_cb, nlc_list);
-	if (ret < 0)
+	if (ret < 0 && !nlc)
 		goto err;
 
 	return nlc_list;
-- 
2.33.0

[nft PATCH 4/5] cache: Filter set list on server side

[Phil Sutter]

Fetch either all table's sets at once, a specific table's sets or even a
specific set if needed instead of iterating over the list of previously
fetched tables and fetching for each, then ignoring anything returned
that doesn't match the filter.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/mnl.h |  2 +-
 src/cache.c   | 63 ++++++++++++++++++++++++++++++---------------------
 src/mnl.c     | 15 ++++++++----
 3 files changed, 49 insertions(+), 31 deletions(-)

diff --git a/include/mnl.h b/include/mnl.h
index 9d54aac876dc1..8659879c17f44 100644
--- a/include/mnl.h
+++ b/include/mnl.h
@@ -58,7 +58,7 @@ int mnl_nft_set_add(struct netlink_ctx *ctx, struct cmd *cmd,
 int mnl_nft_set_del(struct netlink_ctx *ctx, struct cmd *cmd);
 
 struct nftnl_set_list *mnl_nft_set_dump(struct netlink_ctx *ctx, int family,
-					const char *table);
+					const char *table, const char *set);
 
 int mnl_nft_setelem_add(struct netlink_ctx *ctx, const struct set *set,
 			const struct expr *expr, unsigned int flags);
diff --git a/src/cache.c b/src/cache.c
index 3ecea38b550d4..b0f13e7ebb16a 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -487,57 +487,66 @@ static int rule_cache_init(struct netlink_ctx *ctx, const struct handle *h,
 struct set_cache_dump_ctx {
 	struct netlink_ctx	*nlctx;
 	struct table		*table;
-	const struct nft_cache_filter *filter;
 };
 
 static int set_cache_cb(struct nftnl_set *nls, void *arg)
 {
 	struct set_cache_dump_ctx *ctx = arg;
+	const char *set_table;
 	const char *set_name;
+	uint32_t set_family;
 	struct set *set;
 	uint32_t hash;
 
+	set_table = nftnl_set_get_str(nls, NFTNL_SET_TABLE);
+	set_family = nftnl_set_get_u32(nls, NFTNL_SET_FAMILY);
+
+	if (set_family != ctx->table->handle.family ||
+	    strcmp(set_table, ctx->table->handle.table.name))
+		return 0;
+
 	set = netlink_delinearize_set(ctx->nlctx, nls);
 	if (!set)
 		return -1;
 
-	if (ctx->filter && ctx->filter->list.set &&
-	    (ctx->filter->list.family != set->handle.family ||
-	     strcmp(ctx->filter->list.table, set->handle.table.name) ||
-	     strcmp(ctx->filter->list.set, set->handle.set.name))) {
-		set_free(set);
-		return 0;
-	}
-
 	set_name = nftnl_set_get_str(nls, NFTNL_SET_NAME);
 	hash = djb_hash(set_name) % NFT_CACHE_HSIZE;
 	cache_add(&set->cache, &ctx->table->set_cache, hash);
 
+	nftnl_set_list_del(nls);
+	nftnl_set_free(nls);
 	return 0;
 }
 
 static int set_cache_init(struct netlink_ctx *ctx, struct table *table,
-			  struct nftnl_set_list *set_list,
-			  const struct nft_cache_filter *filter)
+			  struct nftnl_set_list *set_list)
 {
 	struct set_cache_dump_ctx dump_ctx = {
 		.nlctx	= ctx,
 		.table	= table,
-		.filter = filter,
 	};
+
 	nftnl_set_list_foreach(set_list, set_cache_cb, &dump_ctx);
 
 	return 0;
 }
 
-static struct nftnl_set_list *set_cache_dump(struct netlink_ctx *ctx,
-					     const struct table *table,
-					     int *err)
+static struct nftnl_set_list *
+set_cache_dump(struct netlink_ctx *ctx,
+	       const struct nft_cache_filter *filter, int *err)
 {
 	struct nftnl_set_list *set_list;
+	int family = NFPROTO_UNSPEC;
+	const char *table = NULL;
+	const char *set = NULL;
 
-	set_list = mnl_nft_set_dump(ctx, table->handle.family,
-				    table->handle.table.name);
+	if (filter) {
+		family = filter->list.family;
+		table = filter->list.table;
+		set = filter->list.set;
+	}
+
+	set_list = mnl_nft_set_dump(ctx, family, table, set);
 	if (!set_list) {
                 if (errno == EINTR) {
 			*err = -1;
@@ -785,18 +794,17 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags,
 		if (!chain_list)
 			return -1;
 	}
+	if (flags & NFT_CACHE_SET_BIT) {
+		set_list = set_cache_dump(ctx, filter, &ret);
+		if (!set_list) {
+			ret = -1;
+			goto cache_fails;
+		}
+	}
 
 	list_for_each_entry(table, &ctx->nft->cache.table_cache.list, cache.list) {
 		if (flags & NFT_CACHE_SET_BIT) {
-			set_list = set_cache_dump(ctx, table, &ret);
-			if (!set_list) {
-				ret = -1;
-				goto cache_fails;
-			}
-			ret = set_cache_init(ctx, table, set_list, filter);
-
-			nftnl_set_list_free(set_list);
-
+			ret = set_cache_init(ctx, table, set_list);
 			if (ret < 0) {
 				ret = -1;
 				goto cache_fails;
@@ -893,6 +901,9 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags,
 	}
 
 cache_fails:
+	if (set_list)
+		nftnl_set_list_free(set_list);
+
 	if (flags & NFT_CACHE_CHAIN_BIT)
 		nftnl_chain_list_free(chain_list);
 
diff --git a/src/mnl.c b/src/mnl.c
index 03484c03d9ff8..9a98eed1ed9db 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -1300,10 +1300,12 @@ err_free:
 }
 
 struct nftnl_set_list *
-mnl_nft_set_dump(struct netlink_ctx *ctx, int family, const char *table)
+mnl_nft_set_dump(struct netlink_ctx *ctx, int family,
+		 const char *table, const char *set)
 {
 	char buf[MNL_SOCKET_BUFFER_SIZE];
 	struct nftnl_set_list *nls_list;
+	int flags = NLM_F_DUMP;
 	struct nlmsghdr *nlh;
 	struct nftnl_set *s;
 	int ret;
@@ -1312,10 +1314,15 @@ mnl_nft_set_dump(struct netlink_ctx *ctx, int family, const char *table)
 	if (s == NULL)
 		memory_allocation_error();
 
-	nlh = nftnl_nlmsg_build_hdr(buf, NFT_MSG_GETSET, family,
-				    NLM_F_DUMP, ctx->seqnum);
 	if (table != NULL)
 		nftnl_set_set_str(s, NFTNL_SET_TABLE, table);
+	if (set) {
+		nftnl_set_set_str(s, NFTNL_SET_NAME, set);
+		flags = NLM_F_ACK;
+	}
+
+	nlh = nftnl_nlmsg_build_hdr(buf, NFT_MSG_GETSET, family,
+				    flags, ctx->seqnum);
 	nftnl_set_nlmsg_build_payload(nlh, s);
 	nftnl_set_free(s);
 
@@ -1324,7 +1331,7 @@ mnl_nft_set_dump(struct netlink_ctx *ctx, int family, const char *table)
 		memory_allocation_error();
 
 	ret = nft_mnl_talk(ctx, nlh, nlh->nlmsg_len, set_cb, nls_list);
-	if (ret < 0)
+	if (ret < 0 && !set)
 		goto err;
 
 	return nls_list;
-- 
2.33.0

[nft PATCH 5/5] cache: Support filtering for a specific flowtable

[Phil Sutter]

Extend nft_cache_filter to hold a flowtable name so 'list flowtable'
command causes fetching the requested flowtable only.

Dump flowtables just once instead of for each table, merely assign
fetched data to tables inside the loop.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/cache.h                               |  1 +
 include/mnl.h                                 |  3 +-
 src/cache.c                                   | 55 ++++++++++++++-----
 src/mnl.c                                     | 14 +++--
 src/netlink.c                                 |  3 +-
 tests/shell/testcases/listing/0020flowtable_0 | 51 +++++++++++++++--
 6 files changed, 103 insertions(+), 24 deletions(-)

diff --git a/include/cache.h b/include/cache.h
index 3a9a5e8197114..d185f3cfeda06 100644
--- a/include/cache.h
+++ b/include/cache.h
@@ -54,6 +54,7 @@ struct nft_cache_filter {
 		const char	*table;
 		const char	*chain;
 		const char	*set;
+		const char	*ft;
 	} list;
 
 	struct {
diff --git a/include/mnl.h b/include/mnl.h
index 8659879c17f44..b006192cf7b23 100644
--- a/include/mnl.h
+++ b/include/mnl.h
@@ -77,7 +77,8 @@ int mnl_nft_obj_add(struct netlink_ctx *ctx, struct cmd *cmd,
 int mnl_nft_obj_del(struct netlink_ctx *ctx, struct cmd *cmd, int type);
 
 struct nftnl_flowtable_list *
-mnl_nft_flowtable_dump(struct netlink_ctx *ctx, int family, const char *table);
+mnl_nft_flowtable_dump(struct netlink_ctx *ctx, int family,
+		       const char *table, const char *ft);
 
 int mnl_nft_flowtable_add(struct netlink_ctx *ctx, struct cmd *cmd,
 			  unsigned int flags);
diff --git a/src/cache.c b/src/cache.c
index b0f13e7ebb16a..71bd06169a7d1 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -229,6 +229,15 @@ static unsigned int evaluate_cache_list(struct nft_ctx *nft, struct cmd *cmd,
 	case CMD_OBJ_MAPS:
 		flags |= NFT_CACHE_TABLE | NFT_CACHE_SET;
 		break;
+	case CMD_OBJ_FLOWTABLE:
+		if (filter &&
+		    cmd->handle.table.name &&
+		    cmd->handle.flowtable.name) {
+			filter->list.family = cmd->handle.family;
+			filter->list.table = cmd->handle.table.name;
+			filter->list.ft = cmd->handle.flowtable.name;
+		}
+		/* fall through */
 	case CMD_OBJ_FLOWTABLES:
 		flags |= NFT_CACHE_TABLE | NFT_CACHE_FLOWTABLE;
 		break;
@@ -681,10 +690,19 @@ struct ft_cache_dump_ctx {
 static int ft_cache_cb(struct nftnl_flowtable *nlf, void *arg)
 {
 	struct ft_cache_dump_ctx *ctx = arg;
-	const char *ft_name;
 	struct flowtable *ft;
+	const char *ft_table;
+	const char *ft_name;
+	uint32_t ft_family;
 	uint32_t hash;
 
+	ft_family = nftnl_flowtable_get_u32(nlf, NFTNL_FLOWTABLE_FAMILY);
+	ft_table = nftnl_flowtable_get_str(nlf, NFTNL_FLOWTABLE_TABLE);
+
+	if (ft_family != ctx->table->handle.family ||
+	    strcmp(ft_table, ctx->table->handle.table.name))
+		return 0;
+
 	ft = netlink_delinearize_flowtable(ctx->nlctx, nlf);
 	if (!ft)
 		return -1;
@@ -693,6 +711,8 @@ static int ft_cache_cb(struct nftnl_flowtable *nlf, void *arg)
 	hash = djb_hash(ft_name) % NFT_CACHE_HSIZE;
 	cache_add(&ft->cache, &ctx->table->ft_cache, hash);
 
+	nftnl_flowtable_list_del(nlf);
+	nftnl_flowtable_free(nlf);
 	return 0;
 }
 
@@ -708,13 +728,21 @@ static int ft_cache_init(struct netlink_ctx *ctx, struct table *table,
 	return 0;
 }
 
-static struct nftnl_flowtable_list *ft_cache_dump(struct netlink_ctx *ctx,
-						  const struct table *table)
+static struct nftnl_flowtable_list *
+ft_cache_dump(struct netlink_ctx *ctx, const struct nft_cache_filter *filter)
 {
 	struct nftnl_flowtable_list *ft_list;
+	int family = NFPROTO_UNSPEC;
+	const char *table = NULL;
+	const char *ft = NULL;
 
-	ft_list = mnl_nft_flowtable_dump(ctx, table->handle.family,
-					 table->handle.table.name);
+	if (filter) {
+		family = filter->list.family;
+		table = filter->list.table;
+		ft = filter->list.ft;
+	}
+
+	ft_list = mnl_nft_flowtable_dump(ctx, family, table, ft);
 	if (!ft_list) {
                 if (errno == EINTR)
 			return NULL;
@@ -801,6 +829,13 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags,
 			goto cache_fails;
 		}
 	}
+	if (flags & NFT_CACHE_FLOWTABLE_BIT) {
+		ft_list = ft_cache_dump(ctx, filter);
+		if (!ft_list) {
+			ret = -1;
+			goto cache_fails;
+		}
+	}
 
 	list_for_each_entry(table, &ctx->nft->cache.table_cache.list, cache.list) {
 		if (flags & NFT_CACHE_SET_BIT) {
@@ -849,15 +884,7 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags,
 			}
 		}
 		if (flags & NFT_CACHE_FLOWTABLE_BIT) {
-			ft_list = ft_cache_dump(ctx, table);
-			if (!ft_list) {
-				ret = -1;
-				goto cache_fails;
-			}
 			ret = ft_cache_init(ctx, table, ft_list);
-
-			nftnl_flowtable_list_free(ft_list);
-
 			if (ret < 0) {
 				ret = -1;
 				goto cache_fails;
@@ -903,6 +930,8 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags,
 cache_fails:
 	if (set_list)
 		nftnl_set_list_free(set_list);
+	if (ft_list)
+		nftnl_flowtable_list_free(ft_list);
 
 	if (flags & NFT_CACHE_CHAIN_BIT)
 		nftnl_chain_list_free(chain_list);
diff --git a/src/mnl.c b/src/mnl.c
index 9a98eed1ed9db..9a97a3965d5b5 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -1826,11 +1826,13 @@ err_free:
 }
 
 struct nftnl_flowtable_list *
-mnl_nft_flowtable_dump(struct netlink_ctx *ctx, int family, const char *table)
+mnl_nft_flowtable_dump(struct netlink_ctx *ctx, int family,
+		       const char *table, const char *ft)
 {
 	struct nftnl_flowtable_list *nln_list;
 	char buf[MNL_SOCKET_BUFFER_SIZE];
 	struct nftnl_flowtable *n;
+	int flags = NLM_F_DUMP;
 	struct nlmsghdr *nlh;
 	int ret;
 
@@ -1838,10 +1840,14 @@ mnl_nft_flowtable_dump(struct netlink_ctx *ctx, int family, const char *table)
 	if (n == NULL)
 		memory_allocation_error();
 
-	nlh = nftnl_nlmsg_build_hdr(buf, NFT_MSG_GETFLOWTABLE, family,
-				    NLM_F_DUMP, ctx->seqnum);
 	if (table != NULL)
 		nftnl_flowtable_set_str(n, NFTNL_FLOWTABLE_TABLE, table);
+	if (ft) {
+		nftnl_flowtable_set_str(n, NFTNL_FLOWTABLE_NAME, ft);
+		flags = NLM_F_ACK;
+	}
+	nlh = nftnl_nlmsg_build_hdr(buf, NFT_MSG_GETFLOWTABLE, family,
+				    flags, ctx->seqnum);
 	nftnl_flowtable_nlmsg_build_payload(nlh, n);
 	nftnl_flowtable_free(n);
 
@@ -1850,7 +1856,7 @@ mnl_nft_flowtable_dump(struct netlink_ctx *ctx, int family, const char *table)
 		memory_allocation_error();
 
 	ret = nft_mnl_talk(ctx, nlh, nlh->nlmsg_len, flowtable_cb, nln_list);
-	if (ret < 0)
+	if (ret < 0 && !ft)
 		goto err;
 
 	return nln_list;
diff --git a/src/netlink.c b/src/netlink.c
index f74c0383a0db3..359d801c29d35 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -1721,7 +1721,8 @@ int netlink_list_flowtables(struct netlink_ctx *ctx, const struct handle *h)
 	struct nftnl_flowtable_list *flowtable_cache;
 	int err;
 
-	flowtable_cache = mnl_nft_flowtable_dump(ctx, h->family, h->table.name);
+	flowtable_cache = mnl_nft_flowtable_dump(ctx, h->family,
+						 h->table.name, NULL);
 	if (flowtable_cache == NULL) {
 		if (errno == EINTR)
 			return -1;
diff --git a/tests/shell/testcases/listing/0020flowtable_0 b/tests/shell/testcases/listing/0020flowtable_0
index 2f0a98d16fd38..47488d8ea92a4 100755
--- a/tests/shell/testcases/listing/0020flowtable_0
+++ b/tests/shell/testcases/listing/0020flowtable_0
@@ -2,19 +2,60 @@
 
 # list only the flowtable asked for with table
 
+FLOWTABLES="flowtable f {
+	hook ingress priority filter
+	devices = { lo }
+}
+flowtable f2 {
+	hook ingress priority filter
+	devices = { d0 }
+}"
+
+RULESET="table inet filter {
+	$FLOWTABLES
+}
+table ip filter {
+	$FLOWTABLES
+}"
+
 EXPECTED="table inet filter {
 	flowtable f {
 		hook ingress priority filter
 		devices = { lo }
 	}
 }"
+EXPECTED2="table ip filter {
+	flowtable f2 {
+		hook ingress priority filter
+		devices = { d0 }
+	}
+}"
+EXPECTED3="table ip filter {
+	flowtable f {
+		hook ingress priority filter
+		devices = { lo }
+	}
+	flowtable f2 {
+		hook ingress priority filter
+		devices = { d0 }
+	}
+}"
+
+ip link add d0 type dummy || {
+	echo "Skipping, no dummy interface available"
+	exit 0
+}
+trap "ip link del d0" EXIT
 
 set -e
 
-$NFT -f - <<< "$EXPECTED"
+$NFT -f - <<< "$RULESET"
 
 GET="$($NFT list flowtable inet filter f)"
-if [ "$EXPECTED" != "$GET" ] ; then
-	$DIFF -u <(echo "$EXPECTED") <(echo "$GET")
-	exit 1
-fi
+$DIFF -u <(echo "$EXPECTED") <(echo "$GET")
+
+GET="$($NFT list flowtable ip filter f2)"
+$DIFF -u <(echo "$EXPECTED2") <(echo "$GET")
+
+GET="$($NFT list flowtables ip)"
+$DIFF -u <(echo "$EXPECTED3") <(echo "$GET")
-- 
2.33.0

Re: [nft PATCH 0/5] Reduce cache overhead a bit

[Pablo Neira Ayuso]

Hi Phil,

On Wed, Dec 01, 2021 at 04:02:53PM +0100, Phil Sutter wrote:
> Comparing performance of various commands with equivalent iptables ones
> I noticed that nftables fetches data from kernel it doesn't need in some
> cases. For instance, listing one table was slowed down by a large other
> table.
> 
> Since there is already code to filter data added to cache, make use of
> that and craft GET requests to kernel a bit further so it returns only
> what is needed.

Using netlink to filter from kernel space is the optimal solution.

> This series is not entirely complete, e.g. objects are still fetched as
> before. It rather converts some low hanging fruits.

Only one thing: It would be good to test this on older kernels,
because IIRC some of the GET requests during the development, I would
suggest to give it a test with -stable kernels. Probably all of the
needed GET commands are already present there.

In the nftables 1.0.1 release process, I tested it with 4.9.x and
tests where running fine, the error reports were coming from missing
features.

Thanks.

Re: [nft PATCH 0/5] Reduce cache overhead a bit

[Phil Sutter]

On Wed, Dec 01, 2021 at 05:48:40PM +0100, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> On Wed, Dec 01, 2021 at 04:02:53PM +0100, Phil Sutter wrote:
> > Comparing performance of various commands with equivalent iptables ones
> > I noticed that nftables fetches data from kernel it doesn't need in some
> > cases. For instance, listing one table was slowed down by a large other
> > table.
> > 
> > Since there is already code to filter data added to cache, make use of
> > that and craft GET requests to kernel a bit further so it returns only
> > what is needed.
> 
> Using netlink to filter from kernel space is the optimal solution.

I was basically copying from iptables-nft. :)

> > This series is not entirely complete, e.g. objects are still fetched as
> > before. It rather converts some low hanging fruits.
> 
> Only one thing: It would be good to test this on older kernels,
> because IIRC some of the GET requests during the development, I would
> suggest to give it a test with -stable kernels. Probably all of the
> needed GET commands are already present there.

Good point, thanks. I'll check and report.

> In the nftables 1.0.1 release process, I tested it with 4.9.x and
> tests where running fine, the error reports were coming from missing
> features.

If ENOENT wasn't reported as EINVAL, We could even fall back to plain
NLM_F_DUMP on older kernels. Maybe tackle that first and build upon
that?

Cheers, Phil

Re: [nft PATCH 0/5] Reduce cache overhead a bit

[Phil Sutter]

Quick follow-up:

On Wed, Dec 01, 2021 at 06:18:57PM +0100, Phil Sutter wrote:
> If ENOENT wasn't reported as EINVAL, We could even fall back to plain
> NLM_F_DUMP on older kernels. Maybe tackle that first and build upon
> that?

Not sure what I was seeing, but I checked again and ENOENT is indeed
returned as it should. I'll look into the fall back idea then.

Sorry for the confusion,

Phil

Re: [nft PATCH 0/5] Reduce cache overhead a bit

[Pablo Neira Ayuso]

On Wed, Dec 01, 2021 at 07:02:56PM +0100, Phil Sutter wrote:
> Quick follow-up:
> 
> On Wed, Dec 01, 2021 at 06:18:57PM +0100, Phil Sutter wrote:
> > If ENOENT wasn't reported as EINVAL, We could even fall back to plain
> > NLM_F_DUMP on older kernels. Maybe tackle that first and build upon
> > that?
> 
> Not sure what I was seeing, but I checked again and ENOENT is indeed
> returned as it should. I'll look into the fall back idea then.

Is the fallback really needed? As I said, if -stable kernel works with
this approach, I think it should be fine.