Re: [PATCH net-next 4/6] vsock: add vsock_loopback transport

[Stefan Hajnoczi]

On Thu, Nov 21, 2019 at 04:25:17PM +0100, Stefano Garzarella wrote:
> On Thu, Nov 21, 2019 at 10:59:48AM +0100, Stefano Garzarella wrote:
> > On Thu, Nov 21, 2019 at 09:34:58AM +0000, Stefan Hajnoczi wrote:
> > > On Tue, Nov 19, 2019 at 12:01:19PM +0100, Stefano Garzarella wrote:
> > > > +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.

> > > > +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 ...?

Stefan

Attachment: signature.asc
Description: PGP signature