Page MenuHomeFreeBSD

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

Authored by neel_neelc.org on Apr 15 2020, 3:45 AM.

Details

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>

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

neel_neelc.org created this revision.Apr 15 2020, 3:45 AM
rgrimes accepted this revision.Apr 15 2020, 8:18 AM
This revision is now accepted and ready to land.Apr 15 2020, 8:18 AM
rgrimes added a reviewer: bz.May 5 2020, 4:14 AM

Can we get some more eyes on this please?

Sure, fine with me.

lutz_donnerhacke.de requested changes to this revision.May 5 2020, 7:07 AM
lutz_donnerhacke.de added inline comments.
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.

This revision now requires changes to proceed.May 5 2020, 7:07 AM

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

neel_neelc.org marked 2 inline comments as done.May 5 2020, 8:44 PM
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,.

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

Here's an updated version.

neel_neelc.org marked 2 inline comments as done.May 6 2020, 12:57 AM
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 added a comment.May 6 2020, 7:17 AM

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

"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; \
In D24427#544221, @ae wrote:

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

Hahaha.

neel_neelc.org abandoned this revision.May 6 2020, 3:45 PM

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