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.
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.
Ok, enough from me on this.
best,
Richard.
