Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1741113 > unrolled thread

Re: [PATCH net-next RFC 2/5] vhost: introduce helper to prefetch desc index

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH net-next RFC 2/5] vhost: introduce helper to prefetch desc index Willem de Bruijn <willemdebruijn.kernel@gmail.com> - 2017-09-28 02:50 +0200
    Re: [PATCH net-next RFC 2/5] vhost: introduce helper to prefetch desc  index Jason Wang <jasowang@redhat.com> - 2017-09-28 09:50 +0200

#1741113 — Re: [PATCH net-next RFC 2/5] vhost: introduce helper to prefetch desc index

On Fri, Sep 22, 2017 at 4:02 AM, Jason Wang <jasowang@redhat.com> wrote:
> This patch introduces vhost_prefetch_desc_indices() which could batch
> descriptor indices fetching and used ring updating. This intends to
> reduce the cache misses of indices fetching and updating and reduce
> cache line bounce when virtqueue is almost full. copy_to_user() was
> used in order to benefit from modern cpus that support fast string
> copy. Batched virtqueue processing will be the first user.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/vhost/vhost.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/vhost/vhost.h |  3 +++
>  2 files changed, 58 insertions(+)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index f87ec75..8424166d 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2437,6 +2437,61 @@ struct vhost_msg_node *vhost_dequeue_msg(struct vhost_dev *dev,
>  }
>  EXPORT_SYMBOL_GPL(vhost_dequeue_msg);
>
> +int vhost_prefetch_desc_indices(struct vhost_virtqueue *vq,
> +                               struct vring_used_elem *heads,
> +                               u16 num, bool used_update)
> +{
> +       int ret, ret2;
> +       u16 last_avail_idx, last_used_idx, total, copied;
> +       __virtio16 avail_idx;
> +       struct vring_used_elem __user *used;
> +       int i;
> +
> +       if (unlikely(vhost_get_avail(vq, avail_idx, &vq->avail->idx))) {
> +               vq_err(vq, "Failed to access avail idx at %p\n",
> +                      &vq->avail->idx);
> +               return -EFAULT;
> +       }
> +       last_avail_idx = vq->last_avail_idx & (vq->num - 1);
> +       vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
> +       total = vq->avail_idx - vq->last_avail_idx;
> +       ret = total = min(total, num);
> +
> +       for (i = 0; i < ret; i++) {
> +               ret2 = vhost_get_avail(vq, heads[i].id,
> +                                     &vq->avail->ring[last_avail_idx]);
> +               if (unlikely(ret2)) {
> +                       vq_err(vq, "Failed to get descriptors\n");
> +                       return -EFAULT;
> +               }
> +               last_avail_idx = (last_avail_idx + 1) & (vq->num - 1);
> +       }

This is understandably very similar to the existing logic in vhost_get_vq_desc.
Can that be extracted to a helper to avoid code duplication?

Perhaps one helper to update vq->avail_idx and return num, and
another to call vhost_get_avail one or more times.

> +
> +       if (!used_update)
> +               return ret;
> +
> +       last_used_idx = vq->last_used_idx & (vq->num - 1);
> +       while (total) {
> +               copied = min((u16)(vq->num - last_used_idx), total);
> +               ret2 = vhost_copy_to_user(vq,
> +                                         &vq->used->ring[last_used_idx],
> +                                         &heads[ret - total],
> +                                         copied * sizeof(*used));
> +
> +               if (unlikely(ret2)) {
> +                       vq_err(vq, "Failed to update used ring!\n");
> +                       return -EFAULT;
> +               }
> +
> +               last_used_idx = 0;
> +               total -= copied;
> +       }

This second part seems unrelated and could be a separate function?

Also, no need for ret2 and double assignment "ret = total =" if not
modifying total
in the the second loop:

  for (i = 0; i < total; ) {
    ...
    i += copied;
  }

[toc] | [next] | [standalone]

#1741238 — Re: [PATCH net-next RFC 2/5] vhost: introduce helper to prefetch desc index

On 2017年09月28日 08:47, Willem de Bruijn wrote:
> On Fri, Sep 22, 2017 at 4:02 AM, Jason Wang <jasowang@redhat.com> wrote:
>> This patch introduces vhost_prefetch_desc_indices() which could batch
>> descriptor indices fetching and used ring updating. This intends to
>> reduce the cache misses of indices fetching and updating and reduce
>> cache line bounce when virtqueue is almost full. copy_to_user() was
>> used in order to benefit from modern cpus that support fast string
>> copy. Batched virtqueue processing will be the first user.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   drivers/vhost/vhost.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/vhost/vhost.h |  3 +++
>>   2 files changed, 58 insertions(+)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index f87ec75..8424166d 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -2437,6 +2437,61 @@ struct vhost_msg_node *vhost_dequeue_msg(struct vhost_dev *dev,
>>   }
>>   EXPORT_SYMBOL_GPL(vhost_dequeue_msg);
>>
>> +int vhost_prefetch_desc_indices(struct vhost_virtqueue *vq,
>> +                               struct vring_used_elem *heads,
>> +                               u16 num, bool used_update)
>> +{
>> +       int ret, ret2;
>> +       u16 last_avail_idx, last_used_idx, total, copied;
>> +       __virtio16 avail_idx;
>> +       struct vring_used_elem __user *used;
>> +       int i;
>> +
>> +       if (unlikely(vhost_get_avail(vq, avail_idx, &vq->avail->idx))) {
>> +               vq_err(vq, "Failed to access avail idx at %p\n",
>> +                      &vq->avail->idx);
>> +               return -EFAULT;
>> +       }
>> +       last_avail_idx = vq->last_avail_idx & (vq->num - 1);
>> +       vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
>> +       total = vq->avail_idx - vq->last_avail_idx;
>> +       ret = total = min(total, num);
>> +
>> +       for (i = 0; i < ret; i++) {
>> +               ret2 = vhost_get_avail(vq, heads[i].id,
>> +                                     &vq->avail->ring[last_avail_idx]);
>> +               if (unlikely(ret2)) {
>> +                       vq_err(vq, "Failed to get descriptors\n");
>> +                       return -EFAULT;
>> +               }
>> +               last_avail_idx = (last_avail_idx + 1) & (vq->num - 1);
>> +       }
> This is understandably very similar to the existing logic in vhost_get_vq_desc.
> Can that be extracted to a helper to avoid code duplication?
>
> Perhaps one helper to update vq->avail_idx and return num, and
> another to call vhost_get_avail one or more times.

Yes it can.

>
>> +
>> +       if (!used_update)
>> +               return ret;
>> +
>> +       last_used_idx = vq->last_used_idx & (vq->num - 1);
>> +       while (total) {
>> +               copied = min((u16)(vq->num - last_used_idx), total);
>> +               ret2 = vhost_copy_to_user(vq,
>> +                                         &vq->used->ring[last_used_idx],
>> +                                         &heads[ret - total],
>> +                                         copied * sizeof(*used));
>> +
>> +               if (unlikely(ret2)) {
>> +                       vq_err(vq, "Failed to update used ring!\n");
>> +                       return -EFAULT;
>> +               }
>> +
>> +               last_used_idx = 0;
>> +               total -= copied;
>> +       }
> This second part seems unrelated and could be a separate function?

Yes.

>
> Also, no need for ret2 and double assignment "ret = total =" if not
> modifying total
> in the the second loop:
>
>    for (i = 0; i < total; ) {
>      ...
>      i += copied;
>    }

Right, will do this in V2.

Thanks

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web