Date: Fri, 21 May 2004 00:02:37 +0200 From: Andre Oppermann <andre@freebsd.org> To: Pawel Jakub Dawidek <pjd@FreeBSD.org> Cc: freebsd-net@freebsd.org Subject: Re: Socket selection. Message-ID: <40AD2AFD.EA2470CC@freebsd.org> References: <20040520173012.GR845@darkness.comp.waw.pl> <40AD1CBA.4CB46233@freebsd.org> <20040520211058.GX845@darkness.comp.waw.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
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 = <something>; > [...] > } > > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?40AD2AFD.EA2470CC>