Re: [PATCH RFC net-next v3 7/8] vsock: Add lockless sendmsg() support

[Simon Horman <simon.horman@corigine.com>]

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@bytedance.com>

...

> @@ -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:

...