src: collapse set element commands from parser

498a5f0c219d ("rule: collapse set element commands") does not help to
reduce memory consumption in the case of large sets defined by one
element per line:

 add element ip x y { 1.1.1.1 }
 add element ip x y { 1.1.1.2 }
 ...

This patch reduces memory consumption by ~75%, set elements are
collapsed into an existing cmd object wherever possible to reduce the
number of cmd objects.

This patch also adds a special case for variables for sets similar to:

  be055af5c58d ("cmd: skip variable set elements when collapsing commands")

This patch requires this small kernel fix:

 commit b53c116642502b0c85ecef78bff4f826a7dd4145
 Author: Pablo Neira Ayuso <pablo@netfilter.org>
 Date:   Fri May 20 00:02:06 2022 +0200

    netfilter: nf_tables: set element extended ACK reporting support

which is already included in recent -stable kernels:

 # cat ruleset.nft
 add table ip x
 add chain ip x y
 add set ip x y { type ipv4_addr; }
 create element ip x y { 1.1.1.1 }
 create element ip x y { 1.1.1.1 }

 # nft -f ruleset.nft
 ruleset.nft:5:25-31: Error: Could not process rule: File exists
 create element ip x y { 1.1.1.1 }
                         ^^^^^^^

since there is no need to relate commands via sequence number anymore,
this allows also removes the uncollapse step.

Fixes: 498a5f0c219d ("rule: collapse set element commands")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/include/cmd.h b/include/cmd.h
index 92a4152bbaea85c96896896539a3581dc75d8afd..0a8779b1ea19b4522e693f044b61ba2a5751cf11 100644 (file)
--- a/include/cmd.h
+++ b/include/cmd.h
@@ -2,12 +2,13 @@
 #define _NFT_CMD_H_
 
 void cmd_add_loc(struct cmd *cmd, uint16_t offset, const struct location *loc);
+struct mnl_err;
 void nft_cmd_error(struct netlink_ctx *ctx, struct cmd *cmd,
                   struct mnl_err *err);
 
+bool nft_cmd_collapse_elems(enum cmd_ops op, struct list_head *cmds,
+                           struct handle *handle, struct expr *init);
+
 void nft_cmd_expand(struct cmd *cmd);
-void nft_cmd_post_expand(struct cmd *cmd);
-bool nft_cmd_collapse(struct list_head *cmds);
-void nft_cmd_uncollapse(struct list_head *cmds);
 
 #endif

