> On 4 Jan 2023, at 20:20, Alexandr Nedvedicky <[email protected]> wrote:
>
> 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
i'll put this in as is, and have a go at some pf_state variable renames as a
follow up.
>
> thanks and
> regards
> sashan
