diff mbox series

vhost-user: Skip unnecessary duplicated VHOST_USER_SET_LOG_BASE requests

Message ID 20240613065150.3100-1-xiangwencheng@dayudpu.com
State New
Headers show
Series vhost-user: Skip unnecessary duplicated VHOST_USER_SET_LOG_BASE requests | expand

Commit Message

BillXiang June 13, 2024, 6:51 a.m. UTC 
From: BillXiang <xiangwencheng@dayudpu.com>

The VHOST_USER_SET_LOG_BASE requests should be categorized into
non-vring specific messages, and should be sent only once.
If send more than once, dpdk will munmap old log_addr which may has been used and cause segmentation fault.

Signed-off-by: BillXiang <xiangwencheng@dayudpu.com>
---
 hw/virtio/vhost-user.c | 1 +
 1 file changed, 1 insertion(+)

Comments

BillXiang July 1, 2024, 7:50 a.m. UTC | #1 
> From: "项文成"<xiangwencheng@dayudpu.com>
> Date:  Thu, Jun 13, 2024, 14:51
> Subject:  [PATCH] vhost-user: Skip unnecessary duplicated VHOST_USER_SET_LOG_BASE requests
> To: <qemu-devel@nongnu.org>
> Cc: <mst@redhat.com>, "BillXiang"<xiangwencheng@dayudpu.com>
> From: BillXiang <xiangwencheng@dayudpu.com>
> 
> The VHOST_USER_SET_LOG_BASE requests should be categorized into
> non-vring specific messages, and should be sent only once.
> If send more than once, dpdk will munmap old log_addr which may has been used and cause segmentation fault.
> 
> Signed-off-by: BillXiang <xiangwencheng@dayudpu.com>
> ---
>  hw/virtio/vhost-user.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index cdf9af4a4b..41e34edd49 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -371,6 +371,7 @@ static bool vhost_user_per_device_request(VhostUserRequest request)
>      case VHOST_USER_RESET_DEVICE:
>      case VHOST_USER_ADD_MEM_REG:
>      case VHOST_USER_REM_MEM_REG:
> +    case VHOST_USER_SET_LOG_BASE:
>          return true;
>      default:
>          return false;
> -- 
> 2.30.0
ping
Alex Bennée July 1, 2024, 8:49 a.m. UTC | #2 
项文成 <xiangwencheng@dayudpu.com> writes:

> From: BillXiang <xiangwencheng@dayudpu.com>
>
> The VHOST_USER_SET_LOG_BASE requests should be categorized into
> non-vring specific messages, and should be sent only once.
> If send more than once, dpdk will munmap old log_addr which may has
> been used and cause segmentation fault.

This looks fine to me but looking at the vhost-user.rst we don't seem to
make any explicit statements about how many times given messages should
be sent.

>
> Signed-off-by: BillXiang <xiangwencheng@dayudpu.com>
> ---
>  hw/virtio/vhost-user.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index cdf9af4a4b..41e34edd49 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -371,6 +371,7 @@ static bool vhost_user_per_device_request(VhostUserRequest request)
>      case VHOST_USER_RESET_DEVICE:
>      case VHOST_USER_ADD_MEM_REG:
>      case VHOST_USER_REM_MEM_REG:
> +    case VHOST_USER_SET_LOG_BASE:
>          return true;
>      default:
>          return false;
BillXiang July 1, 2024, 9:36 a.m. UTC | #3 
> From: "Alex Bennée"<alex.bennee@linaro.org>
> Date:  Mon, Jul 1, 2024, 16:49
> Subject:  Re: [PATCH] vhost-user: Skip unnecessary duplicated VHOST_USER_SET_LOG_BASE requests
> To: "项文成"<xiangwencheng@dayudpu.com>
> Cc: <qemu-devel@nongnu.org>, <mst@redhat.com>
> 项文成 <xiangwencheng@dayudpu.com> writes:
> 
> > From: BillXiang <xiangwencheng@dayudpu.com>
> >
> > The VHOST_USER_SET_LOG_BASE requests should be categorized into
> > non-vring specific messages, and should be sent only once.
> > If send more than once, dpdk will munmap old log_addr which may has
> > been used and cause segmentation fault.
> 
> This looks fine to me but looking at the vhost-user.rst we don't seem to
> make any explicit statements about how many times given messages should
> be sent.
> 
There is indeed no explicit statements about how many times given messages
 should be sent in vhost-user.rst but already have some discussions such as 
