On Fri, Nov 22, 2019 at 09:25:46AM +0000, Stefan Hajnoczi wrote:

On Thu, Nov 21, 2019 at 04:25:17PM +0100, Stefano Garzarella wrote:

quoted

On Thu, Nov 21, 2019 at 10:59:48AM +0100, Stefano Garzarella wrote:

quoted

On Thu, Nov 21, 2019 at 09:34:58AM +0000, Stefan Hajnoczi wrote:

quoted

On Tue, Nov 19, 2019 at 12:01:19PM +0100, Stefano Garzarella wrote:

quoted

+static struct workqueue_struct *vsock_loopback_workqueue;
+static struct vsock_loopback *the_vsock_loopback;

the_vsock_loopback could be a static global variable (not a pointer) and
vsock_loopback_workqueue could also be included in the struct.

The RCU pointer is really a way to synchronize vsock_loopback_send_pkt()
and vsock_loopback_cancel_pkt() with module exit.  There is no other
reason for using a pointer.

It's cleaner to implement the synchronization once in af_vsock.c (or
virtio_transport_common.c) instead of making each transport do it.
Maybe try_module_get() and related APIs provide the necessary semantics
so that core vsock code can hold the transport module while it's being
used to send/cancel a packet.

Right, the module cannot be unloaded until open sockets, so here the
synchronization is not needed.

The synchronization come from virtio-vsock device that can be
hot-unplugged while sockets are still open, but that can't happen here.

I will remove the pointers and RCU in the v2.

Can I keep your R-b or do you prefer to watch v2 first?

I'd like to review v2.

Sure!

quoted

quoted

quoted

quoted

+MODULE_ALIAS_NETPROTO(PF_VSOCK);

Why does this module define the alias for PF_VSOCK?  Doesn't another
module already define this alias?

It is a way to load this module when PF_VSOCK is starting to be used.
MODULE_ALIAS_NETPROTO(PF_VSOCK) is already defined in vmci_transport
and hyperv_transport. IIUC it is used for the same reason.

In virtio_transport we don't need it because it will be loaded when
the PCI device is discovered.

Do you think there's a better way?
Should I include the vsock_loopback transport directly in af_vsock
without creating a new module?

That last thing I said may not be possible:
I remembered that I tried, but DEPMOD found a cyclic dependency because
vsock_transport use virtio_transport_common that use vsock, so if I
include vsock_transport in the vsock module, DEPMOD is not happy.

Do you think it's okay in this case to keep MODULE_ALIAS_NETPROTO(PF_VSOCK)
or is there a better way?

The reason I asked is because the semantics of duplicate module aliases
aren't clear to me.  Do all modules with the same alias get loaded?
Or just the first?  Or ...?

It wasn't clear to me either, but when I tried, I saw that all modules
with the same alias got loaded.

Stefano