lkml.org 
[lkml]   [2019]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/1] drivers/vhost : Removes unnecessary 'else' in vhost_copy_from_user
From
Date

On 2019/12/13 上午5:15, Leonardo Bras wrote:
> There is no need for this else statement, given that if block will return.
> This change is not supposed to change the output binary.
>
> It reduces identation level on most lines in this function, and also
> fixes an split string on vq_err().
>
> Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
> ---
> drivers/vhost/vhost.c | 50 +++++++++++++++++++++----------------------
> 1 file changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index f44340b41494..b23d1b74c32f 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -824,34 +824,32 @@ static int vhost_copy_from_user(struct vhost_virtqueue *vq, void *to,
>
> if (!vq->iotlb)
> return __copy_from_user(to, from, size);
> - else {
> - /* This function should be called after iotlb
> - * prefetch, which means we're sure that vq
> - * could be access through iotlb. So -EAGAIN should
> - * not happen in this case.
> - */
> - void __user *uaddr = vhost_vq_meta_fetch(vq,
> - (u64)(uintptr_t)from, size,
> - VHOST_ADDR_DESC);
> - struct iov_iter f;
>
> - if (uaddr)
> - return __copy_from_user(to, uaddr, size);
> + /* This function should be called after iotlb
> + * prefetch, which means we're sure that vq
> + * could be access through iotlb. So -EAGAIN should
> + * not happen in this case.
> + */
> + void __user *uaddr = vhost_vq_meta_fetch(vq,
> + (u64)(uintptr_t)from, size,
> + VHOST_ADDR_DESC);
> + struct iov_iter f;


I think this will lead at least warnings from compiler.

Generally, I would not bother to make changes like this especially
consider it will bring troubles when trying to backporting fixes to
downstream in the future.

There're some more interesting things: e.g current metadata IOTLB
performance is bad for dynamic mapping since it will be reset each time
a new updating is coming.

We can optimize this by only reset the metadata IOTLB when the updating
is for metdata.

Want to try this?

Thanks


>
> - ret = translate_desc(vq, (u64)(uintptr_t)from, size, vq->iotlb_iov,
> - ARRAY_SIZE(vq->iotlb_iov),
> - VHOST_ACCESS_RO);
> - if (ret < 0) {
> - vq_err(vq, "IOTLB translation failure: uaddr "
> - "%p size 0x%llx\n", from,
> - (unsigned long long) size);
> - goto out;
> - }
> - iov_iter_init(&f, READ, vq->iotlb_iov, ret, size);
> - ret = copy_from_iter(to, size, &f);
> - if (ret == size)
> - ret = 0;
> - }
> + if (uaddr)
> + return __copy_from_user(to, uaddr, size);
> +
> + ret = translate_desc(vq, (u64)(uintptr_t)from, size, vq->iotlb_iov,
> + ARRAY_SIZE(vq->iotlb_iov),
> + VHOST_ACCESS_RO);
> + if (ret < 0) {
> + vq_err(vq, "IOTLB translation failure: uaddr %p size 0x%llx\n",
> + from, (unsigned long long)size);
> + goto out;
> + }
> + iov_iter_init(&f, READ, vq->iotlb_iov, ret, size);
> + ret = copy_from_iter(to, size, &f);
> + if (ret == size)
> + ret = 0;
>
> out:
> return ret;

\
 
 \ /
  Last update: 2019-12-18 06:54    [W:0.050 / U:3.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site