From owner-freebsd-net@FreeBSD.ORG Wed Nov 18 15:35:40 2009 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id F41341065670; Wed, 18 Nov 2009 15:35:39 +0000 (UTC) (envelope-from rwatson@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id CA64B8FC14; Wed, 18 Nov 2009 15:35:39 +0000 (UTC) Received: from [192.168.2.101] (host217-43-176-60.range217-43.btcentralplus.com [217.43.176.60]) by cyrus.watson.org (Postfix) with ESMTPSA id 59FD246B58; Wed, 18 Nov 2009 10:35:38 -0500 (EST) Mime-Version: 1.0 (Apple Message framework v1077) Content-Type: text/plain; charset=us-ascii From: "Robert N. M. Watson" In-Reply-To: <200911181012.07167.jhb@freebsd.org> Date: Wed, 18 Nov 2009 15:35:36 +0000 Content-Transfer-Encoding: quoted-printable Message-Id: <70A1A5B3-1F19-4AC1-8975-F7E950785BBF@freebsd.org> References: <01D9CB64-F04C-4506-ACF2-1DE459FC69CD@freebsd.org> <200911181012.07167.jhb@freebsd.org> To: John Baldwin X-Mailer: Apple Mail (2.1077) Cc: FreeBSD Net , Brooks Davis , "M. Warner Losh" , Antoine Brodin Subject: Re: [PATCH FOR REVIEW] interface description (revised) 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: Wed, 18 Nov 2009 15:35:40 -0000 On 18 Nov 2009, at 15:12, John Baldwin wrote: > However, this is a bit complicated, and to be honest, I don't think = interface > descriptions are a critical path. Robert has already said before that > IF_AFDATA_RLOCK() isn't really the "correct" lock but is being abused = for=20 > this. Given that, I would probably just add a single global sx lock. = This > has the added advantage that you can just use copyin/copyout directly = and > skip all the extra complication. I don't think we need the extra = concurrency > for interface descriptions to make this so complicated. If you used a = global > sx lock with a simple string for descriptions, the code would end up = looking > like this: >=20 > static struct sx ifdescr_lock; > SX_SYSINIT(&ifdescr_lock, "ifnet descr"); This strikes me as a good idea -- there won't be much real-world = contention on the lock, I'd think, and this greatly simplifies the code. = We might think about whether there's other, currently = under-synchronized, ifnet meta-data that could be protected usefully = with the same lock. > case SIOCSIFDESCR: > error =3D priv_check(); > if (error) > break; >=20 > if (ifr->ifr_buffer.length > ifdescr_maxlen) > return (ENAMETOOLONG); >=20 > buf =3D malloc(ifr->ifr_buffer.length, M_IFDESCR, = M_WAITOK | > M_ZERO); > error =3D copyin(ifr->ifr_buffer.buffer, buf, > ifr->ifr_buffer.length - 1); > if (error) { > free(buf, M_IFDESCR); > break; > } > sx_xlock(&ifdescr_lock); > ifp->if_description =3D buf; > sx_xunlock(&ifdescr_lock); > break; >=20 > Note that this takes approach 1) from above, but it is also a moot = point now=20 > since the 'get' ioctl doesn't allocate memory anymore. This code seems pretty reasonable to me, but there's a race here if two = threads try to use the set ioctl on the same interface at once. We = should test whether ifp->if_description is already set after the = sx_xlock() above, and swap pointers, freeing the old one after = xunlock(), if so. Robert=