Re: [RFC PATCH net-next v6 07/14] virtio/vsock: add common datagram send path

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jul 26, 2024 at 04:22:16PM GMT, Amery Hung wrote:
On Tue, Jul 23, 2024 at 7:42 AM Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:

On Wed, Jul 10, 2024 at 09:25:48PM GMT, Amery Hung wrote:
>From: Bobby Eshleman <bobby.eshleman@xxxxxxxxxxxxx>
>
>This commit implements the common function
>virtio_transport_dgram_enqueue for enqueueing datagrams. It does not add
>usage in either vhost or virtio yet.
>
>Signed-off-by: Bobby Eshleman <bobby.eshleman@xxxxxxxxxxxxx>
>Signed-off-by: Amery Hung <amery.hung@xxxxxxxxxxxxx>
>---
> include/linux/virtio_vsock.h            |  1 +
> include/net/af_vsock.h                  |  2 +
> net/vmw_vsock/af_vsock.c                |  2 +-
> net/vmw_vsock/virtio_transport_common.c | 87 ++++++++++++++++++++++++-
> 4 files changed, 90 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>index f749a066af46..4408749febd2 100644
>--- a/include/linux/virtio_vsock.h
>+++ b/include/linux/virtio_vsock.h
>@@ -152,6 +152,7 @@ struct virtio_vsock_pkt_info {
>       u16 op;
>       u32 flags;
>       bool reply;
>+      u8 remote_flags;
> };
>
> struct virtio_transport {
>diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>index 44db8f2c507d..6e97d344ac75 100644
>--- a/include/net/af_vsock.h
>+++ b/include/net/af_vsock.h
>@@ -216,6 +216,8 @@ void vsock_for_each_connected_socket(struct vsock_transport *transport,
>                                    void (*fn)(struct sock *sk));
> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk);
> bool vsock_find_cid(unsigned int cid);
>+const struct vsock_transport *vsock_dgram_lookup_transport(unsigned int cid,
>+                                                         __u8 flags);

Why __u8 and not just u8?


Will change to u8.


>
> struct vsock_skb_cb {
>       unsigned int src_cid;
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index ab08cd81720e..f83b655fdbe9 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -487,7 +487,7 @@ vsock_connectible_lookup_transport(unsigned int cid, __u8 flags)
>       return transport;
> }
>
>-static const struct vsock_transport *
>+const struct vsock_transport *
> vsock_dgram_lookup_transport(unsigned int cid, __u8 flags)
> {
>       const struct vsock_transport *transport;
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index a1c76836d798..46cd1807f8e3 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1040,13 +1040,98 @@ int virtio_transport_shutdown(struct vsock_sock *vsk, int mode)
> }
> EXPORT_SYMBOL_GPL(virtio_transport_shutdown);
>
>+static int virtio_transport_dgram_send_pkt_info(struct vsock_sock *vsk,
>+                                              struct virtio_vsock_pkt_info *info)
>+{
>+      u32 src_cid, src_port, dst_cid, dst_port;
>+      const struct vsock_transport *transport;
>+      const struct virtio_transport *t_ops;
>+      struct sock *sk = sk_vsock(vsk);
>+      struct virtio_vsock_hdr *hdr;
>+      struct sk_buff *skb;
>+      void *payload;
>+      int noblock = 0;
>+      int err;
>+
>+      info->type = virtio_transport_get_type(sk_vsock(vsk));
>+
>+      if (info->pkt_len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE)
>+              return -EMSGSIZE;
>+
>+      transport = vsock_dgram_lookup_transport(info->remote_cid, info->remote_flags);

Can `transport` be null?

I don't understand why we are calling vsock_dgram_lookup_transport()
again. Didn't we already do that in vsock_dgram_sendmsg()?


transport should be valid here sin)e we null-checked it in
vsock_dgram_sendmsg(). The reason vsock_dgram_lookup_transport() is
called again here is we don't have the transport when we called into
transport->dgram_enqueue(). I can also instead add transport to the
argument of dgram_enqueue() to eliminate this redundant lookup.

Yes, I would absolutely eliminate this double lookup.

You can add either a parameter, or define the callback in each transport and internally use the statically allocated transport in each.

For example for vhost/vsock.c:

static int vhost_transport_dgram_enqueue(....) {
    return virtio_transport_dgram_enqueue(&vhost_transport.transport,
                                          ...)
}

In virtio_transport_recv_pkt() we already do something similar.


Also should we add a comment mentioning that we can't use
virtio_transport_get_ops()? IIUC becuase the vsk can be not assigned
to a specific transport, right?


Correct. For virtio dgram socket, transport is not assigned unless
vsock_dgram_connect() is called. I will add a comment here explaining
this.

Thanks,
Stefano





[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux