On 23/11/20 0:56, Phil Sutter wrote:
Hi,
On Sat, Nov 21, 2020 at 01:17:24PM +0100, Pablo Neira Ayuso wrote:
On Fri, Nov 20, 2020 at 08:16:40PM +0100, Phil Sutter wrote:
Derek Dai reports:
"If there are a lot of command in JSON node, seqnum_to_json() will slow
down application (eg: firewalld) dramatically since it iterate whole
command list every time."
He sent a patch implementing a lookup table, but we can do better: Speed
this up by introducing a hash table to store the struct json_cmd_assoc
objects in, taking their netlink sequence number as key.
Quickly tested restoring a ruleset containing about 19k rules:
| # time ./before/nft -jeaf large_ruleset.json >/dev/null
| 4.85user 0.47system 0:05.48elapsed 97%CPU (0avgtext+0avgdata 69732maxresident)k
| 0inputs+0outputs (15major+16937minor)pagefaults 0swaps
| # time ./after/nft -jeaf large_ruleset.json >/dev/null
| 0.18user 0.44system 0:00.70elapsed 89%CPU (0avgtext+0avgdata 68484maxresident)k
| 0inputs+0outputs (15major+16645minor)pagefaults 0swaps
LGTM.
BTW, Jose (he's on Cc) should rewrite his patch to exercise the
monitor path when --echo and --json are combined _and_ input is _not_
json.
IIRC v4 of the patch already takes into account this situation.
Specifically this piece of code inside netlink_echo_callback. Returning
the json_events_cb (the path leading to the seqnum_to_json call) when
input is json.
- if (nft_output_json(&ctx->nft->output))
- return json_events_cb(nlh, &echo_monh);
+ if (nft_output_json(&nft->output)) {
+ if (!nft->json_root) {
+ nft->json_echo = json_array();
+ if (!nft->json_echo)
+ memory_allocation_error();
+ echo_monh.format = NFTNL_OUTPUT_JSON;
+ } else
+ return json_events_cb(nlh, &echo_monh);
+ }
return netlink_events_cb(nlh, &echo_monh);
}
I also remember Eric Garver ran firewalld tests with this patch.
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20200804103846.58872-1-guigom@xxxxxxxxxx/#2499299
Hence, leaving --echo and --json where input is json in the way you
need (using the sequence number to reuse the json input
representation).
OK?
Yes, that's fine with me!
It's been some time, but I think this patch was ready to be merged back
then and that does not interfere with the json_events_cb path. Just
adding echo+json capability when reading native syntax.
Regards!
