Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 31 May 2023 18:22:19 +0200
From:   Simon Horman <simon.horman@corigine.com>
To:     Bobby Eshleman <bobby.eshleman@bytedance.com>
Cc:     Stefan Hajnoczi <stefanha@redhat.com>,
        Stefano Garzarella <sgarzare@redhat.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "K. Y. Srinivasan" <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>,
        Bryan Tan <bryantan@vmware.com>,
        Vishnu Dasa <vdasa@vmware.com>,
        VMware PV-Drivers Reviewers <pv-drivers@vmware.com>,
        kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hyperv@vger.kernel.org
Subject: Re: [PATCH RFC net-next v3 7/8] vsock: Add lockless sendmsg() support
Message-ID: <ZHd0O1L7FH2XJEnd@corigine.com>
References: <20230413-b4-vsock-dgram-v3-0-c2414413ef6a@bytedance.com>
 <20230413-b4-vsock-dgram-v3-7-c2414413ef6a@bytedance.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230413-b4-vsock-dgram-v3-7-c2414413ef6a@bytedance.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?vg8sEP3ZReXmc0eHMfNOXBsnhq1I3zHEzEEEVsauF9Kctai/NS9rsxSkf5KU?=
 =?us-ascii?Q?mW9KA4uTVUudzPKcKbuVyzRTLCeeeOsTiQOM/57DeC1V9vyHs3WpXQEuAzlo?=
 =?us-ascii?Q?bK8swoLHOhpI+szypXjReR0BkOriNCkfFj2yOZALrgiWOqrKvL3AdqGOpbQX?=
 =?us-ascii?Q?+SQH/PDtgKaj9mTBpLItMRB6266Me7O8C7WyByl+PsIzf2woSAZ7C0zC/M1H?=
 =?us-ascii?Q?Pky6c8ZkGeWY4pgEWwREunKGSzyieVZhIL1iFl5xfIICwwBPxburryPaf99o?=
 =?us-ascii?Q?/qu9L/9X1QlYPGu1QOjQiOt2PCmiXByjY+AwbBOsL3uGpv6nG0NmGkQPXLhE?=
 =?us-ascii?Q?b5ZKFvKYoM5JorgD6VRnn0LPnwP3c/e2E+ljeXzoINfARkivfd3/9DPvp8vj?=
 =?us-ascii?Q?lgtluAJJ/6cd9VQ/rcH181eu6+BeJ57HIA+LGamQ6Y+y5TVJ0XiYn92VilmU?=
 =?us-ascii?Q?4v7Y60XwwQ17aEPc6DN+dHsWq0OqWskejmbwHHehRO1+K83j4ZaHn/bT2c4U?=
 =?us-ascii?Q?0LbAe7ORguXdrWIDF3i9ydCreZsHIV2yCkpFczJrYeA8CuoTKx1oR44MVij/?=
 =?us-ascii?Q?Oyvz/iKKbEJ24GvCS5IoQqDY109RIP7dx9ngTCyxovwadfscPV0GIgfFCDxt?=
 =?us-ascii?Q?j/UiKrxTGbvFeaNuDKmd921TooEPHCKFvJQ8bG+3CZ/po7pH5cW/iuU6Wa6b?=
 =?us-ascii?Q?TpLE+J0/l4TN6YntSwlesYA39StQl13AIDZs5iWu0EroVN2HwgWoWqphUImV?=
 =?us-ascii?Q?emttUDiXDHgUw2K//Qad1SRYyMovsaIXql/i0x8/HQByxqlRMXKcIHdwUFx1?=
 =?us-ascii?Q?eJ9mqN66HSqCfROW2rc18Yr/6L+e1xKdhe2ADe5oLe4eF7ZMqqPDYHGWJ+u/?=
 =?us-ascii?Q?Sa+WnLdK0PvsyXI0pFDa0Z1zYBfmlL4NGLvG+1VhyetVEpVfk6k5kdW76Wa6?=
 =?us-ascii?Q?weXBhsPWolg8eFjgThSaSdwXO0RH57AY7WAIAfPjW12e+qrxTuWqRomYgcOE?=
 =?us-ascii?Q?3hYVe9Abpjv5ddnu+2WTb903lePGhP0zWjftRiVHa4u+dPKFVjOhuWWP3l92?=
 =?us-ascii?Q?9QRYI5CIxyrUFtqbCjgH4kKMpiGfsEqddK3pKV4TeD7j8ZMnJVu4oIz66IZ+?=
 =?us-ascii?Q?UdLf+k47bU7DRI0zC04QqvQUrN/CDxmNrkcNKn395AGZbqA+u8qbIf+BCoFs?=
 =?us-ascii?Q?GnVvBbpIERNiYIC7jAk3o0KKMchNB51IFN2kT15kC7iiBSwkFUoezbLvTWFw?=
 =?us-ascii?Q?7jzdExu31ea3nG2jJJRWxSUqC21JtJm04zbcJRgRyuYrVQ/j9D/MLFHy5LOq?=
 =?us-ascii?Q?c36WFAZbfMp8dngH2uOwQnizON0BvkJvBHnNAE1aSNzqEIxxMiuu4ByPM72z?=
 =?us-ascii?Q?6AIcEgrFUhw4tInZilSfVSYar1N/h+IIu2yFy0RX4fXwSmb1a/QhzRIehV1s?=
 =?us-ascii?Q?ykgWBjPqhjfRaASCP21+Fb/KQtv1NmBoYoYjXHJhmEtztM7DvI3rO63/fsmZ?=
 =?us-ascii?Q?+Lyowq4N4fJL92M0JqOEQdHi81dkDS0nzihJ+O+grqAbfgOHXcOMUyMewqXd?=
 =?us-ascii?Q?lcz/zHgDKCal+haTP0YLyCsgkPpYhf88lS81oEQ4Mg6cgjMDlJV9wGZw1JMR?=
 =?us-ascii?Q?bybClH9iRpRWWnXw72JA3oXcpJCwffwiwX/S/aRjBg5BwnSI7r7n/8t9DUDm?=
 =?us-ascii?Q?bQ//9Q=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dafe0ad1-1ade-4b73-c472-08db61f33955
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2023 16:22:27.2722
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: R1FzCkie19MjJwBjduRnvKipKA2NzRENuWeySDh1c+UDzJvWhyqMZP0HgtixiLhpaBdsrdTOVarFz7FMlj22wGYYB4CC5R3z6Dc0CUqc73I=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR13MB3717
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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:

...