On Thu, Apr 30, 2020 at 03:43:00PM -0400, Michael S. Tsirkin wrote:
> On Thu, Apr 30, 2020 at 06:25:21PM +0200, Stefano Garzarella wrote:
> > On Thu, Apr 30, 2020 at 10:06:26AM +0000, Justin He wrote:
> > > Hi Stefano
> > >
> > > > -----Original Message-----
> > > > From: Stefano Garzarella <sgarz...@redhat.com>
> > > > Sent: Thursday, April 30, 2020 4:26 PM
> > > > To: Justin He <justin...@arm.com>
> > > > Cc: Stefan Hajnoczi <stefa...@redhat.com>; Michael S. Tsirkin
> > > > <m...@redhat.com>; Jason Wang <jasow...@redhat.com>;
> > > > k...@vger.kernel.org; virtualizat...@lists.linux-foundation.org;
> > > > net...@vger.kernel.org; linux-kernel@vger.kernel.org; Kaly Xin
> > > > <kaly....@arm.com>
> > > > Subject: Re: [PATCH] vhost: vsock: don't send pkt when vq is not started
> > > >
> > > > Hi Jia,
> > > > thanks for the patch, some comments below:
> > > >
> > > > On Thu, Apr 30, 2020 at 10:13:14AM +0800, Jia He wrote:
> > > > > Ning Bo reported an abnormal 2-second gap when booting Kata container
> > > > [1].
> > > > > The unconditional timeout is caused by
> > > > VSOCK_DEFAULT_CONNECT_TIMEOUT of
> > > > > connect at client side. The vhost vsock client tries to connect an
> > > > > initlizing virtio vsock server.
> > > > >
> > > > > The abnormal flow looks like:
> > > > > host-userspace vhost vsock guest vsock
> > > > > ============== ===========
> > > > > ============
> > > > > connect() --------> vhost_transport_send_pkt_work()
> > > > > initializing
> > > > > | vq->private_data==NULL
> > > > > | will not be queued
> > > > > V
> > > > > schedule_timeout(2s)
> > > > > vhost_vsock_start() <--------- device
> > > > > ready
> > > > > set vq->private_data
> > > > >
> > > > > wait for 2s and failed
> > > > >
> > > > > connect() again vq->private_data!=NULL recv
> > > > > connecting pkt
> > > > >
> > > > > 1. host userspace sends a connect pkt, at that time, guest vsock is
> > > > > under
> > > > > initializing, hence the vhost_vsock_start has not been called. So
> > > > > vq->private_data==NULL, and the pkt is not been queued to send to
> > > > > guest.
> > > > > 2. then it sleeps for 2s
> > > > > 3. after guest vsock finishes initializing, vq->private_data is set.
> > > > > 4. When host userspace wakes up after 2s, send connecting pkt again,
> > > > > everything is fine.
> > > > >
> > > > > This fixes it by checking vq->private_data in
> > > > > vhost_transport_send_pkt,
> > > > > and return at once if !vq->private_data. This makes user connect()
> > > > > be returned with ECONNREFUSED.
> > > > >
> > > > > After this patch, kata-runtime (with vsock enabled) boottime reduces
> > > > > from
> > > > > 3s to 1s on ThunderX2 arm64 server.
> > > > >
> > > > > [1] https://github.com/kata-containers/runtime/issues/1917
> > > > >
> > > > > Reported-by: Ning Bo <n...@live.com>
> > > > > Signed-off-by: Jia He <justin...@arm.com>
> > > > > ---
> > > > > drivers/vhost/vsock.c | 8 ++++++++
> > > > > 1 file changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> > > > > index e36aaf9ba7bd..67474334dd88 100644
> > > > > --- a/drivers/vhost/vsock.c
> > > > > +++ b/drivers/vhost/vsock.c
> > > > > @@ -241,6 +241,7 @@ vhost_transport_send_pkt(struct virtio_vsock_pkt
> > > > *pkt)
> > > > > {
> > > > > struct vhost_vsock *vsock;
> > > > > int len = pkt->len;
> > > > > +struct vhost_virtqueue *vq;
> > > > >
> > > > > rcu_read_lock();
> > > > >
> > > > > @@ -252,6 +253,13 @@ vhost_transport_send_pkt(struct virtio_vsock_pkt
> > > > *pkt)
> > > > > return -ENODEV;
> > > > > }
> > > > >
> > > > > +vq = &vsock->vqs[VSOCK_VQ_RX];
> > > > > +if (!vq->private_data) {
> > > >
> > > > I think is better to use vhost_vq_get_backend():
> > > >
> > > > if (!vhost_vq_get_backend(&vsock->vqs[VSOCK_VQ_RX])) {
> > > > ...
> > > >
> > > > This function should be called with 'vq->mutex' acquired as explained in
> > > > the comment, but here we can avoid that, because we are not using the
> > > > vq,
> > > > so it is safe, because in vhost_transport_do_send_pkt() we check it
> > > > again.
> > > >
> > > > Please add a comment explaining that.
> > > >
> > >
> > > Thanks, vhost_vq_get_backend is better. I chose a 5.3 kernel to develop
> > > and missed this helper.
> >
> > :-)
> >
> > > >
> > > > As an alternative to this patch, should we kick the send worker when the
> > > > device is ready?
> > > >
> > > > IIUC we reach the timeout because the send worker (that runs
> > > > vhost_transport_do_send_pkt()) exits immediately since
> > > > 'vq->private_data'
> > > > is NULL, and no one will requeue it.
> > > >
> > > > Let's do it when we know the device is ready:
> > > >
> > > > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> > > > index e36aaf9ba7bd..295b5867944f 100644
> > > > --- a/drivers/vhost/vsock.c
> > > > +++ b/drivers/vhost/vsock.c
> > > > @@ -543,6 +543,11 @@ static int vhost_vsock_start(struct vhost_vsock
> > > > *vsock)
> > > > mutex_unlock(&vq->mutex);
> > > > }
> > > >
> > > > + /* Some packets may have been queued before the device was
> > > > started,
> > > > + * let's kick the send worker to send them.
> > > > + */
> > > > + vhost_work_queue(&vsock->dev, &vsock->send_pkt_work);
> > > > +
> > > Yes, it works.
> > > But do you think a threshold should be set here to prevent the queue
> > > from being too long? E.g. the client user sends too many connect pkts
> > > in a short time before the server is completely ready.
> >
> > When the user call the connect() the socket status is moved to
> > SS_CONNECTING (see net/vmw_vsock/af_vsock.c), so another connect() on
> > the same socket will receive EALREADY error.
> >
> > If the user uses multiple sockets, the socket layer already check for
> > any limits, so I don't think we should put a threshold here.
> >
> > >
> > > > mutex_unlock(&vsock->dev.mutex);
> > > > return 0;
> > > >
> > > > I didn't test it, can you try if it fixes the issue?
> > > >
> > > > I'm not sure which is better...
> > > I don't know, either. Wait for more comments 😊
> >
> > I prefer the second option, because the device is in a transitional
> > state and a connect can block (for at most two seconds) until the device is
> > started.
> >
> > For the first option, I'm also not sure if ECONNREFUSED is the right error
> > to return, maybe is better ENETUNREACH.
> >
> > Cheers,
> > Stefano
>
> IIRC:
>
> ECONNREFUSED is what one gets when connecting to remote a port which does not
> yet have a listening socket, so remote sends back RST.
> ENETUNREACH is when local network's down, so you can't even send a
> connection request.
> EHOSTUNREACH is remote network is down.
Thanks for the clarification!
I was looking at connect(2) man page and there isn't EHOSTUNREACH in the
ERRORS section :-(
But connect(3p) contains the following that match what you said:
ECONNRESET
Remote host reset the connection request.
ENETUNREACH
No route to the network is present.
EHOSTUNREACH
The destination host cannot be reached (probably because
the host is down or a remote router cannot reach it).
So in this case, I think ENETUNREACH should be the best one, since the
device is down and we can't send the connection request, but also
EHOSTUNREACH should fit...
In af_vsock.c we already return ENETUNREACH when the stream is not allowed
or we don't have a transport to use.
Thanks,
Stefano
