cvs commit: src/sys/netinet ip_fw_pfil.c

[Sam Leffler]

Gleb Smirnoff wrote:
> On Thu, Dec 09, 2004 at 01:11:29PM -0800, Sam Leffler wrote:
> S> Chris and I had a discussion a while back about not adding code that 
> S> checks for the presence of optional functionality.  Using tags was 
> S> intended to eliminate checks like this.  Doing this sort of stuff can 
> S> cause an unmaintainable/unreadable mess.  Unless you can show there is a 
> 
> I don't see reduced readability. I don't see reduced maintainability.

Sigh.

> 
> S> noticeable performance gain from doing this I think it's a bad idea. 
> S> Remember that m_tag_find already inlines the check for whether any tags 
> S> are present or not before doing the lookup.
> 
> S> If tag lookup cost becomes an important consideration in writing code 
> S> then we need to address that basic functionality.
> 
> Searching a list is always more expensive than comparing a pointer value.

And your point?

> 
> There may exist a tag from divert, from altq and any future tag. It is
> obvious that this change gives performance gain, and it is obvious that
> this gain is not noticeable.

You are adding cost to the most common use case in order to satisfy your 
preferred operating conditions.  The overhead for optional features must 
be weighed against the cost to users that do not need/use these 
features.  I suspect the vast majority of users do not use altq, divert, 
or ipfw, pf, whatever.  I'm sure someone will chime in and explain that 
everyone should run a packet filter on their machine (that Darren's 
schtick ? :)) but my point is that it is not obvious this is a win.

> 
> Now I'm working on interaction between netgraph and ipfw. I'm going
> to add a very similar tag lookup here. The vast majority of hosts
> running ipfw do not use dummynet or netgraph facility. Why would they
> look in every packet for tags that would never appear in any packet?

I'm glad your working on this stuff but please don't lose sight that 
netgraph and ipfw are optional components and their operation should not 
weigh down the operation of a system w/o them.  If the cost is minimal 
I've got no issue.  But since you refuse to measure the cost and simply 
state "it's obviously no cost" I'm going to complain.  I understand this 
is one memory compare, but adding one here, one there, and eventually it 
does add up.

> 
> I'd like to remind you that in this thread you were against removal
> of the very similar check before doing tag lookup:
> 
> http://freebsd.rambler.ru/bsdmail/freebsd-current_2004/msg22234.html

True, but there are differences.  The post you cite was about vlans and 
checking every packet that passes through a driver.  That case was 
significantly more performance critical than yours.

> 
> P.S. If you and Chris insist on reverting, I will do revert, but you didn't
> made my opinion.
> 

I never asked to revert; I am stating that I think this is a bad 
direction to go in.  I removed all such checks a while back when I 
converted a lot of the old goo to use tags and at that time I MEASURED 
the overehead associated with the changes.

	Sam