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>