Re: [PATCH net-next RFC 5/5] vhost_net: basic tx virtqueue batched processing

[Jason Wang]

On 2017å09æ28æ 08:55, Willem de Bruijn wrote:
> > @@ -461,6 +460,7 @@ static void handle_tx(struct vhost_net *net)
> >          struct socket *sock;
> >          struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
> >          bool zcopy, zcopy_used;
> > +       int i, batched = VHOST_NET_BATCH;
> >
> >          mutex_lock(&vq->mutex);
> >          sock = vq->private_data;
> > @@ -475,6 +475,12 @@ static void handle_tx(struct vhost_net *net)
> >          hdr_size = nvq->vhost_hlen;
> >          zcopy = nvq->ubufs;
> >
> > +       /* Disable zerocopy batched fetching for simplicity */
> This special case can perhaps be avoided if we no longer block
> on vhost_exceeds_maxpend, but revert to copying.

Yes, I think so. For simplicity, I do it for data copy first. If the 
idea is convinced, I will try to do zerocopy on top.

> > +       if (zcopy) {
> > +               heads = &used;
> Can this special case of batchsize 1 not use vq->heads?

It doesn't in fact?

> > +               batched = 1;
> > +       }
> > +
> >          for (;;) {
> >                  /* Release DMAs done buffers first */
> >                  if (zcopy)
> > @@ -486,95 +492,114 @@ static void handle_tx(struct vhost_net *net)
> >                  if (unlikely(vhost_exceeds_maxpend(net)))
> >                          break;
> > +                       /* TODO: Check specific error and bomb out
> > +                        * unless ENOBUFS?
> > +                        */
> > +                       err = sock->ops->sendmsg(sock, &msg, len);
> > +                       if (unlikely(err < 0)) {
> > +                               if (zcopy_used) {
> > +                                       vhost_net_ubuf_put(ubufs);
> > +                                       nvq->upend_idx =
> > +                                  ((unsigned)nvq->upend_idx - 1) % UIO_MAXIOV;
> > +                               }
> > +                               vhost_discard_vq_desc(vq, 1);
> > +                               goto out;
> > +                       }
> > +                       if (err != len)
> > +                               pr_debug("Truncated TX packet: "
> > +                                       " len %d != %zd\n", err, len);
> > +                       if (!zcopy) {
> > +                               vhost_add_used_idx(vq, 1);
> > +                               vhost_signal(&net->dev, vq);
> > +                       } else if (!zcopy_used) {
> > +                               vhost_add_used_and_signal(&net->dev,
> > +                                                         vq, head, 0);
> While batching, perhaps can also move this producer index update
> out of the loop and using vhost_add_used_and_signal_n.

Yes.

> > +                       } else
> > +                               vhost_zerocopy_signal_used(net, vq);
> > +                       vhost_net_tx_packet(net);
> > +                       if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
> > +                               vhost_poll_queue(&vq->poll);
> > +                               goto out;
> >                          }
> > -                       vhost_discard_vq_desc(vq, 1);
> > -                       break;
> > -               }
> > -               if (err != len)
> > -                       pr_debug("Truncated TX packet: "
> > -                                " len %d != %zd\n", err, len);
> > -               if (!zcopy_used)
> > -                       vhost_add_used_and_signal(&net->dev, vq, head, 0);
> > -               else
> > -                       vhost_zerocopy_signal_used(net, vq);
> > -               vhost_net_tx_packet(net);
> > -               if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
> > -                       vhost_poll_queue(&vq->poll);
> > -                       break;
> This patch touches many lines just for indentation. If having to touch
> these lines anyway (dirtying git blame), it may be a good time to move
> the processing of a single descriptor code into a separate helper function.
> And while breaking up, perhaps another helper for setting up ubuf_info.
> If you agree, preferably in a separate noop refactor patch that precedes
> the functional changes.

Right and it looks better, will try to do this.

Thanks