Hello,
On Tue, Aug 30, 2016 at 10:53:56AM +1000, David Gwynne wrote:
>
> > On 17 Dec 2015, at 13:30, Richard Procter <richard.n.proc...@gmail.com>
> > wrote:
> >
> >
> > Hi Sasha,
> >
> > On Fri, 18 Dec 2015, Alexandr Nedvedicky wrote:
> >
> >>> Right. I'll just note though that the patch as it stands allows
> >>> multiple winners [...] Whether that's a realistic issue, I don't know.
> >>> I have though been bitten by enough edge cases like this to be very
> >>> wary of them.
> >>
> >> I think it's not realistic with current PF at OpenBSD. The pf_test()
> >> function
> >> does not run concurrently, so there can be no such race.
> >
> > Fair enough :) I presumed that an upcoming concurrent pf_test() would rely
> > on this patch.
> >
> >> FYI: PF@solaris uses mutex there to protect the insertion to gcl. Code
> >> goes as
> >> follows at Solaris:
> >>
> >> if (!(r->rule_flags & PFRULE_EXPIRED)) {
> >> mutex_enter(&gcl_mutex);
> >> /* retest under exclusive protection */
> >> if (!(r->rule_flags & PFRULE_EXPIRED)) {
> >> r->rule_flag |= PFRULE_EXPIRED;
> >> SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle);
> >> }
> >> }
> >
> > I was wondering about that. For this patch presumably it's thought
> > unnecessary to mutex the SLIST ops (or for that matter to preserve rule
> > lifetime) for one-shot rules as the purge thread runs so infrequently.
>
> right now pf doesnt run concurrently so extra locking is unnecessary. when it
> does run concurrently then it will need a mutex, despite how infrequently the
> gc runs.
I see. I'll remove the mutex and resend the patch.
>
> >
> > BTW, the purge queue exists to pass a result between threads; an
> > alternative is to recompute it in the purge thread by searching for
> > expired rules: no need for the queue, its mutexes, etc. Easier to preserve
> > rule lifetime, too. May need another anchor stack though. I might have a
> > go next year when the code is more settled.
>
> i believe the purge thread exists because once upon a time pool_free from
> interrupt context wasnt the best. i might be misremembering that though.
PF at Solaris still keeps purge thread around, although it's entirely
possible to let packet free memory. PF on Solaris does not have to deal with
such constrains.
>
> currently that thread only looks at states. traversing the ruleset is
> possible i guess.
yes, ruleset traversal is possible, however gcl (garbage collector list) is
meant as a shortcut. If packet hits ONCE rule it sets the flag to indicate
rule has matched and inserts the rule to gcl.
the purge thread then does not need to walk traverse all rulesets to collect
'dead' once rules. Instead it just walks the gcl to find out, which ONCE
rules are dead to pull them out.
There is also yet another option:
remove gcl completely and just mark the ONCE rule got hit. we can
leave dead ONCE rule to remain in ruleset until new rules will be
loaded or system will be rebooted.
> if the semantic of the once rules is that they only match
> once, and if pf will run on multiple cpus, then coordinating between the cpus
> will require atomicity or serialisation of some sort.
PF on Solaris is willing to pay price of not having ONCE rules 100% atomic,
there is currently no atomic op, which would set the flag to indicate rule
matched packet.
regards
sasha