https://lore.kernel.org/qemu-devel/20230127083027-mutt-send-email-mst@kernel.org/.
> >
> > Signed-off-by: BillXiang <xiangwencheng@dayudpu.com>
> > ---
> >  hw/virtio/vhost-user.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index cdf9af4a4b..41e34edd49 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -371,6 +371,7 @@ static bool vhost_user_per_device_request(VhostUserRequest request)
> >      case VHOST_USER_RESET_DEVICE:
> >      case VHOST_USER_ADD_MEM_REG:
> >      case VHOST_USER_REM_MEM_REG:
> > +    case VHOST_USER_SET_LOG_BASE:
> >          return true;
> >      default:
> >          return false;
> 
> -- 
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
Alex Bennée July 1, 2024, 3:14 p.m. UTC | #4 
"BillXiang" <xiangwencheng@dayudpu.com> writes:

>> From: "Alex Bennée"<alex.bennee@linaro.org>
>> Date:  Mon, Jul 1, 2024, 16:49
>> Subject:  Re: [PATCH] vhost-user: Skip unnecessary duplicated VHOST_USER_SET_LOG_BASE requests
>> To: "项文成"<xiangwencheng@dayudpu.com>
>> Cc: <qemu-devel@nongnu.org>, <mst@redhat.com>
>> 项文成 <xiangwencheng@dayudpu.com> writes:
>> 
>> > From: BillXiang <xiangwencheng@dayudpu.com>
>> >
>> > The VHOST_USER_SET_LOG_BASE requests should be categorized into
>> > non-vring specific messages, and should be sent only once.
>> > If send more than once, dpdk will munmap old log_addr which may has
>> > been used and cause segmentation fault.
>> 
>> This looks fine to me but looking at the vhost-user.rst we don't seem to
>> make any explicit statements about how many times given messages should
>> be sent.
>> 
> There is indeed no explicit statements about how many times given messages
>  should be sent in vhost-user.rst but already have some discussions such as 
> https://lore.kernel.org/qemu-devel/20230127083027-mutt-send-email-mst@kernel.org/.

Right, but I think we should then update the specification if this is
the way we want things to work. Otherwise we are putting a backend
specific hack that another backend might be able to tolerate.

>> >
>> > Signed-off-by: BillXiang <xiangwencheng@dayudpu.com>
>> > ---
>> >  hw/virtio/vhost-user.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> > index cdf9af4a4b..41e34edd49 100644
>> > --- a/hw/virtio/vhost-user.c
>> > +++ b/hw/virtio/vhost-user.c
>> > @@ -371,6 +371,7 @@ static bool vhost_user_per_device_request(VhostUserRequest request)
>> >      case VHOST_USER_RESET_DEVICE:
>> >      case VHOST_USER_ADD_MEM_REG:
>> >      case VHOST_USER_REM_MEM_REG:
>> > +    case VHOST_USER_SET_LOG_BASE:
>> >          return true;
>> >      default:
>> >          return false;
>> 
>> -- 
>> Alex Bennée
>> Virtualisation Tech Lead @ Linaro
Michael S. Tsirkin July 1, 2024, 3:16 p.m. UTC | #5 
On Mon, Jul 01, 2024 at 04:14:35PM +0100, Alex Bennée wrote:
> "BillXiang" <xiangwencheng@dayudpu.com> writes:
> 
> >> From: "Alex Bennée"<alex.bennee@linaro.org>
> >> Date:  Mon, Jul 1, 2024, 16:49
> >> Subject:  Re: [PATCH] vhost-user: Skip unnecessary duplicated VHOST_USER_SET_LOG_BASE requests
> >> To: "项文成"<xiangwencheng@dayudpu.com>
> >> Cc: <qemu-devel@nongnu.org>, <mst@redhat.com>
> >> 项文成 <xiangwencheng@dayudpu.com> writes:
> >> 
> >> > From: BillXiang <xiangwencheng@dayudpu.com>
> >> >
> >> > The VHOST_USER_SET_LOG_BASE requests should be categorized into
> >> > non-vring specific messages, and should be sent only once.
> >> > If send more than once, dpdk will munmap old log_addr which may has
> >> > been used and cause segmentation fault.
> >> 
> >> This looks fine to me but looking at the vhost-user.rst we don't seem to
> >> make any explicit statements about how many times given messages should
> >> be sent.
> >> 
> > There is indeed no explicit statements about how many times given messages
> >  should be sent in vhost-user.rst but already have some discussions such as 
> > https://lore.kernel.org/qemu-devel/20230127083027-mutt-send-email-mst@kernel.org/.
> 
> Right, but I think we should then update the specification if this is
> the way we want things to work. Otherwise we are putting a backend
> specific hack that another backend might be able to tolerate.

I think it's a dpdk bug, we *allow* resending same message many times.
However, less messages is better, I don't see a reason to
repeat the same message many times.

