[PATCH] Only lock send buffer in sopoll() if needed

[Robert N. M. Watson]

I'm not convinced that the race with SBS_CANTSENDMORE is OK, even though I really want to write that it is. The risk here is that we miss an asynchronous disconnect event, and that the thread then blocks even though an event is pending, which is a nasty turn of events. We might want to dig about a bit and see whether this case 'matters' -- e.g., that there are important cases where a test for writability might need to catch SBS_CANTRCVMORE but SBS_CANTSENDMORE is not simultaneously set.

Could you say a bit more about the performance benefits of this change -- is it substantial? If so, we might need to add a new socket-buffer flag along the lines of SB[RS]_DISCONNECTED that is 'broadcast' to both socket buffers on certain events. Doing so might require rejiggering elsewhere by causing additional locks to need to be held, possibly out of order.

Another thing that's worth pondering -- and it is a lot of work -- is finally sorting out the conflation of SOCK_LOCK() and SOCKBUF_LOCK(&so_rcv). We have a number of races in the socket state machine stemming from this conflation (due to lockless reads along the lines of the one you are proposing, only against so_state and friends). This is a non-trivial change, and it's not 100% clear to me how to make it, so would require quite a bit of analysis. It might have the effect of needing three non-trivial adjustments: (1) instantiating two new locks per socket, one sleepable, one a mutex; (2) rejiggering a set of 'global' socket properties out from under the receive lock, such as (but definitely not limited to) so_state; (3) doing some socket-layer vs. protocol-layer rejiggering so that protocol state machine is the 'primary' with event notifications to the socket layer to update its state, as opposed to the current world order where some state transitions are triggered by one layer, and some by the other. Although not pretty, you can see an example of a resolution of a socket-protocol race in the current behaviour of solisten, which used to drive the state machine from the socket layer, which conflicted with protocol-layer checks; now, the protocol uses the socket layer as a library to test (and later set) properties.

I don't think the issue you are looking at requires starting to dig into that can of worms, but it is a long-standing problem in socket locking / protocol-layer interaction that does need to get resolved in order to make the behaviour of conditions like the one you are looking at 'more natural'.

Robert

On 30 Sep 2014, at 19:00, John Baldwin <jhb at FreeBSD.org> wrote:

> Right now sopoll() always locks both socket buffers.  The receive socket 
> buffer lock is always needed, but the send socket buffer lock is only needed 
> while polling for writing (there is a potential test of SBS_CANTSENDMORE 
> without the lock, but I think this might be ok).  What do folks think?
> 
> Index: uipc_socket.c
> ===================================================================
> --- uipc_socket.c       (revision 272305)
> +++ uipc_socket.c       (working copy)
> @@ -3003,7 +3003,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))
> @@ -3038,7 +3043,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