Re: [PATCH 1/1] vhost-user: Skip unnecessary duplicated VHOST_USER_ADD/REM_MEM_REG requests

[Raphael Norwitz]

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
> 
>