> >> >
> >> > Signed-off-by: BillXiang <xiangwencheng@dayudpu.com>
> >> > ---
> >> >  hw/virtio/vhost-user.c | 1 +
> >> >  1 file changed, 1 insertion(+)
> >> >
> >> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >> > index cdf9af4a4b..41e34edd49 100644
> >> > --- a/hw/virtio/vhost-user.c
> >> > +++ b/hw/virtio/vhost-user.c
> >> > @@ -371,6 +371,7 @@ static bool vhost_user_per_device_request(VhostUserRequest request)
> >> >      case VHOST_USER_RESET_DEVICE:
> >> >      case VHOST_USER_ADD_MEM_REG:
> >> >      case VHOST_USER_REM_MEM_REG:
> >> > +    case VHOST_USER_SET_LOG_BASE:
> >> >          return true;
> >> >      default:
> >> >          return false;
> >> 
> >> -- 
> >> Alex Bennée
> >> Virtualisation Tech Lead @ Linaro
> 
> -- 
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
BillXiang July 2, 2024, 3:22 a.m. UTC | #6 
> From: "Michael S. Tsirkin"<mst@redhat.com>
> Date:  Mon, Jul 1, 2024, 23:17
> Subject:  Re: [PATCH] vhost-user: Skip unnecessary duplicated VHOST_USER_SET_LOG_BASE requests
> To: "Alex Bennée"<alex.bennee@linaro.org>
> Cc: "BillXiang"<xiangwencheng@dayudpu.com>, <qemu-devel@nongnu.org>
> On Mon, Jul 01, 2024 at 04:14:35PM +0100, Alex Bennée wrote:
> > "BillXiang" <xiangwencheng@dayudpu.com> writes:
> > 
> > >> From: "Alex Bennée"<alex.bennee@linaro.org>
> > >> Date:  Mon, Jul 1, 2024, 16:49
> > >> Subject:  Re: [PATCH] vhost-user: Skip unnecessary duplicated VHOST_USER_SET_LOG_BASE requests
> > >> To: "项文成"<xiangwencheng@dayudpu.com>
> > >> Cc: <qemu-devel@nongnu.org>, <mst@redhat.com>
> > >> 项文成 <xiangwencheng@dayudpu.com> writes:
> > >> 
> > >> > From: BillXiang <xiangwencheng@dayudpu.com>
> > >> >
> > >> > The VHOST_USER_SET_LOG_BASE requests should be categorized into
> > >> > non-vring specific messages, and should be sent only once.
> > >> > If send more than once, dpdk will munmap old log_addr which may has
> > >> > been used and cause segmentation fault.
> > >> 
> > >> This looks fine to me but looking at the vhost-user.rst we don't seem to
> > >> make any explicit statements about how many times given messages should
> > >> be sent.
> > >> 
> > > There is indeed no explicit statements about how many times given messages
> > >  should be sent in vhost-user.rst but already have some discussions such as 
> > > https://lore.kernel.org/qemu-devel/20230127083027-mutt-send-email-mst@kernel.org/.
> > 
> > Right, but I think we should then update the specification if this is
> > the way we want things to work. Otherwise we are putting a backend
> > specific hack that another backend might be able to tolerate.

I agree with you that we should then update the specification and maybe for vhost_user_per_device_request.

> 
> I think it's a dpdk bug, we *allow* resending same message many times.
> However, less messages is better, I don't see a reason to
> repeat the same message many times.
> 

Of course these repeated VHOST_USER_SET_LOG_BASE can be handled by dpdk 
and it's what I did now. When live migration started, I have some extra-code to wait 
for the final log_base but I think it's ugly. 
AFAIK it's better to resolve this by vhost_user_per_device_request and it's what "per_device"
really mean.

> > >> >
> > >> > Signed-off-by: BillXiang <xiangwencheng@dayudpu.com>
> > >> > ---
> > >> >  hw/virtio/vhost-user.c | 1 +
> > >> >  1 file changed, 1 insertion(+)
> > >> >
> > >> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > >> > index cdf9af4a4b..41e34edd49 100644
> > >> > --- a/hw/virtio/vhost-user.c
> > >> > +++ b/hw/virtio/vhost-user.c
> > >> > @@ -371,6 +371,7 @@ static bool vhost_user_per_device_request(VhostUserRequest request)
> > >> >      case VHOST_USER_RESET_DEVICE:
> > >> >      case VHOST_USER_ADD_MEM_REG:
> > >> >      case VHOST_USER_REM_MEM_REG:
> > >> > +    case VHOST_USER_SET_LOG_BASE:
> > >> >          return true;
> > >> >      default:
> > >> >          return false;
> > >> 
> > >> -- 
> > >> Alex Bennée
> > >> Virtualisation Tech Lead @ Linaro
> > 
> > -- 
> > Alex Bennée
> > Virtualisation Tech Lead @ Linaro
diff mbox series

Patch

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index cdf9af4a4b..41e34edd49 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -371,6 +371,7 @@  static bool vhost_user_per_device_request(VhostUserRequest request)
     case VHOST_USER_RESET_DEVICE:
     case VHOST_USER_ADD_MEM_REG:
     case VHOST_USER_REM_MEM_REG:
+    case VHOST_USER_SET_LOG_BASE:
         return true;
     default:
         return false;