cvs commit: src/sys/netinet ip_fw_pfil.c

[Andre Oppermann]

Sam Leffler wrote:
> 
> Andre Oppermann wrote:
> > "Christian S.J. Peron" wrote:
> >
> >>On Thu, Dec 09, 2004 at 04:41:47PM +0000, Gleb Smirnoff wrote:
> >>
> >>>glebius     2004-12-09 16:41:47 UTC
> >>>
> >>>  FreeBSD src repository
> >>>
> >>>  Modified files:
> >>>    sys/netinet          ip_fw_pfil.c
> >>>  Log:
> >>>  Check that DUMMYNET_LOADED before seeking dummynet m_tag.
> >>
> >>I think Sam had some reservations about doing this before, We had some
> >>discussions and in the end it was pretty much concluded that since
> >>tags are rarely present, and m_tag_locate is only called if tags are present,
> >>adding this check unconditionally added a memory write and a compare
> >>for every packet.
> >>
> >>This change may be a mistake unless you can prove some significant
> >>performance gain.
> >
> >
> > Checking for DUMMYNET_LOADED is a simple pointer compare to NULL and doesn't
> > add a memory write.  Not a big difference for sure but not hurting either.
> >
> 
> Chris and I had a discussion a while back about not adding code that
> checks for the presence of optional functionality.  Using tags was
> intended to eliminate checks like this.  Doing this sort of stuff can
> cause an unmaintainable/unreadable mess.  Unless you can show there is a
> noticeable performance gain from doing this I think it's a bad idea.
> Remember that m_tag_find already inlines the check for whether any tags
> are present or not before doing the lookup.
> 
> If tag lookup cost becomes an important consideration in writing code
> then we need to address that basic functionality.

Actually this is a good argument and reasoning and I buy into it.  Not
that is matter this much in this case but having nice and clean code
wins big over time.  I've spent and am still spending too much time
cleaning up old BSD PDP-11 "optimizations" and other shortcuts.

-- 
Andre