Hello Sasha,
Alexandr Nedvedicky [alexandr.nedvedi...@oracle.com] wrote:
> Hello,
>
> attached is SMP patch for PF. consider it as toxic proof of concept as it has
> paniced my amd64 system (see attached phone-shot). I have to figure out how to
> debug it yet. The problem is the USB keyboard has died, so I had no chance to
> type anything. fortunately the issue is 100% reproducible.
>
> The patch compiles in .MP and non-MP version. As you'll see more work is
> needed to stabilize it and get full SMP support of PF. Those PF
> features are not covered by SMP changes:
> - packet queues
> - packet logging
> - pf-sync
This is an impressive diff, wow! I started to look at it and my first
impression is that it is too big. You should really try to split it
in smaller pieces to get proper reviews.
Anyway here are some comments about splitting/cleaning this diff, I'll
need more time to be really able to comment on your work. I'd just
want to say "wow" again. This is amazing!
1) Your diff includes a lot of "cleanups" which are not directly
related to your SMP work. By cleanups I'm talking about the
FALLTHROUGH, "#ifdef", comments, (void), etc that your added in
various places. I'd suggest submitting a first diff including
all these cleanups. It can be easily reviewed and committed and
this will reduce the noise of your SMP work (and size of the diff).
2) I saw that you found some ALTQ leftovers, you have some Solaris
compatibility goo, stack alignment tricks or when sometimes you
need to return a variable to (de)reference it (ie pf_get_sport).
These could also be single-shot easy-to-review diffs.
3) A lot of chunks in your diff are related to counter modifications.
This could be a diff in itself. I'm a bit afraid by the number of
different macro to deal with counters. Then why did you choose to
use atomic operations rather than per-CPU counters or any other
solution? I'm also raising this question because some counters are
64bit long and there's no atomic operation to modify such value on
32bit architectures.
4) Regarding reference counting around pool-allocated object, I'd
subject to wrap the pool_{get,put} into their own function this
would greatly reduce the "#ifdef _PF_SMP_/#else" dances like:
+#ifdef _PF_SMP_
+ pf_rule_smp_rele(rule);
+#else /* !_PF_SMP_ */
pool_put(&pf_rule_pl, rule);
+#endif /* !_PF_SMP_ */
5) I'm not sure to understand the goal of the new "pf_refcnt_t" type
but using a long (why not unsigned?) makes sense with regards to
atomic operations. Note however that your comment describing it
is incorrect. I'd rather delete the comment. It's a good
explanation for an email but the intend is quite obvious.
6) In pf_osfp.c rather than changing the signature of some functions
in the _PF_SMP_ case only, I'd suggest to adapt the existing code.
Having fewer "#ifdef _PF_SMP_/#else" makes it easier to understand,
review and work with the code 8) This comment also applies to
pfr_pool_get().
7) The PF_SMP_INSERT_WQ() macro to replace SLIST_INSERT() seems over-
generic to me. Do you plan to use it with a different allocator?
Can't we use it for the SP version of pf_table too or at least
create a macro/function that behave differently for the SMP version
to reduce the "#ifdef" dances...
8) Your protection of the pfi_ifhead RB-tree principally correspond to
the existing splsoftnet(), don't you think it would make sense to
use a single macro for the IPL and on SMP rwlock?
Cheers,
Martin
