Re: possible selrecord optimization ?
On Thu, Jan 23, 2014 at 02:52:41PM -0500, John Baldwin wrote: > On Wednesday, January 22, 2014 7:39:48 pm Luigi Rizzo wrote: ... > > 2. am i correct that we do need to protect concurrent invocations > >of selrecord() on the same selinfo because mtx_pool_find() > >return the same mutex for a given struct selinfo ? > > If you mean 'do not need', yes. mtx_pool_find() does a hash on the address, yes, i meant "do not need". thanks luigi ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: possible selrecord optimization ?
On Wednesday, January 22, 2014 7:39:48 pm Luigi Rizzo wrote: > On Wed, Jan 22, 2014 at 02:29:56PM -0500, John Baldwin wrote: > > On Tuesday, January 21, 2014 9:25:27 pm Luigi Rizzo wrote: > > > Looking at how selrecord() / selwakeup() and their Linux counterparts > > > poll_wait() and wake_up() are used, i noticed the following: > > > > I wonder if we could use the same optimization as Linux: > > > as soon as pollscan/selscan detects a non-blocking fd, > > > make selrecord a no-op (which is probably as simple > > > as setting SELTD_RESCAN; and since it only goes up > > > we do not need to lock to check it). > > > > Yes, I think this would work fine. I think setting SELTD_RESCAN as a way > > to > > do it is fine as well. > > excellent, thanks. > > I also have two related questions: > > 1. why isn't the struct mtx part of the struct selinfo instead >of being grabbed from the mtxpool_select ? I think this is because there is no selinfo_init() and no selinfo_destroy(), so there is no way to manage the lifetime of the mutex were it embedded into the structure. Also, if there are a lot of these structures, but only a subset of them are ever accessed, then a smaller set of locks that are hashed onto the structures may work just fine without introducing extra contention (but with the benefit of saving space in the structures). > 2. am i correct that we do need to protect concurrent invocations >of selrecord() on the same selinfo because mtx_pool_find() >return the same mutex for a given struct selinfo ? If you mean 'do not need', yes. mtx_pool_find() does a hash on the address, so it will always return the same lock for a given selinfo, and no external locking should be needed by callers. However, callers often need to check other state for which they need to hold a lock anyway. OTOH, if, for example, socket buffer locks were reader/writer locks, sopoll_generic() could use read locks on the socket buffers even while calling selrecord(). > In case, any objections if i add some comments to the code > to explain the above ? Not from me. :) -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: possible selrecord optimization ?
On Wed, Jan 22, 2014 at 02:29:56PM -0500, John Baldwin wrote: > On Tuesday, January 21, 2014 9:25:27 pm Luigi Rizzo wrote: > > Looking at how selrecord() / selwakeup() and their Linux counterparts > > poll_wait() and wake_up() are used, i noticed the following: > > I wonder if we could use the same optimization as Linux: > > as soon as pollscan/selscan detects a non-blocking fd, > > make selrecord a no-op (which is probably as simple > > as setting SELTD_RESCAN; and since it only goes up > > we do not need to lock to check it). > > Yes, I think this would work fine. I think setting SELTD_RESCAN as a way to > do it is fine as well. excellent, thanks. I also have two related questions: 1. why isn't the struct mtx part of the struct selinfo instead of being grabbed from the mtxpool_select ? 2. am i correct that we do need to protect concurrent invocations of selrecord() on the same selinfo because mtx_pool_find() return the same mutex for a given struct selinfo ? In case, any objections if i add some comments to the code to explain the above ? cheers luigi ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: possible selrecord optimization ?
On Tuesday, January 21, 2014 9:25:27 pm Luigi Rizzo wrote:
> Looking at how selrecord() / selwakeup() and their Linux counterparts
> poll_wait() and wake_up() are used, i noticed the following:
>
> - linux tends to call wake_up() unconditionally
> at the beginning of the poll handler
>
> - FreeBSD tends to call selrecord() only when it detects a blocking
> situation, and this also requires something (a lock or a retry;
> the lock in selinfo is not good for this) to avoid the race between
> the blocking_test..selrecord pair and the selwakeup().
>
> FreeBSD could call selrecord unconditionally (and save the extra
> lock/retry), but selrecord is expensive as it queues the thread on
> the struct selinfo, and this takes a lock.
>
> I wonder if we could use the same optimization as Linux:
> as soon as pollscan/selscan detects a non-blocking fd,
> make selrecord a no-op (which is probably as simple
> as setting SELTD_RESCAN; and since it only goes up
> we do not need to lock to check it).
Yes, I think this would work fine. I think setting SELTD_RESCAN as a way to
do it is fine as well.
> This way, we would pay at most for one extra selrecord per
> poll/select.
>
> Even more interesting, it could simplify the logic and locking
> in poll handlers.
>
> As an example, in sys/uipc_socket.c :: sopoll_generic()
> we could completely decouple the locks on so_snd and so_rcv.
Splitting the locks up is not really feasible because the so_rcv lock doubles
as SOCK_LOCK() and is needed even in the POLLOUT case as sowritable() needs it
for so_state. What you might be able to do instead is be a bit fancier and
only lock so_snd if writing is being polled for by making it conditional
on the values in events. This would avoid locking so_snd when just polling
for read:
Index: uipc_socket.c
===
--- uipc_socket.c (revision 261029)
+++ uipc_socket.c (working copy)
@@ -2912,7 +2912,12 @@ sopoll_generic(struct socket *so, int events, stru
{
int revents = 0;
- SOCKBUF_LOCK(&so->so_snd);
+ if (events & (POLLOUT | POLLWRNORM))
+ SOCKBUF_LOCK(&so->so_snd);
+ /*
+* The so_rcv lock doubles as SOCK_LOCK so it it is needed for
+* all requests.
+*/
SOCKBUF_LOCK(&so->so_rcv);
if (events & (POLLIN | POLLRDNORM))
if (soreadabledata(so))
@@ -2947,7 +2952,8 @@ sopoll_generic(struct socket *so, int events, stru
}
SOCKBUF_UNLOCK(&so->so_rcv);
- SOCKBUF_UNLOCK(&so->so_snd);
+ if (events & (POLLOUT | POLLWRNORM))
+ SOCKBUF_UNLOCK(&so->so_snd);
return (revents);
}
--
John Baldwin
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
