Re: [PATCH net-next 12/14] vsock/vmci: register vmci_transport only when VMCI guest/host are active
On Mon, Nov 11, 2019 at 04:27:28PM +, Jorgen Hansen wrote:
> > From: Stefano Garzarella [mailto:sgarz...@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(_host_active_users) > 0);
> > }
> >
> > +int vmci_host_users(void)
> > +{
> > + return atomic_read(_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(_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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 11/14] vsock: add multi-transports support
On Mon, Nov 11, 2019 at 01:53:39PM +, Jorgen Hansen wrote:
> > From: Stefano Garzarella [mailto:sgarz...@redhat.com]
> > Sent: Wednesday, October 23, 2019 11:56 AM
>
> Thanks a lot for working on this!
>
Thanks to you for the reviews!
> > With the multi-transports support, we can use vsock with nested VMs (using
> > also different hypervisors) loading both guest->host and
> > host->guest transports at the same time.
> >
> > Major changes:
> > - vsock core module can be loaded regardless of the transports
> > - vsock_core_init() and vsock_core_exit() are renamed to
> > vsock_core_register() and vsock_core_unregister()
> > - vsock_core_register() has a feature parameter (H2G, G2H, DGRAM)
> > to identify which directions the transport can handle and if it's
> > support DGRAM (only vmci)
> > - each stream socket is assigned to a transport when the remote CID
> > is set (during the connect() or when we receive a connection request
> > on a listener socket).
>
> How about allowing the transport to be set during bind as well? That
> would allow an application to ensure that it is using a specific transport,
> i.e., if it binds to the host CID, it will use H2G, and if it binds to
> something
> else it will use G2H? You can still use VMADDR_CID_ANY if you want to
> initially listen to both transports.
Do you mean for socket that will call the connect()?
For listener socket the "[PATCH net-next 14/14] vsock: fix bind() behaviour
taking care of CID" provides this behaviour.
Since the listener sockets don't use any transport specific callback
(they don't send any data to the remote peer), but they are used as placeholder,
we don't need to assign them to a transport.
>
>
> > The remote CID is used to decide which transport to use:
> > - remote CID > VMADDR_CID_HOST will use host->guest transport
> > - remote CID <= VMADDR_CID_HOST will use guest->host transport
> > - listener sockets are not bound to any transports since no transport
> > operations are done on it. In this way we can create a listener
> > socket, also if the transports are not loaded or with VMADDR_CID_ANY
> > to listen on all transports.
> > - DGRAM sockets are handled as before, since only the vmci_transport
> > provides this feature.
> >
> > Signed-off-by: Stefano Garzarella
> > ---
> > RFC -> v1:
> > - documented VSOCK_TRANSPORT_F_* flags
> > - fixed vsock_assign_transport() when the socket is already assigned
> > (e.g connection failed)
> > - moved features outside of struct vsock_transport, and used as
> > parameter of vsock_core_register()
> > ---
> > drivers/vhost/vsock.c | 5 +-
> > include/net/af_vsock.h | 17 +-
> > net/vmw_vsock/af_vsock.c| 237 ++--
> > net/vmw_vsock/hyperv_transport.c| 26 ++-
> > net/vmw_vsock/virtio_transport.c| 7 +-
> > net/vmw_vsock/virtio_transport_common.c | 28 ++-
> > net/vmw_vsock/vmci_transport.c | 31 +++-
> > 7 files changed, 270 insertions(+), 81 deletions(-)
> >
>
>
> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index
> > d89381166028..85d9a147 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -130,7 +130,12 @@ static struct proto vsock_proto = { #define
> > VSOCK_DEFAULT_BUFFER_MAX_SIZE (1024 * 256) #define
> > VSOCK_DEFAULT_BUFFER_MIN_SIZE 128
> >
> > -static const struct vsock_transport *transport_single;
> > +/* Transport used for host->guest communication */ static const struct
> > +vsock_transport *transport_h2g;
> > +/* Transport used for guest->host communication */ static const struct
> > +vsock_transport *transport_g2h;
> > +/* Transport used for DGRAM communication */ static const struct
> > +vsock_transport *transport_dgram;
> > static DEFINE_MUTEX(vsock_register_mutex);
> >
> > / UTILS /
> > @@ -182,7 +187,7 @@ static int vsock_auto_bind(struct vsock_sock *vsk)
> > return __vsock_bind(sk, _addr);
> > }
> >
> > -static int __init vsock_init_tables(void)
> > +static void vsock_init_tables(void)
> > {
> > int i;
> >
> > @@ -191,7 +196,6 @@ static int __init vsock_init_tables(void)
> >
> > for (i = 0; i < ARRAY_SIZE(vsock_connected_table); i++)
> > INIT_LIST_HEAD(_connected_table[i]);
> > - return 0;
> > }
> >
> > static void __vsock_insert_bound(struct list_head *list, @@ -376,6 +380,62
> > @@ void vsock_enqueue_accept(struct sock *listener, struct sock
> > *connected) } EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
> >
> > +/* Assign a transport to a socket and call the .init transport callback.
> > + *
> > + * Note: for stream socket this must be called when vsk->remote_addr is
> > +set
> > + * (e.g. during the connect() or when a connection request on a
> > +listener
> > + * socket is received).
> > + * The vsk->remote_addr is used to decide which transport to use:
> > + * - remote CID > VMADDR_CID_HOST will use host->guest transport
> >
RE: [PATCH net-next 14/14] vsock: fix bind() behaviour taking care of CID
> From: Stefano Garzarella [mailto:sgarz...@redhat.com] > Sent: Wednesday, October 23, 2019 11:56 AM > When we are looking for a socket bound to a specific address, > we also have to take into account the CID. > > This patch is useful with multi-transports support because it > allows the binding of the same port with different CID, and > it prevents a connection to a wrong socket bound to the same > port, but with different CID. > > Reviewed-by: Stefan Hajnoczi > Signed-off-by: Stefano Garzarella > --- > net/vmw_vsock/af_vsock.c | 10 -- > 1 file changed, 8 insertions(+), 2 deletions(-) Reviewed-by: Jorgen Hansen ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH net-next 13/14] vsock: prevent transport modules unloading
> From: Stefano Garzarella [mailto:sgarz...@redhat.com] > Sent: Wednesday, October 23, 2019 11:56 AM > This patch adds 'module' member in the 'struct vsock_transport' > in order to get/put the transport module. This prevents the > module unloading while sockets are assigned to it. > > We increase the module refcnt when a socket is assigned to a > transport, and we decrease the module refcnt when the socket > is destructed. > > Reviewed-by: Stefan Hajnoczi > Signed-off-by: Stefano Garzarella > --- > RFC -> v1: > - fixed typo 's/tranport/transport/' in a comment (Stefan) > --- > drivers/vhost/vsock.c| 2 ++ > include/net/af_vsock.h | 2 ++ > net/vmw_vsock/af_vsock.c | 20 > net/vmw_vsock/hyperv_transport.c | 2 ++ > net/vmw_vsock/virtio_transport.c | 2 ++ > net/vmw_vsock/vmci_transport.c | 1 + > 6 files changed, 25 insertions(+), 4 deletions(-) Reviewed-by: Jorgen Hansen ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH net-next 12/14] vsock/vmci: register vmci_transport only when VMCI guest/host are active
> From: Stefano Garzarella [mailto:sgarz...@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(_host_active_users) > 0);
> }
>
> +int vmci_host_users(void)
> +{
> + return atomic_read(_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(_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:
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH net-next 11/14] vsock: add multi-transports support
> From: Stefano Garzarella [mailto:sgarz...@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
Thanks a lot for working on this!
> With the multi-transports support, we can use vsock with nested VMs (using
> also different hypervisors) loading both guest->host and
> host->guest transports at the same time.
>
> Major changes:
> - vsock core module can be loaded regardless of the transports
> - vsock_core_init() and vsock_core_exit() are renamed to
> vsock_core_register() and vsock_core_unregister()
> - vsock_core_register() has a feature parameter (H2G, G2H, DGRAM)
> to identify which directions the transport can handle and if it's
> support DGRAM (only vmci)
> - each stream socket is assigned to a transport when the remote CID
> is set (during the connect() or when we receive a connection request
> on a listener socket).
How about allowing the transport to be set during bind as well? That
would allow an application to ensure that it is using a specific transport,
i.e., if it binds to the host CID, it will use H2G, and if it binds to something
else it will use G2H? You can still use VMADDR_CID_ANY if you want to
initially listen to both transports.
> The remote CID is used to decide which transport to use:
> - remote CID > VMADDR_CID_HOST will use host->guest transport
> - remote CID <= VMADDR_CID_HOST will use guest->host transport
> - listener sockets are not bound to any transports since no transport
> operations are done on it. In this way we can create a listener
> socket, also if the transports are not loaded or with VMADDR_CID_ANY
> to listen on all transports.
> - DGRAM sockets are handled as before, since only the vmci_transport
> provides this feature.
>
> Signed-off-by: Stefano Garzarella
> ---
> RFC -> v1:
> - documented VSOCK_TRANSPORT_F_* flags
> - fixed vsock_assign_transport() when the socket is already assigned
> (e.g connection failed)
> - moved features outside of struct vsock_transport, and used as
> parameter of vsock_core_register()
> ---
> drivers/vhost/vsock.c | 5 +-
> include/net/af_vsock.h | 17 +-
> net/vmw_vsock/af_vsock.c| 237 ++--
> net/vmw_vsock/hyperv_transport.c| 26 ++-
> net/vmw_vsock/virtio_transport.c| 7 +-
> net/vmw_vsock/virtio_transport_common.c | 28 ++-
> net/vmw_vsock/vmci_transport.c | 31 +++-
> 7 files changed, 270 insertions(+), 81 deletions(-)
>
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index
> d89381166028..85d9a147 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -130,7 +130,12 @@ static struct proto vsock_proto = { #define
> VSOCK_DEFAULT_BUFFER_MAX_SIZE (1024 * 256) #define
> VSOCK_DEFAULT_BUFFER_MIN_SIZE 128
>
> -static const struct vsock_transport *transport_single;
> +/* Transport used for host->guest communication */ static const struct
> +vsock_transport *transport_h2g;
> +/* Transport used for guest->host communication */ static const struct
> +vsock_transport *transport_g2h;
> +/* Transport used for DGRAM communication */ static const struct
> +vsock_transport *transport_dgram;
> static DEFINE_MUTEX(vsock_register_mutex);
>
> / UTILS /
> @@ -182,7 +187,7 @@ static int vsock_auto_bind(struct vsock_sock *vsk)
> return __vsock_bind(sk, _addr);
> }
>
> -static int __init vsock_init_tables(void)
> +static void vsock_init_tables(void)
> {
> int i;
>
> @@ -191,7 +196,6 @@ static int __init vsock_init_tables(void)
>
> for (i = 0; i < ARRAY_SIZE(vsock_connected_table); i++)
> INIT_LIST_HEAD(_connected_table[i]);
> - return 0;
> }
>
> static void __vsock_insert_bound(struct list_head *list, @@ -376,6 +380,62
> @@ void vsock_enqueue_accept(struct sock *listener, struct sock
> *connected) } EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
>
> +/* Assign a transport to a socket and call the .init transport callback.
> + *
> + * Note: for stream socket this must be called when vsk->remote_addr is
> +set
> + * (e.g. during the connect() or when a connection request on a
> +listener
> + * socket is received).
> + * The vsk->remote_addr is used to decide which transport to use:
> + * - remote CID > VMADDR_CID_HOST will use host->guest transport
> + * - remote CID <= VMADDR_CID_HOST will use guest->host transport */
> +int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock
> +*psk) {
> + const struct vsock_transport *new_transport;
> + struct sock *sk = sk_vsock(vsk);
> +
> + switch (sk->sk_type) {
> + case SOCK_DGRAM:
> + new_transport = transport_dgram;
> + break;
> + case SOCK_STREAM:
> + if (vsk->remote_addr.svm_cid > VMADDR_CID_HOST)
> + new_transport = transport_h2g;
> + else
> + new_transport = transport_g2h;
> + break;
You already mentioned that you are working on
[PATCH -next] virtiofs: Fix old-style declaration
There expect the 'static' keyword to come first in a
declaration, and we get warnings like this with "make W=1":
fs/fuse/virtio_fs.c:687:1: warning: 'static' is not at beginning of declaration
[-Wold-style-declaration]
fs/fuse/virtio_fs.c:692:1: warning: 'static' is not at beginning of declaration
[-Wold-style-declaration]
fs/fuse/virtio_fs.c:1029:1: warning: 'static' is not at beginning of
declaration [-Wold-style-declaration]
Signed-off-by: YueHaibing
---
fs/fuse/virtio_fs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index b77acea..2ac6818 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -684,12 +684,12 @@ static int virtio_fs_restore(struct virtio_device *vdev)
}
#endif /* CONFIG_PM_SLEEP */
-const static struct virtio_device_id id_table[] = {
+static const struct virtio_device_id id_table[] = {
{ VIRTIO_ID_FS, VIRTIO_DEV_ANY_ID },
{},
};
-const static unsigned int feature_table[] = {};
+static const unsigned int feature_table[] = {};
static struct virtio_driver virtio_fs_driver = {
.driver.name= KBUILD_MODNAME,
@@ -1026,7 +1026,7 @@ __releases(fiq->lock)
}
}
-const static struct fuse_iqueue_ops virtio_fs_fiq_ops = {
+static const struct fuse_iqueue_ops virtio_fs_fiq_ops = {
.wake_forget_and_unlock = virtio_fs_wake_forget_and_unlock,
.wake_interrupt_and_unlock = virtio_fs_wake_interrupt_and_unlock,
.wake_pending_and_unlock= virtio_fs_wake_pending_and_unlock,
--
2.7.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtiofs: Use static const, not const static
On Mon, Nov 11, 2019 at 05:26:41PM +0800, zhengbin wrote:
> Move the static keyword to the front of declarations.
Please mention why this change is necessary in the commit description.
>
> Reported-by: Hulk Robot
> Signed-off-by: zhengbin
> ---
> fs/fuse/virtio_fs.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index b77acea..2ac6818 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -684,12 +684,12 @@ static int virtio_fs_restore(struct virtio_device *vdev)
> }
> #endif /* CONFIG_PM_SLEEP */
>
> -const static struct virtio_device_id id_table[] = {
> +static const struct virtio_device_id id_table[] = {
> { VIRTIO_ID_FS, VIRTIO_DEV_ANY_ID },
> {},
> };
>
> -const static unsigned int feature_table[] = {};
> +static const unsigned int feature_table[] = {};
>
> static struct virtio_driver virtio_fs_driver = {
> .driver.name= KBUILD_MODNAME,
> @@ -1026,7 +1026,7 @@ __releases(fiq->lock)
> }
> }
>
> -const static struct fuse_iqueue_ops virtio_fs_fiq_ops = {
> +static const struct fuse_iqueue_ops virtio_fs_fiq_ops = {
> .wake_forget_and_unlock = virtio_fs_wake_forget_and_unlock,
> .wake_interrupt_and_unlock = virtio_fs_wake_interrupt_and_unlock,
> .wake_pending_and_unlock= virtio_fs_wake_pending_and_unlock,
> --
> 2.7.4
>
signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 01/13] compiler.h: Split {READ, WRITE}_ONCE definitions out into rwonce.h
On Mon, Nov 11, 2019 at 9:10 AM Christian Borntraeger
wrote:
> On 08.11.19 20:57, Arnd Bergmann wrote:
> > On Fri, Nov 8, 2019 at 6:01 PM Will Deacon wrote:
> >>
> >> In preparation for allowing architectures to define their own
> >> implementation of the 'READ_ONCE()' macro, move the generic
> >> '{READ,WRITE}_ONCE()' definitions out of the unwieldy 'linux/compiler.h'
> >> and into a new 'rwonce.h' header under 'asm-generic'.
> >
> > Adding Christian Bornträger to Cc, he originally added the
> > READ_ONCE()/WRITE_ONCE()
> > code.
> >
> > I wonder if it would be appropriate now to revert back to a much simpler
> > version
> > of these helpers for any modern compiler. As I understand, only gcc-4.6 and
> > gcc4.7 actually need the song-and-dance version with the union and
> > switch/case,
> > while for others, we can might be able back to a macro doing a volatile
> > access.
>
> As far as I know this particular issue with volatile access on aggregate
> types
> was fixed in gcc 4.8. On the other hand we know that the current construct
> will
> work on all compilers. Not so sure about the orignal ACCESS_ONCE
> implementation.
I've seen problems with clang on the current version, leading to unnecessary
temporaries being spilled to the stack in some cases, so I think it would still
help to simplify it.
We probably don't want the exact ACCESS_ONCE() implementation back
that existed before, but rather something that implements the stricter
READ_ONCE() and WRITE_ONCE(). I'd probably also want to avoid the
__builtin_memcpy() exception for odd-sized accesses and instead have
a separate way to do those.
Arnd
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 01/13] compiler.h: Split {READ, WRITE}_ONCE definitions out into rwonce.h
On 08.11.19 20:57, Arnd Bergmann wrote:
> On Fri, Nov 8, 2019 at 6:01 PM Will Deacon wrote:
>>
>> In preparation for allowing architectures to define their own
>> implementation of the 'READ_ONCE()' macro, move the generic
>> '{READ,WRITE}_ONCE()' definitions out of the unwieldy 'linux/compiler.h'
>> and into a new 'rwonce.h' header under 'asm-generic'.
>
> Adding Christian Bornträger to Cc, he originally added the
> READ_ONCE()/WRITE_ONCE()
> code.
>
> I wonder if it would be appropriate now to revert back to a much simpler
> version
> of these helpers for any modern compiler. As I understand, only gcc-4.6 and
> gcc4.7 actually need the song-and-dance version with the union and
> switch/case,
> while for others, we can might be able back to a macro doing a volatile
> access.
As far as I know this particular issue with volatile access on aggregate types
was fixed in gcc 4.8. On the other hand we know that the current construct will
work on all compilers. Not so sure about the orignal ACCESS_ONCE implementation.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
