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

[Michael S. Tsirkin]

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