Re: [PATCH net-next v3 3/4] vsock/virtio: coalesce RX write-space notifications in lock batches

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch is presented purely as a performance optimisation but
  gives no numbers.
- [Low] Retained for the separate validation policy; my baseline decision
  is that this is not a real defect.
- [Low] The patch changes the behaviour of three interfaces without
  updating their comments.

--

vsock/virtio: coalesce RX write-space notifications in lock batches

This commit stops calling sk_write_space() for every packet in a virtio
vsock RX lock batch. It records one pending notification and delivers it
through the saved default callback before release_sock(). The default
callback is saved in __vsock_create(), and sockets whose sk_write_space
has been replaced are no longer eligible for batching.

> Each received packet updates peer credit and calls sk_write_space() when
> send space is available. A writer cannot use the newly advertised credit
> until the socket lock is released, so repeated callbacks within one lock
> batch cannot let it make progress sooner.

[Severity: Low]
This shows that the change is safe, but are there any numbers showing it
helps? Neither this commit message nor the other three in the series
gives throughput, CPU or wakeup-count data, or names a workload where
per-packet sk_write_space() costs something measurable.

The change does have costs. Every struct vsock_sock gets a new function
pointer, including VMCI and Hyper-V sockets, but only the virtio RX batch
path reads it. virtio_transport_recv_pkt_batchable() gets one more
admission condition. The wakeup moves to
virtio_transport_rx_batch_finish().

The callback being coalesced is also cheap when nobody is waiting.
sock_def_write_space() only calls sock_writeable(), skwq_has_sleeper()
and sk_wake_async_rcu().

Documentation/process/submitting-patches.rst says:

  Quantify optimizations and trade-offs. If you claim improvements in
  performance ... include numbers that back them up. But also describe
  non-obvious costs.

Could the commit message include measurements that justify the extra
per-socket field?

> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> index 5549298c1ec6b..9d8ae62209eda 100644
> --- a/include/net/af_vsock.h
> +++ b/include/net/af_vsock.h
> @@ -63,6 +63,8 @@ struct vsock_sock {
>  	u32 peer_shutdown;
>  	bool sent_request;
>  	bool ignore_connecting_rst;
> +	/* Initial callback, used to identify replacements. */
> +	void (*default_write_space)(struct sock *sk);

[Severity: Low]
This comment only describes the identification role.
virtio_transport_rx_batch_finish() also calls this pointer to deliver
deferred notifications. Should the comment say that too?

Two other comments are also unchanged by this patch.

The comment above the exported virtio_transport_rx_batch_finish() still
reads:

  For a non-empty batch, the caller must hold the socket lock acquired with
  lock_sock(). This function releases the lock and the batch's lookup
  reference.

It does not say that a write-space callback now runs from this function
while the socket lock is still held. At the end of the series it also
runs a data-ready callback after release_sock(), and the comment is
still the same.

The comment above virtio_transport_recv_pkt_locked() documents
@batchable and the return value, but not the new @batch parameter:

  The caller holds sk's socket lock.  Set @batchable if the socket can remain
  locked for another ordinary STREAM/RW packet.  Return true if the caller
  must free @skb.

A non-NULL @batch suppresses the immediate sk->sk_write_space() call and
defers it to batch finish. Could these comments be updated to match?

[ ... ]

> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index bfe40ecf0b2f0..d6ebfde880db6 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c

[ ... ]

> @@ -1860,8 +1862,12 @@ virtio_transport_recv_pkt_locked(struct virtio_transport *t,
>  	if (vsk->local_addr.svm_cid != VMADDR_CID_ANY)
>  		vsk->local_addr.svm_cid = dst->svm_cid;
>  
> -	if (space_available)
> -		sk->sk_write_space(sk);
> +	if (space_available) {
> +		if (batch)
> +			batch->write_space_pending = true;
> +		else
> +			sk->sk_write_space(sk);
> +	}

[ ... ]

> @@ -1954,15 +1960,20 @@ EXPORT_SYMBOL_GPL(virtio_transport_recv_pkt);
>   */
>  void virtio_transport_rx_batch_finish(struct virtio_transport_rx_batch *batch)
>  {
> +	bool write_space_pending = batch->write_space_pending;
>  	struct sock *sk = batch->sk;
>  
>  	batch->sk = NULL;
>  	batch->pkts = 0;
>  	batch->bytes = 0;
> +	batch->write_space_pending = false;
>  
>  	if (!sk)
>  		return;
>  
> +	if (write_space_pending)
> +		vsock_sk(sk)->default_write_space(sk);

[Severity: Low]
Is it intended to call the saved default callback here rather than the
current sk->sk_write_space?

sock_map_update_elem() from BPF takes only bh_lock_sock(). It can
therefore attach a psock while the RX batch holds lock_sock(). In that
case sk_psock_start_verdict() does:

	WRITE_ONCE(sk->sk_write_space, sk_psock_write_space);

A notification latched for a packet admitted before the attach would
then skip sk_psock_write_space(), which is what schedules psock->work.

>From a closer look, this may be harmless:

- virtio_transport_sock_has_psock() is checked at every packet
  boundary, and the psock is published before sk_write_space is
  replaced. So only a packet admitted before that point can leave a
  pending notification. That is the same outcome as the pre-series case
  where the attach lands just after that packet's own sk_write_space()
  call.

- The next packet fails admission and takes the non-batched path, which
  calls the new sk_psock_write_space().

- sk_psock_backlog() reschedules itself with
  schedule_delayed_work(&psock->work, 1) on -EAGAIN. It is also
  scheduled on enqueue, so psock TX should not stall.

- sk_psock_write_space() forwards to psock->saved_write_space, so
  user-space writers get the same wakeup either way.

If that reasoning holds, would a short comment explaining why it is safe
to bypass a callback installed mid-batch help future readers?

> +
>  	release_sock(sk);
>  	sock_put(sk);
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013063112.168321-1-physicalmtea%40gmail.com