From owner-freebsd-net@FreeBSD.ORG Thu May 20 15:02:41 2004 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id F253C16A4CE for ; Thu, 20 May 2004 15:02:40 -0700 (PDT) Received: from mailtoaster1.pipeline.ch (mailtoaster1.pipeline.ch [62.48.0.70]) by mx1.FreeBSD.org (Postfix) with ESMTP id 0314743D46 for ; Thu, 20 May 2004 15:02:40 -0700 (PDT) (envelope-from andre@freebsd.org) Received: (qmail 48449 invoked from network); 20 May 2004 22:02:39 -0000 Received: from unknown (HELO freebsd.org) ([62.48.0.53]) (envelope-sender ) by mailtoaster1.pipeline.ch (qmail-ldap-1.03) with SMTP for ; 20 May 2004 22:02:39 -0000 Message-ID: <40AD2AFD.EA2470CC@freebsd.org> Date: Fri, 21 May 2004 00:02:37 +0200 From: Andre Oppermann X-Mailer: Mozilla 4.8 [en] (Windows NT 5.0; U) X-Accept-Language: en MIME-Version: 1.0 To: Pawel Jakub Dawidek References: <20040520173012.GR845@darkness.comp.waw.pl> <40AD1CBA.4CB46233@freebsd.org> <20040520211058.GX845@darkness.comp.waw.pl> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit cc: freebsd-net@freebsd.org Subject: Re: Socket selection. X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 May 2004 22:02:41 -0000 Pawel Jakub Dawidek wrote: > > On Thu, May 20, 2004 at 11:01:46PM +0200, Andre Oppermann wrote: > +> Pawel Jakub Dawidek wrote: > +> > > +> > Hello. > +> > > +> > In in_pcblookup_hash() function, in the last loop if we find exact > +> > match, we return immediately, if it is "wild", we store a pointer and > +> > we countinue looking for exact match. > +> > I wonder if this is ok, that we change pointer every time we find a > +> > "wild" match. Is it inteded? Shouldn't it be: > +> > > +> > http://people.freebsd.org/~pjd/patches/in_pcb.c.2.patch > +> > +> No. This is a stack variable which is unconditionally initialized to > +> NULL a few lines earlier. Checking for variable == NULL is always > +> going to be true and makes your 'optimization' just redundand. > > But we have loop there: > > local_wild = NULL; > [...] > LIST_FOREACH(...) { > [...] > local_wild = ; > [...] > } > > Isn't that possible that local_wild will be set few times? >From the loop as such it is. Normally the conditions leading to this code are that there is only one match. However you want to have the last match, not the first one. My statement in the first reply wasn't clear/correct enough I have to admit. ;-) The networking code is very tangled and obfuscated. The first thing we should do is to clean it up and make readable, self-consistent and logic again. This kind of micro-optimizations often open a can of worms and do not really improve things unless there true evidence (profiled) that this is a place where a lot of time is spent. > +> > While I'm here, I want to improve code readability a bit: > +> > > +> > http://people.freebsd.org/~pjd/patches/in_pcb.c.3.patch > +> > > +> > Is it ok? > +> > +> No. You change the logic of the 'if' statements to something totally > +> different. This ain't going to work in any way. > > Hmm, no: > > for (...) { > if (a == b && c == d) { > /* do something */ > } > /* nothing here */ > } > > is equivalent to: > > for (...) { > if (a != b || c != d) > continue; > /* do something */ > } > > isn't it? Well, from the second look it is indeed in this case. However it is obfusicated enough to be a show-stopper for this 'optimization'. The way it is now is far more logical and clearly expresses the intention of the check. Only do the following checks if 'a' and 'c' are true. Not the other way around. When someone is going to modify this loop later on I can virtually guarantee you that he'll fall into the trap with your changes. I probably would be baffled by the logic for the first moment and then I would curse you and revert it to be more obviously logic again... ;-) -- Andre