* [PATCH 1/1] vhost-user: Skip unnecessary duplicated VHOST_USER_ADD/REM_MEM_REG requests
@ 2023-01-23 12:21 Minghao Yuan
0 siblings, 0 replies; 4+ messages in thread
From: Minghao Yuan @ 2023-01-23 12:21 UTC (permalink / raw)
To: raphael.norwitz, swapnil.ingle, peter.turschm; +Cc: mst, qemu-devel
The VHOST_USER_ADD/REM_MEM_REG requests should be categorized into
non-vring specific messages, and should be sent only once.
Signed-off-by: Minghao Yuan <yuanmh12@chinatelecom.cn>
---
hw/virtio/vhost-user.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index d9ce0501b2..3f2a8c3bdd 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -459,6 +459,8 @@ static bool vhost_user_one_time_request(VhostUserRequest request)
case VHOST_USER_SET_MEM_TABLE:
case VHOST_USER_GET_QUEUE_NUM:
case VHOST_USER_NET_SET_MTU:
+ case VHOST_USER_ADD_MEM_REG:
+ case VHOST_USER_REM_MEM_REG:
return true;
default:
return false;
--
2.27.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] vhost-user: Skip unnecessary duplicated VHOST_USER_ADD/REM_MEM_REG requests
2023-01-17 14:56 ` Raphael Norwitz
@ 2023-01-27 13:33 ` Michael S. Tsirkin
0 siblings, 0 replies; 4+ messages in thread
From: Michael S. Tsirkin @ 2023-01-27 13:33 UTC (permalink / raw)
To: Raphael Norwitz; +Cc: Minghao Yuan, Swapnil Ingle, Peter Turschmid, qemu-devel
On Tue, Jan 17, 2023 at 02:56:50PM +0000, Raphael Norwitz wrote:
> I’m confused by this “one time request” path.
>
> MST - why do we classify SET_MEM_TABLE as a one time request if we send it on every hot-add/hot-remove.
>
> In particular I’m tripping over the following in vhost_user_write:
>
> /*
> * For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE,
> * we just need send it once in the first time. For later such
> * request, we just ignore it.
> */
> if (vhost_user_one_time_request(msg->hdr.request) && dev->vq_index != 0) {
> msg->hdr.flags &= ~VHOST_USER_NEED_REPLY_MASK;
> return 0;
> }
It's about the weird way vhost net works - it's actually split
from the frontend. So it's modeled as multiple devices
and vq_index will let you distinguish.
This has advantages and disadvantages.
> With the hot-add case in mind, this comment sounds off. IIUC hot-add works for vhost-user-blk and vhost-user-scsi because dev->vq_index is set to 0 and never changed.
>
> Ref: https://git.qemu.org/?p=qemu.git;a=blob;f=hw/scsi/vhost-user-scsi.c;h=b7a71a802cdbf7430704f83fc8c6c04c135644b7;hb=HEAD#l121
>
> Breakpoint 1, vhost_user_set_mem_table (dev=0x.., mem=0x..) at ../hw/virtio/vhost-user.c
> (gdb) where
> #0 vhost_user_set_mem_table (dev=0x..., mem=0x...) at ...hw/virtio/vhost-user.c
> #1 0x… in vhost_commit (listener=0x..) at .../hw/virtio/vhost.c
> #2 0x… in memory_region_transaction_commit () at ...memory.c
> ...
> (gdb) p dev->nvqs
> $4 = 10
> (gdb) p dev->vq_index
> $3 = 0
> (gdb)
>
> Looks like this functionality came in here:
>
> commit b931bfbf042983f311b3b09894d8030b2755a638
> Author: Changchun Ouyang <changchun.ouyang@intel.com>
> Date: Wed Sep 23 12:20:00 2015 +0800
>
> vhost-user: add multiple queue support
>
> This patch is initially based a patch from Nikolay Nikolaev.
>
> This patch adds vhost-user multiple queue support, by creating a nc
> and vhost_net pair for each queue.
>
> ...
>
> In older version, it was reported that some messages are sent more times
> than necessary. Here we came an agreement with Michael that we could
> categorize vhost user messages to 2 types: non-vring specific messages,
> which should be sent only once, and vring specific messages, which should
> be sent per queue.
>
> Here I introduced a helper function vhost_user_one_time_request(), which
> lists following messages as non-vring specific messages:
>
> VHOST_USER_SET_OWNER
> VHOST_USER_RESET_DEVICE
> VHOST_USER_SET_MEM_TABLE
> VHOST_USER_GET_QUEUE_NUM
>
> For above messages, we simply ignore them when they are not sent the first
> time.
>
> With hot-add in mind, should we revisit the non-vring specific messages and possibly clean the code up?
Sure.
>
> > On Jan 1, 2023, at 11:45 PM, Minghao Yuan <yuanmh12@chinatelecom.cn> wrote:
> >
> > The VHOST_USER_ADD/REM_MEM_REG requests should be categorized into
> > non-vring specific messages, and should be sent only once.
> >
> > Signed-off-by: Minghao Yuan <yuanmh12@chinatelecom.cn>
> > ---
> > configure | 2 +-
> > hw/virtio/vhost-user.c | 2 ++
> > 2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/configure b/configure
> > index 9e407ce2e3..8b4deca342 100755
>
> This configure change looks irrelevant. Did you mean to send it?
>
> > --- a/configure
> > +++ b/configure
> > @@ -1147,7 +1147,7 @@ cat > $TMPC << EOF
> > # endif
> > # endif
> > #elif defined(__GNUC__) && defined(__GNUC_MINOR__)
> > -# if __GNUC__ < 7 || (__GNUC__ == 7 && __GNUC_MINOR__ < 4)
> > +# if __GNUC__ < 7 || (__GNUC__ == 7 && __GNUC_MINOR__ < 3)
> > # error You need at least GCC v7.4.0 to compile QEMU
> > # endif
> > #else
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index d9ce0501b2..3f2a8c3bdd 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -459,6 +459,8 @@ static bool vhost_user_one_time_request(VhostUserRequest request)
> > case VHOST_USER_SET_MEM_TABLE:
> > case VHOST_USER_GET_QUEUE_NUM:
> > case VHOST_USER_NET_SET_MTU:
> > + case VHOST_USER_ADD_MEM_REG:
> > + case VHOST_USER_REM_MEM_REG:
> > return true;
> > default:
> > return false;
> > --
> > 2.27.0
> >
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] vhost-user: Skip unnecessary duplicated VHOST_USER_ADD/REM_MEM_REG requests
2023-01-01 21:45 Minghao Yuan
@ 2023-01-17 14:56 ` Raphael Norwitz
2023-01-27 13:33 ` Michael S. Tsirkin
0 siblings, 1 reply; 4+ messages in thread
From: Raphael Norwitz @ 2023-01-17 14:56 UTC (permalink / raw)
To: Minghao Yuan; +Cc: Swapnil Ingle, Peter Turschmid, mst, qemu-devel
I’m confused by this “one time request” path.
MST - why do we classify SET_MEM_TABLE as a one time request if we send it on every hot-add/hot-remove.
In particular I’m tripping over the following in vhost_user_write:
/*
* For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE,
* we just need send it once in the first time. For later such
* request, we just ignore it.
*/
if (vhost_user_one_time_request(msg->hdr.request) && dev->vq_index != 0) {
msg->hdr.flags &= ~VHOST_USER_NEED_REPLY_MASK;
return 0;
}
With the hot-add case in mind, this comment sounds off. IIUC hot-add works for vhost-user-blk and vhost-user-scsi because dev->vq_index is set to 0 and never changed.
Ref: https://git.qemu.org/?p=qemu.git;a=blob;f=hw/scsi/vhost-user-scsi.c;h=b7a71a802cdbf7430704f83fc8c6c04c135644b7;hb=HEAD#l121
Breakpoint 1, vhost_user_set_mem_table (dev=0x.., mem=0x..) at ../hw/virtio/vhost-user.c
(gdb) where
#0 vhost_user_set_mem_table (dev=0x..., mem=0x...) at ...hw/virtio/vhost-user.c
#1 0x… in vhost_commit (listener=0x..) at .../hw/virtio/vhost.c
#2 0x… in memory_region_transaction_commit () at ...memory.c
...
(gdb) p dev->nvqs
$4 = 10
(gdb) p dev->vq_index
$3 = 0
(gdb)
Looks like this functionality came in here:
commit b931bfbf042983f311b3b09894d8030b2755a638
Author: Changchun Ouyang <changchun.ouyang@intel.com>
Date: Wed Sep 23 12:20:00 2015 +0800
vhost-user: add multiple queue support
This patch is initially based a patch from Nikolay Nikolaev.
This patch adds vhost-user multiple queue support, by creating a nc
and vhost_net pair for each queue.
...
In older version, it was reported that some messages are sent more times
than necessary. Here we came an agreement with Michael that we could
categorize vhost user messages to 2 types: non-vring specific messages,
which should be sent only once, and vring specific messages, which should
be sent per queue.
Here I introduced a helper function vhost_user_one_time_request(), which
lists following messages as non-vring specific messages:
VHOST_USER_SET_OWNER
VHOST_USER_RESET_DEVICE
VHOST_USER_SET_MEM_TABLE
VHOST_USER_GET_QUEUE_NUM
For above messages, we simply ignore them when they are not sent the first
time.
With hot-add in mind, should we revisit the non-vring specific messages and possibly clean the code up?
> On Jan 1, 2023, at 11:45 PM, Minghao Yuan <yuanmh12@chinatelecom.cn> wrote:
>
> The VHOST_USER_ADD/REM_MEM_REG requests should be categorized into
> non-vring specific messages, and should be sent only once.
>
> Signed-off-by: Minghao Yuan <yuanmh12@chinatelecom.cn>
> ---
> configure | 2 +-
> hw/virtio/vhost-user.c | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/configure b/configure
> index 9e407ce2e3..8b4deca342 100755
This configure change looks irrelevant. Did you mean to send it?
> --- a/configure
> +++ b/configure
> @@ -1147,7 +1147,7 @@ cat > $TMPC << EOF
> # endif
> # endif
> #elif defined(__GNUC__) && defined(__GNUC_MINOR__)
> -# if __GNUC__ < 7 || (__GNUC__ == 7 && __GNUC_MINOR__ < 4)
> +# if __GNUC__ < 7 || (__GNUC__ == 7 && __GNUC_MINOR__ < 3)
> # error You need at least GCC v7.4.0 to compile QEMU
> # endif
> #else
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index d9ce0501b2..3f2a8c3bdd 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -459,6 +459,8 @@ static bool vhost_user_one_time_request(VhostUserRequest request)
> case VHOST_USER_SET_MEM_TABLE:
> case VHOST_USER_GET_QUEUE_NUM:
> case VHOST_USER_NET_SET_MTU:
> + case VHOST_USER_ADD_MEM_REG:
> + case VHOST_USER_REM_MEM_REG:
> return true;
> default:
> return false;
> --
> 2.27.0
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/1] vhost-user: Skip unnecessary duplicated VHOST_USER_ADD/REM_MEM_REG requests
@ 2023-01-01 21:45 Minghao Yuan
2023-01-17 14:56 ` Raphael Norwitz
0 siblings, 1 reply; 4+ messages in thread
From: Minghao Yuan @ 2023-01-01 21:45 UTC (permalink / raw)
To: raphael.norwitz, swapnil.ingle, peter.turschm; +Cc: mst, qemu-devel
The VHOST_USER_ADD/REM_MEM_REG requests should be categorized into
non-vring specific messages, and should be sent only once.
Signed-off-by: Minghao Yuan <yuanmh12@chinatelecom.cn>
---
configure | 2 +-
hw/virtio/vhost-user.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/configure b/configure
index 9e407ce2e3..8b4deca342 100755
--- a/configure
+++ b/configure
@@ -1147,7 +1147,7 @@ cat > $TMPC << EOF
# endif
# endif
#elif defined(__GNUC__) && defined(__GNUC_MINOR__)
-# if __GNUC__ < 7 || (__GNUC__ == 7 && __GNUC_MINOR__ < 4)
+# if __GNUC__ < 7 || (__GNUC__ == 7 && __GNUC_MINOR__ < 3)
# error You need at least GCC v7.4.0 to compile QEMU
# endif
#else
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index d9ce0501b2..3f2a8c3bdd 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -459,6 +459,8 @@ static bool vhost_user_one_time_request(VhostUserRequest request)
case VHOST_USER_SET_MEM_TABLE:
case VHOST_USER_GET_QUEUE_NUM:
case VHOST_USER_NET_SET_MTU:
+ case VHOST_USER_ADD_MEM_REG:
+ case VHOST_USER_REM_MEM_REG:
return true;
default:
return false;
--
2.27.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-27 13:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-23 12:21 [PATCH 1/1] vhost-user: Skip unnecessary duplicated VHOST_USER_ADD/REM_MEM_REG requests Minghao Yuan
-- strict thread matches above, loose matches on Subject: below --
2023-01-01 21:45 Minghao Yuan
2023-01-17 14:56 ` Raphael Norwitz
2023-01-27 13:33 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).