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
> 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:
>
> 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 = priv_check();
> if (error)
> break;
>
> if (ifr->ifr_buffer.length > ifdescr_maxlen)
> return (ENAMETOOLONG);
>
> buf = malloc(ifr->ifr_buffer.length, M_IFDESCR, M_WAITOK |
> M_ZERO);
> error = 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 = buf;
> sx_xunlock(&ifdescr_lock);
> break;
>
> Note that this takes approach 1) from above, but it is also a moot point now
> 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_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[email protected]"
