On Wed, May 31, 2023 at 12:35:11AM +0000, Bobby Eshleman wrote:
...
Hi Bobby,
some more feedback from my side.
> Throughput metrics for single-threaded SOCK_DGRAM and
> single/multi-threaded SOCK_STREAM showed no statistically signficant
nit: s/signficant/significant/
> throughput changes (lowest p-value reaching 0.27), with the range of the
> mean difference ranging between -5% to +1%.
>
> Signed-off-by: Bobby Eshleman <bobby.eshleman@xxxxxxxxxxxxx>
...
> @@ -120,8 +125,8 @@ struct vsock_transport {
>
> /* DGRAM. */
> int (*dgram_bind)(struct vsock_sock *, struct sockaddr_vm *);
> - int (*dgram_enqueue)(struct vsock_sock *, struct sockaddr_vm *,
> - struct msghdr *, size_t len);
> + int (*dgram_enqueue)(const struct vsock_transport *, struct vsock_sock *,
> + struct sockaddr_vm *, struct msghdr *, size_t len);
Perhaps just a personal preference, but the arguments for these callbacks
could have names.
> bool (*dgram_allow)(u32 cid, u32 port);
> int (*dgram_get_cid)(struct sk_buff *skb, unsigned int *cid);
> int (*dgram_get_port)(struct sk_buff *skb, unsigned int *port);
> @@ -196,6 +201,17 @@ void vsock_core_unregister(const struct vsock_transport *t);
> /* The transport may downcast this to access transport-specific functions */
> const struct vsock_transport *vsock_core_get_transport(struct vsock_sock *vsk);
>
> +static inline struct vsock_remote_info *
> +vsock_core_get_remote_info(struct vsock_sock *vsk)
> +{
> +
nit: no blank line here
> + /* vsk->remote_info may be accessed if the rcu read lock is held OR the
> + * socket lock is held
> + */
> + return rcu_dereference_check(vsk->remote_info,
> + lockdep_sock_is_held(sk_vsock(vsk)));
> +}
> +
> /**** UTILS ****/
>
> /* vsock_table_lock must be held */
...
> @@ -300,17 +449,36 @@ static void vsock_insert_unbound(struct vsock_sock *vsk)
> spin_unlock_bh(&vsock_table_lock);
> }
>
> -void vsock_insert_connected(struct vsock_sock *vsk)
> +int vsock_insert_connected(struct vsock_sock *vsk)
> {
> - struct list_head *list = vsock_connected_sockets(
> - &vsk->remote_addr, &vsk->local_addr);
> + struct list_head *list;
> + struct vsock_remote_info *remote_info;
nit: I know that this file doesn't follow the reverse xmas tree
scheme - longest line to shortest - for local variable declarations.
But as networking code I think it would be good towards towards
that scheme as code is changed.
struct vsock_remote_info *remote_info;
struct list_head *list;
> +
> + rcu_read_lock();
> + remote_info = vsock_core_get_remote_info(vsk);
> + if (!remote_info) {
> + rcu_read_unlock();
> + return -EINVAL;
> + }
> + list = vsock_connected_sockets(&remote_info->addr, &vsk->local_addr);
> + rcu_read_unlock();
>
> spin_lock_bh(&vsock_table_lock);
> __vsock_insert_connected(list, vsk);
> spin_unlock_bh(&vsock_table_lock);
> +
> + return 0;
> }
...
> @@ -1120,7 +1122,9 @@ virtio_transport_recv_connecting(struct sock *sk,
> case VIRTIO_VSOCK_OP_RESPONSE:
> sk->sk_state = TCP_ESTABLISHED;
> sk->sk_socket->state = SS_CONNECTED;
> - vsock_insert_connected(vsk);
> + err = vsock_insert_connected(vsk);
> + if (err)
> + goto destroy;
The destroy label uses skerr, but it is uninitialised here.
A W=1 or C=1 will probably tell you this.
> sk->sk_state_change(sk);
> break;
> case VIRTIO_VSOCK_OP_INVALID:
...
