Hello Martin,
I accept or your comments. I just have few quick notes/questions now.
> 2) I saw that you found some ALTQ leftovers, you have some Solaris
(2) I think ALTQs leftovers are still in CVS repo, will double check
anyway. Stack alignment is not Solaris compatibility hack it's sparc
compatibility. May be your C compiler takes care of this and grants
16/32/64 bit stack alignment. I have not examined build process
that closely yet.
(3)
> use atomic operations rather than per-CPU counters or any other
> solution? I'm also raising this question because some counters are
can you point me to manual page or source code sample so I can have a look how
to use per-CPU counter?
> 5) I'm not sure to understand the goal of the new "pf_refcnt_t" type
(5) Solaris defines pf_refcnt_t as 64-bit unsigned integer, pf_refcnt_t hopes
to make porting easier. It can be defined as 32-bit on 32-bit machines.
> 7) The PF_SMP_INSERT_WQ() macro to replace SLIST_INSERT() seems over-
PF_SMP_INSERT_WQ() purpose of those is to allow every CPU/thread to
operate on its own work-queue of ktables/kentries. The current pf
uses 'intrusive' link members pfrkt_workq/pfrke_workq in pfr_ktable/pfr_kentry.
The only idea is to stay as much close to current version as possible.
I'll try to break the patch into smaller chunks of changes. And post them.
regards
sasha
On Fri, Jun 26, 2015 at 02:36:38PM +0200, Martin Pieuchot wrote:
> 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
