Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 22 Nov 2019 11:02:12 +0100
From:   Stefano Garzarella <sgarzare@redhat.com>
To:     Stefan Hajnoczi <stefanha@redhat.com>
Cc:     Stefan Hajnoczi <stefanha@gmail.com>, netdev@vger.kernel.org,
        virtualization@lists.linux-foundation.org,
        Dexuan Cui <decui@microsoft.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
        Jorgen Hansen <jhansen@vmware.com>
Subject: Re: [PATCH net-next 4/6] vsock: add vsock_loopback transport
Message-ID: <20191122100212.u3mvt6qkay7zexz7@steredhat.homenet.telecomitalia.it>
References: <20191119110121.14480-1-sgarzare@redhat.com>
 <20191119110121.14480-5-sgarzare@redhat.com>
 <20191121093458.GB439743@stefanha-x1.localdomain>
 <20191121095948.bc7lc3ptsh6jxizw@steredhat>
 <20191121152517.zfedz6hg6ftcb2ks@steredhat>
 <20191122092546.GA464656@stefanha-x1.localdomain>
MIME-Version: 1.0
In-Reply-To: <20191122092546.GA464656@stefanha-x1.localdomain>
Content-Type: text/plain; charset=WINDOWS-1252
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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:
> > 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;
> > > >=20
> > > > the_vsock_loopback could be a static global variable (not a pointer=
) and
> > > > vsock_loopback_workqueue could also be included in the struct.
> > > >=20
> > > > The RCU pointer is really a way to synchronize vsock_loopback_send_=
pkt()
> > > > and vsock_loopback_cancel_pkt() with module exit.  There is no othe=
r
> > > > reason for using a pointer.
> > > >=20
> > > > It's cleaner to implement the synchronization once in af_vsock.c (o=
r
> > > > virtio_transport_common.c) instead of making each transport do it.
> > > > Maybe try_module_get() and related APIs provide the necessary seman=
tics
> > > > so that core vsock code can hold the transport module while it's be=
ing
> > > > used to send/cancel a packet.
> > >=20
> > > Right, the module cannot be unloaded until open sockets, so here the
> > > synchronization is not needed.
> > >=20
> > > The synchronization come from virtio-vsock device that can be
> > > hot-unplugged while sockets are still open, but that can't happen her=
e.
> > >=20
> > > I will remove the pointers and RCU in the v2.
> > >=20
> > > Can I keep your R-b or do you prefer to watch v2 first?
>=20
> I'd like to review v2.
>=20

Sure!

> > > > > +MODULE_ALIAS_NETPROTO(PF_VSOCK);
> > > >=20
> > > > Why does this module define the alias for PF_VSOCK?  Doesn't anothe=
r
> > > > module already define this alias?
> > >=20
> > > 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.
> > >=20
> > > In virtio_transport we don't need it because it will be loaded when
> > > the PCI device is discovered.
> > >=20
> > > Do you think there's a better way?
> > > Should I include the vsock_loopback transport directly in af_vsock
> > > without creating a new module?
> > >=20
> >=20
> > 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.
> >=20
> > Do you think it's okay in this case to keep MODULE_ALIAS_NETPROTO(PF_VS=
OCK)
> > or is there a better way?
>=20
> 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