diff --git a/include/expression.h b/include/expression.h
index 8982110cce95232177c4b2f579ac8d997dca743b..da2f693e72d37bc317d57b463c6edf5a8c32c6dc 100644 (file)
--- a/include/expression.h
+++ b/include/expression.h
@@ -255,7 +255,6 @@ struct expr {
        enum expr_types         etype:8;
        enum ops                op:8;
        unsigned int            len;
-       struct cmd              *cmd;
 
        union {
                struct {

diff --git a/include/list.h b/include/list.h
index 857921e34201f312422b0e83192ec5980b30a3c3..37fbe3e275cc0743d11d8925be1d8ee3092ec982 100644 (file)
--- a/include/list.h
+++ b/include/list.h
@@ -348,6 +348,17 @@ static inline void list_splice_tail_init(struct list_head *list,
 #define list_first_entry(ptr, type, member) \
        list_entry((ptr)->next, type, member)
 
+/**
+ * list_last_entry - get the last element from a list
+ * @ptr:        the list head to take the element from.
+ * @type:       the type of the struct this is embedded in.
+ * @member:     the name of the list_head within the struct.
+ *
+ * Note, that list is expected to be not empty.
+ */
+#define list_last_entry(ptr, type, member) \
+       list_entry((ptr)->prev, type, member)
+
 /**
  * list_next_entry - get the next element in list
  * @pos: the type * to cursor

diff --git a/include/rule.h b/include/rule.h
index 5b3e12b5d7dcff762a741e7ff6bf106f556ef72b..a1628d82d275c2f4ad78df499987faed3638219b 100644 (file)
--- a/include/rule.h
+++ b/include/rule.h
@@ -718,7 +718,6 @@ struct cmd {
        enum cmd_obj            obj;
        struct handle           handle;
        uint32_t                seqnum;
-       struct list_head        collapse_list;
        union {
                void            *data;
                struct expr     *expr;

diff --git a/src/cmd.c b/src/cmd.c
index 9a572b5660c73c29ab487383f4dd76d6c87c0336..e010dcb8113e58864d52bb902ab4425bb251c2e6 100644 (file)
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -378,6 +378,32 @@ static void nft_cmd_expand_chain(struct chain *chain, struct list_head *new_cmds
        }
 }
 
+bool nft_cmd_collapse_elems(enum cmd_ops op, struct list_head *cmds,
+                           struct handle *handle, struct expr *init)
+{
+       struct cmd *last_cmd;
+
+       if (list_empty(cmds))
+               return false;
+
+       if (init->etype == EXPR_VARIABLE)
+               return false;
+
+       last_cmd = list_last_entry(cmds, struct cmd, list);
+       if (last_cmd->op != op ||
+           last_cmd->obj != CMD_OBJ_ELEMENTS ||
+           last_cmd->expr->etype == EXPR_VARIABLE ||
+           last_cmd->handle.family != handle->family ||
+           strcmp(last_cmd->handle.table.name, handle->table.name) ||
+           strcmp(last_cmd->handle.set.name, handle->set.name))
+               return false;
+
+       list_splice_tail_init(&init->expressions, &last_cmd->expr->expressions);
+       last_cmd->expr->size += init->size;
+
+       return true;
+}
+
 void nft_cmd_expand(struct cmd *cmd)
 {
        struct list_head new_cmds;
@@ -459,82 +485,3 @@ void nft_cmd_expand(struct cmd *cmd)
                break;
        }
 }
-
-bool nft_cmd_collapse(struct list_head *cmds)
-{
-       struct cmd *cmd, *next, *elems = NULL;
-       struct expr *expr, *enext;
-       bool collapse = false;
-
-       list_for_each_entry_safe(cmd, next, cmds, list) {
-               if (cmd->op != CMD_ADD &&
-                   cmd->op != CMD_CREATE) {
-                       elems = NULL;
-                       continue;
-               }
-
-               if (cmd->obj != CMD_OBJ_ELEMENTS) {
-                       elems = NULL;
-                       continue;
-               }
-
-               if (cmd->expr->etype == EXPR_VARIABLE)
-                       continue;
-
-               if (!elems) {
-                       elems = cmd;
-                       continue;
-               }
-
-               if (cmd->op != elems->op) {
-                       elems = cmd;
-                       continue;
-               }
-
-               if (elems->handle.family != cmd->handle.family ||
-                   strcmp(elems->handle.table.name, cmd->handle.table.name) ||
-                   strcmp(elems->handle.set.name, cmd->handle.set.name)) {
-                       elems = cmd;
-                       continue;
-               }
-
-               collapse = true;
-               list_for_each_entry_safe(expr, enext, &cmd->expr->expressions, list) {
-                       expr->cmd = cmd;
-                       list_move_tail(&expr->list, &elems->expr->expressions);
-               }
-               elems->expr->size += cmd->expr->size;
-               list_move_tail(&cmd->list, &elems->collapse_list);
-       }
-
-       return collapse;
-}
-
-void nft_cmd_uncollapse(struct list_head *cmds)
-{
-       struct cmd *cmd, *cmd_next, *collapse_cmd, *collapse_cmd_next;
-       struct expr *expr, *next;
-
-       list_for_each_entry_safe(cmd, cmd_next, cmds, list) {
-               if (list_empty(&cmd->collapse_list))
-                       continue;
-
-               assert(cmd->obj == CMD_OBJ_ELEMENTS);
-
-               list_for_each_entry_safe(expr, next, &cmd->expr->expressions, list) {
-                       if (!expr->cmd)
-                               continue;
-
-                       list_move_tail(&expr->list, &expr->cmd->expr->expressions);
-                       cmd->expr->size--;
-                       expr->cmd = NULL;
-               }
-
-               list_for_each_entry_safe(collapse_cmd, collapse_cmd_next, &cmd->collapse_list, list) {
-                       if (cmd->elem.set)
-                               collapse_cmd->elem.set = set_get(cmd->elem.set);
-
-                       list_add(&collapse_cmd->list, &cmd->list);
-               }
-       }
-}

diff --git a/src/libnftables.c b/src/libnftables.c
index 2ae215013cb0a1f9e47145b1a0100268558b75db..2834c9922486d2c4348c2085be1833bb84713fa0 100644 (file)
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -513,7 +513,6 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
 {
        struct nft_cache_filter *filter;
        struct cmd *cmd, *next;
-       bool collapsed = false;
        unsigned int flags;
        int err = 0;
 
@@ -529,9 +528,6 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
 
        nft_cache_filter_fini(filter);
 
-       if (nft_cmd_collapse(cmds))
-               collapsed = true;
-
        list_for_each_entry(cmd, cmds, list) {
                if (cmd->op != CMD_ADD &&
                    cmd->op != CMD_CREATE)
@@ -553,9 +549,6 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
                }
        }
 
-       if (collapsed)
-               nft_cmd_uncollapse(cmds);
-
        if (err < 0 || nft->state->nerrs)
                return -1;
 

diff --git a/src/parser_bison.y b/src/parser_bison.y
index e2936d10efe4c7ae7670aee0420eddb095fe2bd4..602fc60e6de3d537a5d3a5e903682a29918e32a3 100644 (file)
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -35,6 +35,7 @@
 #include <libnftnl/udata.h>
 
 #include <rule.h>
+#include <cmd.h>
 #include <statement.h>
 #include <expression.h>
 #include <headers.h>
@@ -1219,6 +1220,12 @@ add_cmd                  :       TABLE           table_spec
                        }
                        |       ELEMENT         set_spec        set_block_expr
                        {
+                               if (nft_cmd_collapse_elems(CMD_ADD, state->cmds, &$2, $3)) {
+                                       handle_free(&$2);
+                                       expr_free($3);
+                                       $$ = NULL;
+                                       break;
+                               }
                                $$ = cmd_alloc(CMD_ADD, CMD_OBJ_ELEMENTS, &$2, &@$, $3);
                        }
                        |       FLOWTABLE       flowtable_spec  flowtable_block_alloc
@@ -1336,6 +1343,12 @@ create_cmd               :       TABLE           table_spec
                        }
                        |       ELEMENT         set_spec        set_block_expr
                        {
+                               if (nft_cmd_collapse_elems(CMD_CREATE, state->cmds, &$2, $3)) {
+                                       handle_free(&$2);
+                                       expr_free($3);
+                                       $$ = NULL;
+                                       break;
+                               }
                                $$ = cmd_alloc(CMD_CREATE, CMD_OBJ_ELEMENTS, &$2, &@$, $3);
                        }
                        |       FLOWTABLE       flowtable_spec  flowtable_block_alloc

diff --git a/src/rule.c b/src/rule.c
index 9bc160ec0d888d80770b20b066e17bd02159dd36..9536e68c75242a1a69207c29290c190bbcdce465 100644 (file)
--- a/src/rule.c
+++ b/src/rule.c
@@ -1332,7 +1332,6 @@ struct cmd *cmd_alloc(enum cmd_ops op, enum cmd_obj obj,
        cmd->attr     = xzalloc_array(NFT_NLATTR_LOC_MAX,
                                      sizeof(struct nlerr_loc));
        cmd->attr_array_len = NFT_NLATTR_LOC_MAX;
-       init_list_head(&cmd->collapse_list);
 
        return cmd;
 }