Re: [RFC PATCH net-next 04/12] vhost_net: split out datacopy logic

Jason Wang
2018-05-22T12:40:00Z

On 2018年05月22日 00:46, Jesse Brandeburg wrote:
> On Mon, 21 May 2018 17:04:25 +0800 Jason wrote:
>> Instead of mixing zerocopy and datacopy logics, this patch tries to
>> split datacopy logic out. This results for a more compact code and
>> specific optimization could be done on top more easily.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> drivers/vhost/net.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 102 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index 4ebac76..4682fcc 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -492,9 +492,95 @@ static bool vhost_has_more_pkts(struct vhost_net *net,
>> likely(!vhost_exceeds_maxpend(net));
>> }
>>
>> +static void handle_tx_copy(struct vhost_net *net)
>> +{
>> + struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
>> + struct vhost_virtqueue *vq = &nvq->vq;
>> + unsigned out, in;
> move "out" and "in" down to inside the for loop as well.
>
>> + int head;
> move this "head" declaration inside for-loop below.

Will do these in next version, basically this function were just copied
from handle_tx_zerocopy and remove unnecessary parts. So I'm thinking an
independent patch from the beginning to avoid changes in two places.
>
>> + struct msghdr msg = {
>> + .msg_name = NULL,
>> + .msg_namelen = 0,
>> + .msg_control = NULL,
>> + .msg_controllen = 0,
>> + .msg_flags = MSG_DONTWAIT,
>> + };
>> + size_t len, total_len = 0;
>> + int err;
>> + size_t hdr_size;
>> + struct socket *sock;
>> + struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
>> + int sent_pkts = 0;
> why do we need so many locals?

So it looks to me ubufs is unnecessary but all other is really needed.

>
>> +
>> + mutex_lock(&vq->mutex);
>> + sock = vq->private_data;
>> + if (!sock)
>> + goto out;
>> +
>> + if (!vq_iotlb_prefetch(vq))
>> + goto out;
>> +
>> + vhost_disable_notify(&net->dev, vq);
>> + vhost_net_disable_vq(net, vq);
>> +
>> + hdr_size = nvq->vhost_hlen;
>> +
>> + for (;;) {
>> + head = vhost_net_tx_get_vq_desc(net, vq, vq->iov,
>> + ARRAY_SIZE(vq->iov),
>> + &out, &in);
>> + /* On error, stop handling until the next kick. */
>> + if (unlikely(head < 0))
>> + break;
>> + /* Nothing new? Wait for eventfd to tell us they refilled. */
>> + if (head == vq->num) {
>> + if (unlikely(vhost_enable_notify(&net->dev, vq))) {
>> + vhost_disable_notify(&net->dev, vq);
>> + continue;
>> + }
>> + break;
>> + }
>> + if (in) {
>> + vq_err(vq, "Unexpected descriptor format for TX: "
>> + "out %d, int %d\n", out, in);
> don't break strings, keep all the bits between " " together, even if it
> is longer than 80 chars.

Ok.

>
>> + break;
>> + }
>> +
>> + len = init_iov_iter(vq, &msg.msg_iter, hdr_size, out);
>> + if (len < 0)
>> + break;
> same comment as previous patch, len is a size_t which is unsigned.

Yes.

>
>> +
>> + total_len += len;
>> + if (total_len < VHOST_NET_WEIGHT &&
>> + vhost_has_more_pkts(net, vq)) {
>> + msg.msg_flags |= MSG_MORE;
>> + } else {
>> + msg.msg_flags &= ~MSG_MORE;
>> + }
> don't need { } here.

Right.

>> +
>> + /* TODO: Check specific error and bomb out unless ENOBUFS? */
>> + err = sock->ops->sendmsg(sock, &msg, len);
>> + if (unlikely(err < 0)) {
>> + vhost_discard_vq_desc(vq, 1);
>> + vhost_net_enable_vq(net, vq);
>> + break;
>> + }
>> + if (err != len)
>> + pr_debug("Truncated TX packet: "
>> + " len %d != %zd\n", err, len);
> single line " " string please
>

Will fix.

Thanks