| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 1 Aug 2018 17:52:05 +0800
From: Tonghao Zhang <xiangxia.m.yue@...il.com>
To: jasowang@...hat.com
Cc: mst@...hat.com, makita.toshiaki@....ntt.co.jp,
virtualization@...ts.linux-foundation.org,
Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic
to vhost_net_busy_poll()
On Wed, Aug 1, 2018 at 2:01 PM Jason Wang <jasowang@...hat.com> wrote:
>
>
>
> On 2018年08月01日 11:00, xiangxia.m.yue@...il.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@...il.com>
> >
> > Factor out generic busy polling logic and will be
> > used for in tx path in the next patch. And with the patch,
> > qemu can set differently the busyloop_timeout for rx queue.
> >
> > In the handle_tx, the busypoll will vhost_net_disable/enable_vq
> > because we have poll the sock. This can improve performance.
> > [This is suggested by Toshiaki Makita <makita.toshiaki@....ntt.co.jp>]
> >
> > And when the sock receive skb, we should queue the poll if necessary.
> >
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@...il.com>
> > ---
> > drivers/vhost/net.c | 131 ++++++++++++++++++++++++++++++++++++----------------
> > 1 file changed, 91 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 32c1b52..5b45463 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -440,6 +440,95 @@ static void vhost_net_signal_used(struct vhost_net_virtqueue *nvq)
> > nvq->done_idx = 0;
> > }
> >
> > +static int sk_has_rx_data(struct sock *sk)
> > +{
> > + struct socket *sock = sk->sk_socket;
> > +
> > + if (sock->ops->peek_len)
> > + return sock->ops->peek_len(sock);
> > +
> > + return skb_queue_empty(&sk->sk_receive_queue);
> > +}
> > +
> > +static void vhost_net_busy_poll_try_queue(struct vhost_net *net,
> > + struct vhost_virtqueue *vq)
> > +{
> > + if (!vhost_vq_avail_empty(&net->dev, vq)) {
> > + vhost_poll_queue(&vq->poll);
> > + } else if (unlikely(vhost_enable_notify(&net->dev, vq))) {
> > + vhost_disable_notify(&net->dev, vq);
> > + vhost_poll_queue(&vq->poll);
> > + }
> > +}
> > +
> > +static void vhost_net_busy_poll_check(struct vhost_net *net,
> > + struct vhost_virtqueue *rvq,
> > + struct vhost_virtqueue *tvq,
> > + bool rx)
> > +{
> > + struct socket *sock = rvq->private_data;
> > +
> > + if (rx)
> > + vhost_net_busy_poll_try_queue(net, tvq);
> > + else if (sock && sk_has_rx_data(sock->sk))
> > + vhost_net_busy_poll_try_queue(net, rvq);
> > + else {
> > + /* On tx here, sock has no rx data, so we
> > + * will wait for sock wakeup for rx, and
> > + * vhost_enable_notify() is not needed. */
>
> A possible case is we do have rx data but guest does not refill the rx
> queue. In this case we may lose notifications from guest.
Yes, should consider this case. thanks.
> > + }
> > +}
> > +
> > +static void vhost_net_busy_poll(struct vhost_net *net,
> > + struct vhost_virtqueue *rvq,
> > + struct vhost_virtqueue *tvq,
> > + bool *busyloop_intr,
> > + bool rx)
> > +{
> > + unsigned long busyloop_timeout;
> > + unsigned long endtime;
> > + struct socket *sock;
> > + struct vhost_virtqueue *vq = rx ? tvq : rvq;
> > +
> > + mutex_lock_nested(&vq->mutex, rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
> > + vhost_disable_notify(&net->dev, vq);
> > + sock = rvq->private_data;
> > +
> > + busyloop_timeout = rx ? rvq->busyloop_timeout:
> > + tvq->busyloop_timeout;
> > +
> > +
> > + /* Busypoll the sock, so don't need rx wakeups during it. */
> > + if (!rx)
> > + vhost_net_disable_vq(net, vq);
>
> Actually this piece of code is not a factoring out. I would suggest to
> add this in another patch, or on top of this series.
I will add this in another patch.
> > +
> > + preempt_disable();
> > + endtime = busy_clock() + busyloop_timeout;
> > +
> > + while (vhost_can_busy_poll(endtime)) {
> > + if (vhost_has_work(&net->dev)) {
> > + *busyloop_intr = true;
> > + break;
> > + }
> > +
> > + if ((sock && sk_has_rx_data(sock->sk) &&
> > + !vhost_vq_avail_empty(&net->dev, rvq)) ||
> > + !vhost_vq_avail_empty(&net->dev, tvq))
> > + break;
>
> Some checks were duplicated in vhost_net_busy_poll_check(). Need
> consider to unify them.
OK
> > +
> > + cpu_relax();
> > + }
> > +
> > + preempt_enable();
> > +
> > + if (!rx)
> > + vhost_net_enable_vq(net, vq);
>
> No need to enable rx virtqueue, if we are sure handle_rx() will be
> called soon.
If we disable rx virtqueue in handle_tx and don't send packets from
guest anymore(handle_tx is not called), so we can wake up for sock rx.
so the network is broken.
> > +
> > + vhost_net_busy_poll_check(net, rvq, tvq, rx);
>
> It looks to me just open code all check here is better and easier to be
> reviewed.
will be changed.
> Thanks
>
> > +
> > + mutex_unlock(&vq->mutex);
> > +}
> > +
> > static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
> > struct vhost_net_virtqueue *nvq,
> > unsigned int *out_num, unsigned int *in_num,
> > @@ -753,16 +842,6 @@ static int peek_head_len(struct vhost_net_virtqueue *rvq, struct sock *sk)
> > return len;
> > }
> >
> > -static int sk_has_rx_data(struct sock *sk)
> > -{
> > - struct socket *sock = sk->sk_socket;
> > -
> > - if (sock->ops->peek_len)
> > - return sock->ops->peek_len(sock);
> > -
> > - return skb_queue_empty(&sk->sk_receive_queue);
> > -}
> > -
> > static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
> > bool *busyloop_intr)
> > {
> > @@ -770,41 +849,13 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
> > struct vhost_net_virtqueue *tnvq = &net->vqs[VHOST_NET_VQ_TX];
> > struct vhost_virtqueue *rvq = &rnvq->vq;
> > struct vhost_virtqueue *tvq = &tnvq->vq;
> > - unsigned long uninitialized_var(endtime);
> > int len = peek_head_len(rnvq, sk);
> >
> > - if (!len && tvq->busyloop_timeout) {
> > + if (!len && rvq->busyloop_timeout) {
> > /* Flush batched heads first */
> > vhost_net_signal_used(rnvq);
> > /* Both tx vq and rx socket were polled here */
> > - mutex_lock_nested(&tvq->mutex, VHOST_NET_VQ_TX);
> > - vhost_disable_notify(&net->dev, tvq);
> > -
> > - preempt_disable();
> > - endtime = busy_clock() + tvq->busyloop_timeout;
> > -
> > - while (vhost_can_busy_poll(endtime)) {
> > - if (vhost_has_work(&net->dev)) {
> > - *busyloop_intr = true;
> > - break;
> > - }
> > - if ((sk_has_rx_data(sk) &&
> > - !vhost_vq_avail_empty(&net->dev, rvq)) ||
> > - !vhost_vq_avail_empty(&net->dev, tvq))
> > - break;
> > - cpu_relax();
> > - }
> > -
> > - preempt_enable();
> > -
> > - if (!vhost_vq_avail_empty(&net->dev, tvq)) {
> > - vhost_poll_queue(&tvq->poll);
> > - } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
> > - vhost_disable_notify(&net->dev, tvq);
> > - vhost_poll_queue(&tvq->poll);
> > - }
> > -
> > - mutex_unlock(&tvq->mutex);
> > + vhost_net_busy_poll(net, rvq, tvq, busyloop_intr, true);
> >
> > len = peek_head_len(rnvq, sk);
> > }
>
Powered by blists - more mailing lists