From owner-freebsd-current@FreeBSD.ORG Mon Apr 12 20:10:45 2004 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 0A9BC16A4CE; Mon, 12 Apr 2004 20:10:45 -0700 (PDT) Received: from shuttle.wide.toshiba.co.jp (shuttle.wide.toshiba.co.jp [202.249.10.124]) by mx1.FreeBSD.org (Postfix) with ESMTP id 2564D43D49; Mon, 12 Apr 2004 20:10:43 -0700 (PDT) (envelope-from jinmei@isl.rdc.toshiba.co.jp) Received: from ocean.jinmei.org (unknown [2001:200:0:8002:200:39ff:fe5e:cfd7]) by shuttle.wide.toshiba.co.jp (Postfix) with ESMTP id E234D1521A; Tue, 13 Apr 2004 12:10:39 +0900 (JST) Date: Tue, 13 Apr 2004 12:10:40 +0900 Message-ID: From: JINMEI Tatuya / =?ISO-2022-JP?B?GyRCP0BMQEMjOkgbKEI=?= To: Luigi Rizzo In-Reply-To: <20040412075638.B67293@xorpc.icir.org> References: <20040409042720.A99087@xorpc.icir.org> User-Agent: Wanderlust/2.10.1 (Watching The Wheels) Emacs/21.3 Mule/5.0 (SAKAKI) Organization: Research & Development Center, Toshiba Corp., Kawasaki, Japan. MIME-Version: 1.0 (generated by SEMI 1.14.5 - "Awara-Onsen") Content-Type: text/plain; charset=US-ASCII X-Mailman-Approved-At: Tue, 13 Apr 2004 04:50:20 -0700 cc: ume@freebsd.org cc: core@kame.net cc: andre@freebsd.org cc: current@freebsd.org cc: net@freebsd.org Subject: Re: suggested patches for netinet6/ X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Apr 2004 03:10:45 -0000 >>>>> On Mon, 12 Apr 2004 07:56:38 -0700, >>>>> Luigi Rizzo said: >> > + nd6_nud_hint() is only called as nd6_nud_hint(NULL, NULL, 0); >> > in some cases from netinet/tcp_input.c >> > With these args, the routine is a NOP. I propose to remove it >> > (and the associated field, ln_byhint, in struct llinfo_nd6) > ... >> (At the same time, however, I'm personally skeptical about the >> effectiveness of such a hint from a higher layer to ND. In that >> sense, it may make sense to purge it...) > not knowing much about ipv6 i cannot comment, however > if this is just a performance optimization it can be [re]introduced > later. This is also a protocol compliance issue, since RFC2461 clearly mentions the use of such a hint (though the requirement level is not very clear; no RFC2119 keywords are used here). Also, I don't think the logic of "we can reintroduce it later" is reasonable. We originally provided working implementation, which was then broken by other changes. In general, what should be fixed (modified) is the "other changes", not the original code. So, as a compromise, I'd propose to keep the source code even if it is not effective. Perhaps it may make sense to comment out this part with a note that the part is temporarily broken and will be fixed later. >> > + In a couple of places, the logic to compute 'olladdr' and 'llchange' >> > is rather convoluted, and it could be greatly simplified (see below) >> >> I'm not sure if this is that trivial. In particular, I don't >> understand (at least from the patch) why we can replace >> >> - olladdr = (sdl->sdl_alen) ? 1 : 0; >> >> with >> >> + olladdr = ln->ln_state >= ND6_LLINFO_REACHABLE; > sorry, that included a bit from my new arp code (basically, > if you have a MAC address stored for the node then both sdl->sdl_alen !=0 > and ln->ln_state >= ND6_LLINFO_REACHABLE. Strictly speaking, this reasoning cannot justify the change, logic-wise. The original code should read "if sdl->sdl_alen is non 0, then you have a MAC address stored". It's a different proposition than "if you have a MAC address stored for the node then (sdl_alen is non 0 and) ln_state >= ND6_LLINFO_REACHABLE." What we should prove to justify the above change is that "if ln_state >= ND6_LLINFO_REACHABLE then you have a MAC address stored". I'm still not sure if this statement holds. > The second form is more > appealing to me because with the new arp code there are no > more host route entries generated by cloning, and the MAC > address resides elsewhere...) > Leave olladdr as it was, the rest of the patch just tried to > replace the conditional statements with boolean expressions. So are you now proposing to keep the code for olladdr as it was (perhaps with some modifications along with the new arp code?) and to replace the other conditions with boolean expressions? If so, I don't oppose to it, though I'd rather personally prefer, e.g., - if (olladdr && lladdr) { - if (bcmp(lladdr, LLADDR(sdl), ifp->if_addrlen)) - llchange = 1; - else - llchange = 0; - } else - llchange = 0; than + llchange = olladdr && lladdr && bcmp(lladdr, &ln->ll_addr, ifp->if_addrlen); because the intention is clearer in the former, particularly when we check the logic comparing to the protocol specification. (In other words, IMO it's compiler's business to perform this kind of "simplification"). JINMEI, Tatuya Communication Platform Lab. Corporate R&D Center, Toshiba Corp. jinmei@isl.rdc.toshiba.co.jp