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 Date: Fri, 22 Nov 2019 11:02:12 +0100 [thread overview] Message-ID: <20191122100212.u3mvt6qkay7zexz7@steredhat.homenet.telecomitalia.it> (raw) In-Reply-To: <20191122092546.GA464656@stefanha-x1.localdomain> 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; > > > > > > > > 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! > > > > > +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
WARNING: multiple messages have this Message-ID (diff)
From: Stefano Garzarella <sgarzare@redhat.com> To: Stefan Hajnoczi <stefanha@redhat.com> Cc: kvm@vger.kernel.org, Dexuan Cui <decui@microsoft.com>, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, netdev@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 Date: Fri, 22 Nov 2019 11:02:12 +0100 [thread overview] Message-ID: <20191122100212.u3mvt6qkay7zexz7@steredhat.homenet.telecomitalia.it> (raw) In-Reply-To: <20191122092546.GA464656@stefanha-x1.localdomain> 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; > > > > > > > > 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! > > > > > +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
next prev parent reply other threads:[~2019-11-22 10:02 UTC|newest] Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-19 11:01 [PATCH net-next 0/6] vsock: add local transport support Stefano Garzarella 2019-11-19 11:01 ` [PATCH net-next 1/6] vsock/virtio_transport_common: remove unused virtio header includes Stefano Garzarella 2019-11-19 11:01 ` [PATCH net-next 2/6] vsock: add VMADDR_CID_LOCAL definition Stefano Garzarella 2019-11-21 14:49 ` Jorgen Hansen via Virtualization 2019-11-21 14:49 ` Jorgen Hansen 2019-11-19 11:01 ` [PATCH net-next 3/6] vsock: add local transport support in the vsock core Stefano Garzarella 2019-11-21 15:04 ` Jorgen Hansen 2019-11-21 15:21 ` Stefano Garzarella 2019-11-21 15:21 ` Stefano Garzarella 2019-11-21 15:53 ` Jorgen Hansen 2019-11-21 16:12 ` Stefano Garzarella 2019-11-21 16:19 ` Jorgen Hansen 2019-11-21 16:19 ` Jorgen Hansen via Virtualization 2019-11-21 16:12 ` Stefano Garzarella 2019-11-21 15:53 ` Jorgen Hansen via Virtualization 2019-11-21 15:04 ` Jorgen Hansen via Virtualization 2019-11-19 11:01 ` [PATCH net-next 4/6] vsock: add vsock_loopback transport Stefano Garzarella 2019-11-20 1:15 ` David Miller 2019-11-20 9:00 ` Stefano Garzarella 2019-11-21 9:34 ` Stefan Hajnoczi 2019-11-21 9:34 ` Stefan Hajnoczi 2019-11-21 9:59 ` Stefano Garzarella 2019-11-21 9:59 ` Stefano Garzarella 2019-11-21 15:25 ` Stefano Garzarella 2019-11-22 9:25 ` Stefan Hajnoczi 2019-11-22 9:25 ` Stefan Hajnoczi 2019-11-22 10:02 ` Stefano Garzarella [this message] 2019-11-22 10:02 ` Stefano Garzarella 2019-11-21 15:25 ` Stefano Garzarella 2019-11-19 11:01 ` [PATCH net-next 5/6] vsock: use local transport when it is loaded Stefano Garzarella 2019-11-21 9:46 ` Stefan Hajnoczi 2019-11-21 9:46 ` Stefan Hajnoczi 2019-11-21 10:01 ` Stefano Garzarella 2019-11-21 10:01 ` Stefano Garzarella 2019-11-19 11:01 ` [PATCH net-next 6/6] vsock/virtio: remove loopback handling Stefano Garzarella 2019-11-21 9:46 ` [PATCH net-next 0/6] vsock: add local transport support Stefan Hajnoczi 2019-11-21 10:05 ` Stefano Garzarella 2019-11-21 10:05 ` Stefano Garzarella 2019-11-21 9:46 ` Stefan Hajnoczi 2019-11-21 14:45 ` Jorgen Hansen via Virtualization 2019-11-21 14:45 ` Jorgen Hansen 2019-11-21 15:13 ` Stefano Garzarella 2019-11-21 15:13 ` Stefano Garzarella
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191122100212.u3mvt6qkay7zexz7@steredhat.homenet.telecomitalia.it \ --to=sgarzare@redhat.com \ --cc=davem@davemloft.net \ --cc=decui@microsoft.com \ --cc=jhansen@vmware.com \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=stefanha@gmail.com \ --cc=stefanha@redhat.com \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.