ipfw: use SLIST_REMOVE_HEAD and SLIST_REMOVE_AFTER for the first and subsequent respective states

[nc]

Summary

ipfw: use SLIST_REMOVE_HEAD and SLIST_REMOVE_AFTER for the first and subsequent respective states when freeing expired dynamic states.

This is done with a previous pointer, which is used by SLIST_REMOVE_AFTER on subsequent states to prevent re-iterating the state table for expired entries. On the first entry, SLIST_REMOVE_HEAD is used as prev would be null.

Submitted by: Neel Chauhan <neel AT neelc DOT org>

[rgrimes]

Can we get some more eyes on this please?

[nc]

Sure, fine with me.

[donner]

sys/netpfil/ipfw/ip_fw_dynamic.c

2060–2061

I'd strongly suggest to rewrite "init_code; while(condition) { temp_code; xxx; loop_code }" by SLIST_FOREACH_SAFE.

2066–2079

I'd like to see a rewrite if this code into a series of "if elseif elseif else". Especially avoid "code_duplication; continue" fragments.

[nc]

I have a revised patch which removes code duplication (as suggested by Lutz Donnerhacke).

[donner]

sys/netpfil/ipfw/ip_fw_dynamic.c

2067–2078

That's too complex. I thought about something like this:

if (i != cached_count) {
   prev = s;
} else if (s-type == O_LIMIT_PARENT && s->limit_count != 0) {
   prev = s;
} else {
   remove(s);
}

2084–2085

That's not longer true after removal of the element.
"prev" stays as it is, when "s" is removed,.

[nc]

Makes complete sense, I didn't realize how complex my code has become.

Here's an updated version.

[donner]

sys/netpfil/ipfw/ip_fw_dynamic.c

2064–2066

This loop in computationally expensive and invariant to the test "s->type == VAL && s->limit->count != 0".

So for the sake of efficiency, the code should be reordered.

 SLIST_FOREACH... {
    if (s->type ....) { del = s; continue; }
    for (i = 0; ...) ...
    if (i != cache_count) { del = s; continue; }
    remove(s);
}

Too bad, that we cant's jump directly out of the inner loop.

 expire_loop_##name:
 SLIST_FOREACH... {
    if (s->type ....) { del = s; continue; }
    for (i = 0; ...)
       if (dyn_hp_cache ...
           continue expire_loop_##name;
    remove(s);
}

[ae]

JFYI, this code will be removed when refactoring to the epoch(9) will be finished.

[donner]

"del" is a bad name for the running variable. I'd feel "prev" more appropriate.

sys/netpfil/ipfw/ip_fw_dynamic.c

2019–2021

Because the temp pointers "sXt" are never used to access member elements, it's possible to avoid them completely.

Just define a temp pointer in the DYN_FREE_STATES macro itself:

#define DYN...   do { \
    void * del = NULL; \

[donner]

In D24427#544221, @ae wrote:

JFYI, this code will be removed when refactoring to the epoch(9) will be finished.

Hahaha.

[nc]

I will close this revision as the code being modified will get refactored out.