Date:   Fri, 1 May 2020 16:37:16 +0200
From:   Stefano Garzarella <sgarzare@...hat.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Justin He <Justin.He@....com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Kaly Xin <Kaly.Xin@....com>
Subject: Re: [PATCH] vhost: vsock: don't send pkt when vq is not started

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 <sgarzare@...hat.com>
> > > > Sent: Thursday, April 30, 2020 4:26 PM
> > > > To: Justin He <Justin.He@....com>
> > > > Cc: Stefan Hajnoczi <stefanha@...hat.com>; Michael S. Tsirkin
> > > > <mst@...hat.com>; Jason Wang <jasowang@...hat.com>;
> > > > kvm@...r.kernel.org; virtualization@...ts.linux-foundation.org;
> > > > netdev@...r.kernel.org; linux-kernel@...r.kernel.org; Kaly Xin
> > > > <Kaly.Xin@....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.b@...e.com>
> > > > > Signed-off-by: Jia He <justin.he@....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

Powered by blists - more mailing lists