[PATCH nft] evaluate: Remove cache_update() in cmd_evaluate_flush()

[PATCH nft] evaluate: Remove cache_update() in cmd_evaluate_flush()

[Elise Lennion]

cache_update() fetches set elements, when the set is big and sorted
this leads to an unnecessary delay on 'nft flush ruleset'.

There is only a possible call to cache_flush() after the update, so
this update isn't needed.

Signed-off-by: Elise Lennion <elise.lennion@gmail.com>
---
 src/evaluate.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index cebc5a9..a506f9a 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2951,12 +2951,6 @@ static int cmd_evaluate_list(struct eval_ctx *ctx, struct cmd *cmd)
 
 static int cmd_evaluate_flush(struct eval_ctx *ctx, struct cmd *cmd)
 {
-	int ret;
-
-	ret = cache_update(cmd->op, ctx->msgs);
-	if (ret < 0)
-		return ret;
-
 	switch (cmd->obj) {
 	case CMD_OBJ_RULESET:
 		cache_flush();
-- 
2.7.4

Re: [PATCH nft] evaluate: Remove cache_update() in cmd_evaluate_flush()

[Anatole Denis]

Le jeu. 5 janv. 2017 à 19:38, Elise Lennion <elise.lennion@gmail.com> 
a écrit :
> cache_update() fetches set elements, when the set is big and sorted
> this leads to an unnecessary delay on 'nft flush ruleset'.
> 
> There is only a possible call to cache_flush() after the update, so
> this update isn't needed.

That is right. However this function as-is is still incomplete, it 
needs support for flushing sets, for which we will need this 
cache_update(). It could still be removed now and reintroduced later 
though, or only applied when really needed.

I also just realized that even CMD_OBJECT_RULESET isn't safe, as it can 
take an optional family parameter, which we completely ignore right 
now. Fixing that would change the cache more selectively than flushing 
everything, thus requires the cache_update().

I would say, if the performance of nft commands is important, the cache 
mechanism would need a refactor to allow lazy-loading pieces of the 
ruleset from the kernel when needed. In its current form you need to 
load the full cache for any nontrivial operation on the cache, with the 
delay you mention.

> 
> 
> Signed-off-by: Elise Lennion <elise.lennion@gmail.com>
> ---
>  src/evaluate.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index cebc5a9..a506f9a 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -2951,12 +2951,6 @@ static int cmd_evaluate_list(struct eval_ctx 
> *ctx, struct cmd *cmd)
> 
>  static int cmd_evaluate_flush(struct eval_ctx *ctx, struct cmd *cmd)
>  {
> -	int ret;
> -
> -	ret = cache_update(cmd->op, ctx->msgs);
> -	if (ret < 0)
> -		return ret;
> -

You still need to set cache_initialized = true in some way (and 
possibly initialize table_list, I don't remember how it is declared) 
else you would be basically reverting cmd_evaluate_flush to a no-op and 
reintroducing the cache (in)coherency bugs, without even reducing the 
delay for batched commands which will reinitialize the cache on the 
next command.


> 
>  	switch (cmd->obj) {
>  	case CMD_OBJ_RULESET:
>  		cache_flush();
> --
> 2.7.4
>