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

Willem de Bruijn
2017-09-28T00:55:13Z

> @@ -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.

> + if (zcopy) {
> + heads = &used;

Can this special case of batchsize 1 not use vq->heads?

> + 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.

> + } 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.