* [nft PATCH] json: echo: Speedup seqnum_to_json()
@ 2020-11-20 19:16 Phil Sutter
2020-11-21 12:17 ` Pablo Neira Ayuso
0 siblings, 1 reply; 5+ messages in thread
From: Phil Sutter @ 2020-11-20 19:16 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel, Derek Dai
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
Bugzilla: https://bugzilla.netfilter.org/show_bug.cgi?id=1479
Reported-by: Derek Dai <daiderek@gmail.com>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
src/parser_json.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/src/parser_json.c b/src/parser_json.c
index b1de56a4a0d27..6ebbb408d6839 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3762,42 +3762,50 @@ static int json_verify_metainfo(struct json_ctx *ctx, json_t *root)
}
struct json_cmd_assoc {
- struct json_cmd_assoc *next;
+ struct hlist_node hnode;
const struct cmd *cmd;
json_t *json;
};
-static struct json_cmd_assoc *json_cmd_list = NULL;
+#define CMD_ASSOC_HSIZE 512
+static struct hlist_head json_cmd_assoc_hash[CMD_ASSOC_HSIZE];
static void json_cmd_assoc_free(void)
{
struct json_cmd_assoc *cur;
+ struct hlist_node *pos, *n;
+ int i;
- while (json_cmd_list) {
- cur = json_cmd_list;
- json_cmd_list = cur->next;
- free(cur);
+ for (i = 0; i < CMD_ASSOC_HSIZE; i++) {
+ hlist_for_each_entry_safe(cur, pos, n,
+ &json_cmd_assoc_hash[i], hnode)
+ free(cur);
}
}
static void json_cmd_assoc_add(json_t *json, const struct cmd *cmd)
{
struct json_cmd_assoc *new = xzalloc(sizeof *new);
+ int key = cmd->seqnum % CMD_ASSOC_HSIZE;
- new->next = json_cmd_list;
new->json = json;
new->cmd = cmd;
- json_cmd_list = new;
+
+ hlist_add_head(&new->hnode, &json_cmd_assoc_hash[key]);
}
static json_t *seqnum_to_json(const uint32_t seqnum)
{
- const struct json_cmd_assoc *cur;
+ int key = seqnum % CMD_ASSOC_HSIZE;
+ struct json_cmd_assoc *cur;
+ struct hlist_node *n;
- for (cur = json_cmd_list; cur; cur = cur->next) {
+
+ hlist_for_each_entry(cur, n, &json_cmd_assoc_hash[key], hnode) {
if (cur->cmd->seqnum == seqnum)
return cur->json;
}
+
return NULL;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [nft PATCH] json: echo: Speedup seqnum_to_json()
2020-11-20 19:16 [nft PATCH] json: echo: Speedup seqnum_to_json() Phil Sutter
@ 2020-11-21 12:17 ` Pablo Neira Ayuso
2020-11-22 23:56 ` Phil Sutter
0 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2020-11-21 12:17 UTC (permalink / raw)
To: Phil Sutter; +Cc: netfilter-devel, Derek Dai, guigom
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.
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?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [nft PATCH] json: echo: Speedup seqnum_to_json()
2020-11-21 12:17 ` Pablo Neira Ayuso
@ 2020-11-22 23:56 ` Phil Sutter
2020-11-30 20:43 ` Jose M. Guisado
0 siblings, 1 reply; 5+ messages in thread
From: Phil Sutter @ 2020-11-22 23:56 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel, Derek Dai, guigom
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.
>
> 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!
Thanks, Phil
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [nft PATCH] json: echo: Speedup seqnum_to_json()
2020-11-22 23:56 ` Phil Sutter
@ 2020-11-30 20:43 ` Jose M. Guisado
2020-12-02 9:47 ` Pablo Neira Ayuso
0 siblings, 1 reply; 5+ messages in thread
From: Jose M. Guisado @ 2020-11-30 20:43 UTC (permalink / raw)
To: Phil Sutter, Pablo Neira Ayuso; +Cc: netfilter-devel, Derek Dai
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@riseup.net/#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!
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [nft PATCH] json: echo: Speedup seqnum_to_json()
2020-11-30 20:43 ` Jose M. Guisado
@ 2020-12-02 9:47 ` Pablo Neira Ayuso
0 siblings, 0 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2020-12-02 9:47 UTC (permalink / raw)
To: Jose M. Guisado; +Cc: Phil Sutter, netfilter-devel, Derek Dai
On Mon, Nov 30, 2020 at 09:43:04PM +0100, Jose M. Guisado wrote:
> 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.
OK, I have pushed out this patch. Thanks for clarifying.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-02 9:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-20 19:16 [nft PATCH] json: echo: Speedup seqnum_to_json() Phil Sutter
2020-11-21 12:17 ` Pablo Neira Ayuso
2020-11-22 23:56 ` Phil Sutter
2020-11-30 20:43 ` Jose M. Guisado
2020-12-02 9:47 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).