Hello,
I agree with change as-is. Though I have some suggestions
for few finishing touches. see comments inline.
On Wed, Jan 04, 2023 at 01:15:54PM +1000, David Gwynne wrote:
> move the pf_state_tree_id type from pfvar.h to pfvar_priv.h.
>
> this is like the pf_state_tree change from yesterday. the
> pf_state_tree_id type is private to the kernel, just like the
> pf_state_tree type.
>
> moving it from RB to RBT saves another 12k in pf.o on amd64.
>
> i also changed some hand rolled for loops over to RBT_FOREACH_SAFE.
> they look right?
All those look good to me. Suggestion below are just
suggestions which I don't insist on.
>
> ok?
>
> Index: pf.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.1166
> diff -u -p -r1.1166 pf.c
> --- pf.c 4 Jan 2023 02:00:49 -0000 1.1166
> +++ pf.c 4 Jan 2023 03:08:04 -0000
</snip>
> @@ -1054,7 +1053,7 @@ pf_state_insert(struct pfi_kif *kif, str
> s->id = htobe64(pf_status.stateid++);
> s->creatorid = pf_status.hostid;
> }
> - if (RB_INSERT(pf_state_tree_id, &tree_id, s) != NULL) {
> + if (RBT_INSERT(pf_state_tree_id, &tree_id, s) != NULL) {
> if (pf_status.debug >= LOG_NOTICE) {
> log(LOG_NOTICE, "pf: state insert failed: "
> "id: %016llx creatorid: %08x",
> @@ -1085,7 +1084,7 @@ pf_find_state_byid(struct pf_state_cmp *
> {
> pf_status.fcounters[FCNT_STATE_SEARCH]++;
>
> - return (RB_FIND(pf_state_tree_id, &tree_id, (struct pf_state *)key));
> + return (RBT_FIND(pf_state_tree_id, &tree_id, (struct pf_state *)key));
> }
would it make sense to rename argument `s` to `st`? Just to lean
towards consistency in pf(4). `st` usually denotes state.
</snip>
> Index: pf_ioctl.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_ioctl.c,v
> retrieving revision 1.394
> diff -u -p -r1.394 pf_ioctl.c
> --- pf_ioctl.c 4 Jan 2023 02:00:49 -0000 1.394
> +++ pf_ioctl.c 4 Jan 2023 03:08:05 -0000
> @@ -1796,10 +1796,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
> NET_LOCK();
> PF_LOCK();
> PF_STATE_ENTER_WRITE();
> - for (s = RB_MIN(pf_state_tree_id, &tree_id); s;
> - s = nexts) {
> - nexts = RB_NEXT(pf_state_tree_id, &tree_id, s);
> -
> + RBT_FOREACH_SAFE(s, pf_state_tree_id, &tree_id, nexts) {
> if (s->direction == PF_OUT) {
> sk = s->key[PF_SK_STACK];
> srcaddr = &sk->addr[1];
again: same change as earlier just rename `s` to `st` when
already there.
> @@ -2828,7 +2825,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
> NET_LOCK();
> PF_LOCK();
> PF_STATE_ENTER_WRITE();
> - RB_FOREACH(state, pf_state_tree_id, &tree_id)
> + RBT_FOREACH(state, pf_state_tree_id, &tree_id)
> pf_src_tree_remove_state(state);
> PF_STATE_EXIT_WRITE();
> RB_FOREACH(n, pf_src_tree, &tree_src_tracking)
> @@ -2861,7 +2858,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
> if (sn->states != 0) {
> PF_ASSERT_LOCKED();
> PF_STATE_ENTER_WRITE();
> - RB_FOREACH(s, pf_state_tree_id,
> + RBT_FOREACH(s, pf_state_tree_id,
> &tree_id)
> pf_state_rm_src_node(s, sn);
> PF_STATE_EXIT_WRITE();
in this case branch too, rename `state` to `st`.
> Index: pf_lb.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_lb.c,v
> retrieving revision 1.72
> diff -u -p -r1.72 pf_lb.c
> --- pf_lb.c 31 Aug 2022 11:29:12 -0000 1.72
> +++ pf_lb.c 4 Jan 2023 03:08:05 -0000
> @@ -311,10 +311,8 @@ pf_map_addr_sticky(sa_family_t af, struc
> }
> if (sns[type]->states != 0) {
> /* XXX expensive */
> - RB_FOREACH(s, pf_state_tree_id,
> - &tree_id)
> - pf_state_rm_src_node(s,
> - sns[type]);
> + RBT_FOREACH(s, pf_state_tree_id, &tree_id)
> + pf_state_rm_src_node(s, sns[type]);
> }
rename `s` to `st`
> sns[type]->expire = 1;
> pf_remove_src_node(sns[type]);
> Index: pfvar.h
> ===================================================================
> RCS file: /cvs/src/sys/net/pfvar.h,v
> retrieving revision 1.526
> diff -u -p -r1.526 pfvar.h
> --- pfvar.h 4 Jan 2023 02:00:49 -0000 1.526
> +++ pfvar.h 4 Jan 2023 03:08:05 -0000
> @@ -1581,10 +1581,6 @@ RB_HEAD(pf_src_tree, pf_src_node);
> RB_PROTOTYPE(pf_src_tree, pf_src_node, entry, pf_src_compare);
> extern struct pf_src_tree tree_src_tracking;
>
> -RB_HEAD(pf_state_tree_id, pf_state);
> -RB_PROTOTYPE(pf_state_tree_id, pf_state,
> - entry_id, pf_state_compare_id);
> -extern struct pf_state_tree_id tree_id;
> extern struct pf_state_list pf_state_list;
I think we can move pf_state_list to pvar_priv.h too
with or without suggestions included the diff is OK sashan
thanks and
regards
sashan
