On Mon, Apr 11, 2016 at 03:54:08PM +0300, Michael S. Tsirkin wrote:

On Mon, Apr 11, 2016 at 11:45:48AM +0100, Stefan Hajnoczi wrote:

quoted

On Fri, Apr 08, 2016 at 04:35:05PM +0100, Ian Campbell wrote:

quoted

On Fri, 2016-04-01 at 15:23 +0100, Stefan Hajnoczi wrote:

quoted

This series is based on Michael Tsirkin's vhost branch (v4.5-rc6).

I'm about to process Claudio Imbrenda's locking fixes for virtio-vsock but
first I want to share the latest version of the code.  Several people are
playing with vsock now so sharing the latest code should avoid duplicate work.

Thanks for this, I've been using it in my project and it mostly seems
fine.

One wrinkle I came across, which I'm not sure if it is by design or a
problem is that I can see this sequence coming from the guest (with
other activity in between):

    1) OP_SHUTDOWN w/ flags == SHUTDOWN_RX
    2) OP_SHUTDOWN w/ flags == SHUTDOWN_TX
    3) OP_SHUTDOWN w/ flags == SHUTDOWN_TX|SHUTDOWN_RX

How did you trigger this sequence?  I'd like to reproduce it.

quoted

quoted

I orignally had my backend close things down at #2, however this meant
that when #3 arrived it was for a non-existent socket (or, worse, an
active one if the ports got reused). I checked v5 of the spec
proposal[0] which says:
    If these bits are set and there are no more virtqueue buffers
    pending the socket is disconnected.

but I'm not entirely sure if this behaviour contradicts this or not
(the bits have both been set at #2, but not at the same time).

BTW, how does one tell if there are no more virtqueue buffers pending
or not while processing the op?

#2 is odd.  The shutdown bits are sticky so they cannot be cleared once
set.  I would have expected just #1 and #3.  The behavior you observe
look like a bug.

The spec text does not convey the meaning of OP_SHUTDOWN well.
OP_SHUTDOWN SHUTDOWN_TX|SHUTDOWN_RX means no further rx/tx is possible
for this connection.  "there are no more virtqueue buffers pending the
socket" really means that this isn't an immediate close from the
perspective of the application.  If the application still has unread rx
buffers then the socket stays readable until the rx data has been fully
read.

Yes but you also wrote:
	If these bits are set and there are no more virtqueue buffers
	pending the socket is disconnected.

how does remote know that there are no buffers pending and so it's safe
to reuse the same source/destination address now?  Maybe destination
should send RST at that point?

You are right, the source/destination address could be reused while the
remote still has the connection in their table.  Connection
establishment would fail with a RST reply.

I can think of two solutions:

1. Implementations must remove connections from their table as soon as
   SHUTDOWN_TX|SHUTDOWN_RX is received.  This way the source/destination
   address tuple can be reused immediately, i.e. new connections with
   the same source/destination would be possible while an application is
   still draining the receive buffers of an old connection.

2. Extend the connection lifecycle so that an A->B
   SHUTDOWN_TX|SHUTDOWN_RX must be followed by a by a B->A RST to close
   a connection.  This way the source/destination address is only in use
   once at a time.

Option #2 seems safer because there is no overlap in source/destination
address usage.