From owner-freebsd-net@FreeBSD.ORG Sun Dec 25 09:45:56 2011 Return-Path: Delivered-To: net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 924B51065670; Sun, 25 Dec 2011 09:45:56 +0000 (UTC) (envelope-from pluknet@gmail.com) Received: from mail-tul01m020-f182.google.com (mail-tul01m020-f182.google.com [209.85.214.182]) by mx1.freebsd.org (Postfix) with ESMTP id 414F48FC14; Sun, 25 Dec 2011 09:45:55 +0000 (UTC) Received: by obbwd18 with SMTP id wd18so9134298obb.13 for ; Sun, 25 Dec 2011 01:45:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=+0KAVv+aZ5vbXNxTgMAyAdouKhTvZBbVyHTsd/z3olI=; b=SLdKTxnWCt/Z9aMarkEkigDocSiHK/4bn6VOfg6HtDNOhou2XFtCrMEQpryeleGymU nggJBv80Srryx4c2cDTVBGUFF+n/FmXXV/UU1yHmTJv8b/1gPvjAO15BuXFJZXLcEbX/ 7XZ6p3M5TeH+3AODNyPqzwqszWsbeUIXVPzg8= MIME-Version: 1.0 Received: by 10.182.76.134 with SMTP id k6mr10261015obw.10.1324806355648; Sun, 25 Dec 2011 01:45:55 -0800 (PST) Received: by 10.182.171.67 with HTTP; Sun, 25 Dec 2011 01:45:55 -0800 (PST) In-Reply-To: <4EF6752C.4040109@FreeBSD.org> References: <201112231446.38057.jhb@freebsd.org> <4EF6752C.4040109@FreeBSD.org> Date: Sun, 25 Dec 2011 12:45:55 +0300 Message-ID: From: Sergey Kandaurov To: John Baldwin Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Bjoern Zeeb , net@freebsd.org Subject: Re: [PATCH] Minor fixes to netinet6/icmp6.c X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 25 Dec 2011 09:45:56 -0000 On 25 December 2011 04:58, John Baldwin wrote: > On 12/23/11 6:38 PM, Sergey Kandaurov wrote: >> >> On 23 December 2011 23:46, John Baldwin =A0wrote: >>> >>> I found these nits while working on the patches to convert if_addr_mtx = to >>> an >>> rwlock. =A0The first change is cosmetic, it just un-inlines a >>> TAILQ_FOREACH(). >>> The second change is an actual bug. =A0The code is currently reading >>> TAILQ_FIRST(&V_ifnet) without holding the appropriate lock. >>> >>> Index: icmp6.c >>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>> --- icmp6.c =A0 =A0 (revision 228777) >>> +++ icmp6.c =A0 =A0 (working copy) >>> @@ -1780,7 +1780,7 @@ ni6_addrs(struct icmp6_nodeinfo *ni6, struct mbuf >>> =A0 =A0 =A0 =A0} >>> >>> =A0 =A0 =A0 =A0IFNET_RLOCK_NOSLEEP(); >>> - =A0 =A0 =A0 for (ifp =3D TAILQ_FIRST(&V_ifnet); ifp; ifp =3D TAILQ_NE= XT(ifp, >>> if_list)) { >>> + =A0 =A0 =A0 TAILQ_FOREACH(ifp,&V_ifnet, if_list) { >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0addrsofif =3D 0; >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0IF_ADDR_LOCK(ifp); >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TAILQ_FOREACH(ifa,&ifp->if_addrhead, ifa= _link) { >> >> >> FWIW, there are much more of them in netinet6. >> Some time ago I started to un-expand them to queue(3). >> [not unfinished yet..] > > > I went ahead and did a sweep for queue(3) changes in netinet6. =A0I went = a bit Great, thank you! This sweep is long overdue. > further and removed things like the ndpr_next hack, etc. =A0This only inc= ludes > queue(3) changes though, not your other fixes like moving common code out= of Oops, yeah. This is an unrelated change. > blocks. =A0I also fixed a few places to use *_EMPTY() instead of checking > *_FIRST() against NULL. =A0There should be no functional change. > > http://www.FreeBSD.org/~jhb/patches/inet6_queue.patch Looks good. Please, commit with two notes: a) You changed a loop with precondition while (i < DRLSTSIZ) { ... } into if (i >=3D DRLSTSIZ) and moved it below i++ increment, which effectively became a loop with post-condition like do { ...} while (). To preserve the current behavior I would move this check up right under *_FOREACH() loop, like this: TAILQ_FOREACH(dr, &V_nd_defrouter, dr_entry) { if (i >=3D DRLSTSIZ) break; b) It should be safe to use non-SAFE() FOREACH() variants of queue(3) macros for most occurrences. SAFE() versions are usually only used when you need to add/remove an element on list w/o need to subsequent restart the *_FOREACH() loop. --=20 wbr, pluknet