Date: Wed, 18 Nov 2009 15:35:36 +0000 From: "Robert N. M. Watson" <rwatson@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: FreeBSD Net <freebsd-net@freebsd.org>, Brooks Davis <brooks@freebsd.org>, "M. Warner Losh" <imp@bsdimp.com>, Antoine Brodin <antoine.brodin@laposte.net> Subject: Re: [PATCH FOR REVIEW] interface description (revised) Message-ID: <70A1A5B3-1F19-4AC1-8975-F7E950785BBF@freebsd.org> In-Reply-To: <200911181012.07167.jhb@freebsd.org> References: <a78074950911171839u3e3fb4f1oae4aa3fc79f1b152@mail.gmail.com> <01D9CB64-F04C-4506-ACF2-1DE459FC69CD@freebsd.org> <200911181012.07167.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?70A1A5B3-1F19-4AC1-8975-F7E950785BBF>