Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 28 Aug 2004 21:19:16 +0000
From:      Darren Reed <darrenr@hub.freebsd.org>
To:        Max Laier <max@love2party.net>
Cc:        cvs-all@FreeBSD.org
Subject:   Re: cvs commit: src/share/man/man4 ipfirewall.4 src/share/man/man9 pfil.9 src/sys/alpha/conf GENERIC src/sys/amd64/conf GENERIC src/sys/conf NOTES files options src/sys/i386/conf GENERIC src/sys/ia64/conf GENERIC SKI src/sys/modules/bridge Makefile ...
Message-ID:  <20040828211916.GA63019@hub.freebsd.org>
In-Reply-To: <200408281950.33363.max@love2party.net>
References:  <200408271516.i7RFGO8L061926@repoman.freebsd.org> <200408272253.14317.max@love2party.net> <20040828164043.GA86995@hub.freebsd.org> <200408281950.33363.max@love2party.net>

next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Aug 28, 2004 at 07:50:26PM +0200, Max Laier wrote:
> > > The check inside pfil_run_hooks() was introduced
> > > before PFIL_HOOKS got part of GENERIC to avoid the lock operation for
> > > empty hooks. It is okay to remove it now that we do the check earlier. A
> > > wrapper is called for, nontheless!
> >
> > Right, it needs removing and wasn't - just don't say it is right how it is.
> 
> Never did. Though I really do not belive that it incurres a performance loss 
> as ph_busy_count will be cached together with ph_want_write anyway ... stupid 
> micro-optimizations ...

It's not micro-optimising that I'm worried about, it's that checking the
same thing twice in a row being suggestive that someone is changing code
without having a full grasp of what they're doing.

> > I wonder if ph_busy_lock could be made to disappear and its use replaced
> > with a check for TAILQ_EMPTY() ?  This would nto require a lock/unlock.
> 
> While TAILQ_EMPTY is okay it'd require a direction lookup and that would 
> result in more overhead ...

It shouldnt.  Despite the macro taking a pointer, the tailq head is in
the pfil_head struct, itself, so the end result should be the same as
checking ph_busy_count ?  Oh, except for 64bit systems ;)

Darren



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040828211916.GA63019>