Linux Virtualization - Nov 2019 - [PATCH net-next 12/14] vsock/vmci: register vmci_transport only when VMCI guest/host are active

> From: Stefano Garzarella [mailto:sgarzare at redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> 
> To allow other transports to be loaded with vmci_transport,
> we register the vmci_transport as G2H or H2G only when a VMCI guest
> or host is active.
> 
> To do that, this patch adds a callback registered in the vmci driver
> that will be called when a new host or guest become active.
> This callback will register the vmci_transport in the VSOCK core.
> If the transport is already registered, we ignore the error coming
> from vsock_core_register().
So today this is mainly an issue for the VMCI vsock transport, because
VMCI autoloads with vsock (and with this solution it can continue to
do that, so none of our old products break due to changed behavior,
which is great). Shouldn't vhost behave similar, so that any module
that registers a h2g transport only does so if it is in active use?

> --- a/drivers/misc/vmw_vmci/vmci_host.c
> +++ b/drivers/misc/vmw_vmci/vmci_host.c
> @@ -108,6 +108,11 @@ bool vmci_host_code_active(void)
>  	     atomic_read(&vmci_host_active_users) > 0);
>  }
> 
> +int vmci_host_users(void)
> +{
> +	return atomic_read(&vmci_host_active_users);
> +}
> +
>  /*
>   * Called on open of /dev/vmci.
>   */
> @@ -338,6 +343,8 @@ static int vmci_host_do_init_context(struct
> vmci_host_dev *vmci_host_dev,
>  	vmci_host_dev->ct_type = VMCIOBJ_CONTEXT;
>  	atomic_inc(&vmci_host_active_users);
> 
> +	vmci_call_vsock_callback(true);
> +
Since we don't unregister the transport if user count drops back to 0, we
could
just call this the first time, a VM is powered on after the module is loaded.
>  	retval = 0;
> 
>  out:

On Mon, Nov 11, 2019 at 04:27:28PM +0000, Jorgen Hansen
wrote:
> > From: Stefano Garzarella [mailto:sgarzare at redhat.com]
> > Sent: Wednesday, October 23, 2019 11:56 AM
> > 
> > To allow other transports to be loaded with vmci_transport,
> > we register the vmci_transport as G2H or H2G only when a VMCI guest
> > or host is active.
> > 
> > To do that, this patch adds a callback registered in the vmci driver
> > that will be called when a new host or guest become active.
> > This callback will register the vmci_transport in the VSOCK core.
> > If the transport is already registered, we ignore the error coming
> > from vsock_core_register().
> 
> So today this is mainly an issue for the VMCI vsock transport, because
> VMCI autoloads with vsock (and with this solution it can continue to
> do that, so none of our old products break due to changed behavior,
> which is great).
I tried to not break anything :-)
>                  Shouldn't vhost behave similar, so that any module
> that registers a h2g transport only does so if it is in active use?
> 
The vhost-vsock module will load when the first hypervisor open
/dev/vhost-vsock, so in theory, when there's at least one active user.
> 
> > --- a/drivers/misc/vmw_vmci/vmci_host.c
> > +++ b/drivers/misc/vmw_vmci/vmci_host.c
> > @@ -108,6 +108,11 @@ bool vmci_host_code_active(void)
> >  	     atomic_read(&vmci_host_active_users) > 0);
> >  }
> > 
> > +int vmci_host_users(void)
> > +{
> > +	return atomic_read(&vmci_host_active_users);
> > +}
> > +
> >  /*
> >   * Called on open of /dev/vmci.
> >   */
> > @@ -338,6 +343,8 @@ static int vmci_host_do_init_context(struct
> > vmci_host_dev *vmci_host_dev,
> >  	vmci_host_dev->ct_type = VMCIOBJ_CONTEXT;
> >  	atomic_inc(&vmci_host_active_users);
> > 
> > +	vmci_call_vsock_callback(true);
> > +
> 
> Since we don't unregister the transport if user count drops back to 0,
we could
> just call this the first time, a VM is powered on after the module is
loaded.
Yes, make sense. can I use the 'vmci_host_active_users' or is better to
add a new 'vmci_host_vsock_loaded'?

My doubt is that vmci_host_active_users can return to 0, so when it returns
to 1, we call vmci_call_vsock_callback() again.

Thanks,
Stefano

> From: Stefano Garzarella [mailto:sgarzare at redhat.com]
> Sent: Monday, November 11, 2019 6:31 PM
> On Mon, Nov 11, 2019 at 04:27:28PM +0000, Jorgen Hansen wrote:
> > > From: Stefano Garzarella [mailto:sgarzare at redhat.com]
> > > Sent: Wednesday, October 23, 2019 11:56 AM
> > >
> > > To allow other transports to be loaded with vmci_transport,
> > > we register the vmci_transport as G2H or H2G only when a VMCI
guest
> > > or host is active.
> > >
> > > To do that, this patch adds a callback registered in the vmci
driver
> > > that will be called when a new host or guest become active.
> > > This callback will register the vmci_transport in the VSOCK core.
> > > If the transport is already registered, we ignore the error
coming
> > > from vsock_core_register().
> >
> > So today this is mainly an issue for the VMCI vsock transport, because
> > VMCI autoloads with vsock (and with this solution it can continue to
> > do that, so none of our old products break due to changed behavior,
> > which is great).
> 
> I tried to not break anything :-)
> 
> >                  Shouldn't vhost behave similar, so that any
module
> > that registers a h2g transport only does so if it is in active use?
> >
> 
> The vhost-vsock module will load when the first hypervisor open
> /dev/vhost-vsock, so in theory, when there's at least one active user.
Ok, sounds good then. 
> 
> >
> > > --- a/drivers/misc/vmw_vmci/vmci_host.c
> > > +++ b/drivers/misc/vmw_vmci/vmci_host.c
> > > @@ -108,6 +108,11 @@ bool vmci_host_code_active(void)
> > >  	     atomic_read(&vmci_host_active_users) > 0);
> > >  }
> > >
> > > +int vmci_host_users(void)
> > > +{
> > > +	return atomic_read(&vmci_host_active_users);
> > > +}
> > > +
> > >  /*
> > >   * Called on open of /dev/vmci.
> > >   */
> > > @@ -338,6 +343,8 @@ static int vmci_host_do_init_context(struct
> > > vmci_host_dev *vmci_host_dev,
> > >  	vmci_host_dev->ct_type = VMCIOBJ_CONTEXT;
> > >  	atomic_inc(&vmci_host_active_users);
> > >
> > > +	vmci_call_vsock_callback(true);
> > > +
> >
> > Since we don't unregister the transport if user count drops back
to 0, we
> could
> > just call this the first time, a VM is powered on after the module is
loaded.
> 
> Yes, make sense. can I use the 'vmci_host_active_users' or is
better to
> add a new 'vmci_host_vsock_loaded'?
> 
> My doubt is that vmci_host_active_users can return to 0, so when it returns
> to 1, we call vmci_call_vsock_callback() again.
vmci_host_active_users can drop to 0 and then increase again, so having a flag
indicating whether the callback has been invoked would ensure that it is only
called once.

Thanks,
Jorgen