From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f44.google.com (mail-wm1-f44.google.com [209.85.128.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9089531D727 for ; Fri, 12 Dec 2025 11:40:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765539612; cv=none; b=m/uyYf9zQHNXzUqhUQ0aQzZI3yrzu1igLxtU04kpipfsSOfO5AwXc4TBpJ1R6qymFp6sdMvs6zpQN2+LDfwyKwbEtksHQASfCcCcwrqb5/7A0aUCJXcqnvR1HqluNXYy7uP6BqgxOCbmeVjEfzTZUioPWSoVKIuh0VKOxph5voQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765539612; c=relaxed/simple; bh=gNsOPmDAe2451KGz9hpylpCVm15a1m7Emxxu7hWfmhQ=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=AK9JAfBCgHBrmutuEQIGfwF/TKnYdbGWGmKaAln/YFa4fBkTJXpshIEp+mHL68RwfeUcSa8fIaXIVOoASXLb6wNrRPZ5Ktyn6Sm7pfDGRDbC+WTCRSdjLYmTv/lLriWe+UwXeLrhVQxh7s5JUWhhMmuzqj37LAhxxmFrH8y+xKI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=fgVVOF/2; arc=none smtp.client-ip=209.85.128.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="fgVVOF/2" Received: by mail-wm1-f44.google.com with SMTP id 5b1f17b1804b1-4779d47be12so10715545e9.2 for ; Fri, 12 Dec 2025 03:40:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1765539606; x=1766144406; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=lXEuhHp/TJS8bvwbNw0xDmmO/TWBfqgggwlo68iD50w=; b=fgVVOF/2XaAwUHdPmaz0XX9KdTJ8tJt5SjMgmGmaZxxMJNnZGmEdEyvl6MOAGPOFbv 1TNUxHMl/26IrA4DKwora5XGj3Gf3hOO6XdiZTCBsoico/XGGw+0v5UxQAKuXzBcKQYl ssoeQAapkJcOBcKXltU5Uu3xf5Cjz13+rZakmRZlkfVTtiqfdr3xzm+nYy3v5uoD7d2t uKkwHz+JDYTBNT9x17Eq3TZmntitn1GQlP8ODs5EGho5F9sfvph5pM2xCsqihcJYZVHd UdXNlkpfow+cGEaIjVnC+7as1msrZ4LpVwLaKk4oyRcROuWMwyoODxb8XgoSJV8KZPCT nkqQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1765539606; x=1766144406; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=lXEuhHp/TJS8bvwbNw0xDmmO/TWBfqgggwlo68iD50w=; b=vqbBEgNweHqnpaxcOR/OFMac0V0+VfTiXXxXnCGDiM47wiYX4n99/S0adybMKRzIlx 1hkYSVG7O4DdYvzN08XSOG1HnAfHbH91o0TZPTdUPzlCKRIQP+mLQ9ixpK2KP7NsBYuq V8OARrEx67kSP0D+3TP4s54Y0GEZOzBQGnA9vmz+VEU/VLAgDUYSgO2iEI9oTIRIxz11 MRFOvVraAi4MHBoyXNGd9vNFQQs0vKUKO01x8BitbdkOe2J2O9IeYlDwjRHLDCrVR2CD Zi7IYnVu2mZVFYDV0PmHGLYp+R5GjT16p6BSkDeybofBtpt6Kj2kg3eUTRHFs0w/GTN7 SQQg== X-Forwarded-Encrypted: i=1; AJvYcCXrui+eEBUFTvvXcOwqs5+jwIKqJEyvZCKOOv37DifH2AGoJXjUyl3R3Iq6FxOnU4+spUIyjkY3d3jDOjU=@vger.kernel.org X-Gm-Message-State: AOJu0Yx2YJMKt9ToPS7ZMXl2OEum5Lv5Ge6zOP5XVlZFHpe3n9WYQEbK jlecnx6VMY6xw0rztAE7TL0OSSi8w00oTPyrc8lY7Vr3EK8doOQL2Abl X-Gm-Gg: AY/fxX6aarOSOI2x7gnN6SlnSh97V3OdF16cZEpKtYRZ1G6NI93MRn+BFy808bUVN58 wJEVnYcaOv45FT2BRNnU2G65dEqYiWckZVO86E6q8WYqwImB/eFdNlMBNSz8xXYhwWKYD2plIsF ZdC9n+X9s0nkGoiU1sI4TJSq6gWbmhFvf2Ew0g0Eg8cDTeQFSiKdm27UpeVf+rS/rkvXTCcMbDn dxSqi6xejutnsJLfYKxGmahjDfEDcEXMFJqdep//g38sGX/Hxgf16x4duc8BkkpAzkWA+t3qbS5 n6c/9BD1Qj0GwSw/7/YTPfH9L9ST6MQNnN5X5qrYoA+yhlKjEYDvX/cdj3LmhXkN0ynvWzfxQD1 Pgza8Et9FTeIhdWvlKvIJI7YBsn3nduN2M70mMDGjnaRq0j8q0DCYejiKl2wZuiH/9lZviaC8q3 +VehRU75pjs6wPIGyJbylMripqg8sVYT5eiS9j9wCxMPf8T5tiERlcHTZLEYZSONDCQ5vJpz3wH 6oN06Y= X-Google-Smtp-Source: AGHT+IE3kaghFqaChtHnkXEcnlbvOdlUAWt+A8n6UrtZfPjq4lzmXLtYTWWXSNLEGevXzX336tSFOA== X-Received: by 2002:a05:600c:1988:b0:477:af07:dd1c with SMTP id 5b1f17b1804b1-47a8f914ec2mr17252625e9.35.1765539605550; Fri, 12 Dec 2025 03:40:05 -0800 (PST) Received: from [192.168.0.173] (108.228-30-62.static.virginmediabusiness.co.uk. [62.30.228.108]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-47a8f7676ffsm27866535e9.4.2025.12.12.03.40.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 12 Dec 2025 03:40:04 -0800 (PST) Message-ID: Date: Fri, 12 Dec 2025 11:40:03 +0000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net v3] vsock/virtio: cap TX credit to local buffer size Content-Language: en-GB To: Stefano Garzarella Cc: "Michael S. Tsirkin" , stefanha@redhat.com, kvm@vger.kernel.org, netdev@vger.kernel.org, virtualization@lists.linux.dev, linux-kernel@vger.kernel.org, jasowang@redhat.com, xuanzhuo@linux.alibaba.com, eperezma@redhat.com, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, horms@kernel.org References: <20251211125104.375020-1-mlbnkm1@gmail.com> <20251211080251-mutt-send-email-mst@kernel.org> From: Melbin K Mathew In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 12/12/2025 10:40, Stefano Garzarella wrote: > On Fri, Dec 12, 2025 at 09:56:28AM +0000, Melbin Mathew Antony wrote: >> Hi Stefano, Michael, >> >> Thanks for the suggestions and guidance. > > You're welcome, but please avoid top-posting in the future: > https://www.kernel.org/doc/html/latest/process/submitting- > patches.html#use-trimmed-interleaved-replies-in-email-discussions > Sure. Thanks >> >> I’ve drafted a 4-part series based on the recap. I’ve included the >> four diffs below for discussion. Can wait for comments, iterate, and >> then send the patch series in a few days. >> >> --- >> >> Patch 1/4 — vsock/virtio: make get_credit() s64-safe and clamp negatives >> >> virtio_transport_get_credit() was doing unsigned arithmetic; if the >> peer shrinks its window, the subtraction can underflow and look like >> “lots of credit”. This makes it compute “space” in s64 and clamp < 0 >> to 0. >> >> diff --git a/net/vmw_vsock/virtio_transport_common.c >> b/net/vmw_vsock/virtio_transport_common.c >> --- a/net/vmw_vsock/virtio_transport_common.c >> +++ b/net/vmw_vsock/virtio_transport_common.c >> @@ -494,16 +494,23 @@ >> EXPORT_SYMBOL_GPL(virtio_transport_consume_skb_sent); >> u32 virtio_transport_get_credit(struct virtio_vsock_sock *vvs, u32 >> credit) >> { >> + s64 bytes; >>  u32 ret; >> >>  if (!credit) >>  return 0; >> >>  spin_lock_bh(&vvs->tx_lock); >> - ret = vvs->peer_buf_alloc - (vvs->tx_cnt - vvs->peer_fwd_cnt); >> - if (ret > credit) >> - ret = credit; >> + bytes = (s64)vvs->peer_buf_alloc - > > Why not just calling virtio_transport_has_space()? virtio_transport_has_space() takes struct vsock_sock *, while virtio_transport_get_credit() takes struct virtio_vsock_sock *, so I cannot directly call has_space() from get_credit() without changing signatures. Would you be OK if I factor the common “space” calculation into a small helper that operates on struct virtio_vsock_sock * and is used by both paths? Something like: /* Must be called with vvs->tx_lock held. Returns >= 0. */ static s64 virtio_transport_tx_space(struct virtio_vsock_sock *vvs) { s64 bytes; bytes = (s64)vvs->peer_buf_alloc - ((s64)vvs->tx_cnt - (s64)vvs->peer_fwd_cnt); if (bytes < 0) bytes = 0; return bytes; } Then: get_credit() would do bytes = virtio_transport_tx_space(vvs); ret = min_t(u32, credit, (u32)bytes); has_space() would use the same helper after obtaining vvs = vsk->trans; Does that match what you had in mind, or would you prefer a different factoring? > >> + ((s64)vvs->tx_cnt - (s64)vvs->peer_fwd_cnt); >> + if (bytes < 0) >> + bytes = 0; >> + >> + ret = min_t(u32, credit, (u32)bytes); >>  vvs->tx_cnt += ret; >>  vvs->bytes_unsent += ret; >>  spin_unlock_bh(&vvs->tx_lock); >> >>  return ret; >> } >> >> >> --- >> >> Patch 2/4 — vsock/virtio: cap TX window by local buffer (helper + use >> everywhere in TX path) >> >> Cap the effective advertised window to min(peer_buf_alloc, buf_alloc) >> and use it consistently in TX paths (get_credit, has_space, >> seqpacket_enqueue). >> >> diff --git a/net/vmw_vsock/virtio_transport_common.c >> b/net/vmw_vsock/virtio_transport_common.c >> --- a/net/vmw_vsock/virtio_transport_common.c >> +++ b/net/vmw_vsock/virtio_transport_common.c >> @@ -491,6 +491,16 @@ void virtio_transport_consume_skb_sent(struct >> sk_buff *skb, bool consume) >> } >> EXPORT_SYMBOL_GPL(virtio_transport_consume_skb_sent); >> +/* Return the effective peer buffer size for TX credit computation. >> + * >> + * The peer advertises its receive buffer via peer_buf_alloc, but we >> cap it >> + * to our local buf_alloc (derived from SO_VM_SOCKETS_BUFFER_SIZE and >> + * already clamped to buffer_max_size). >> + */ >> +static u32 virtio_transport_tx_buf_alloc(struct virtio_vsock_sock *vvs) >> +{ >> + return min(vvs->peer_buf_alloc, vvs->buf_alloc); >> +} >> >> u32 virtio_transport_get_credit(struct virtio_vsock_sock *vvs, u32 >> credit) >> { >>  s64 bytes; >> @@ -502,7 +512,8 @@ u32 virtio_transport_get_credit(struct >> virtio_vsock_sock *vvs, u32 credit) >>  return 0; >> >>  spin_lock_bh(&vvs->tx_lock); >> - bytes = (s64)vvs->peer_buf_alloc - >> + bytes = (s64)virtio_transport_tx_buf_alloc(vvs) - >>  ((s64)vvs->tx_cnt - (s64)vvs->peer_fwd_cnt); >>  if (bytes < 0) >>  bytes = 0; >> @@ -834,7 +845,7 @@ virtio_transport_seqpacket_enqueue(struct >> vsock_sock *vsk, >>  spin_lock_bh(&vvs->tx_lock); >> >> - if (len > vvs->peer_buf_alloc) { >> + if (len > virtio_transport_tx_buf_alloc(vvs)) { >>  spin_unlock_bh(&vvs->tx_lock); >>  return -EMSGSIZE; >>  } >> @@ -884,7 +895,8 @@ static s64 virtio_transport_has_space(struct >> vsock_sock *vsk) >>  struct virtio_vsock_sock *vvs = vsk->trans; >>  s64 bytes; >> >> - bytes = (s64)vvs->peer_buf_alloc - (vvs->tx_cnt - vvs->peer_fwd_cnt); >> + bytes = (s64)virtio_transport_tx_buf_alloc(vvs) - >> + ((s64)vvs->tx_cnt - (s64)vvs->peer_fwd_cnt); >>  if (bytes < 0) >>  bytes = 0; >> >>  return bytes; >> } >> >> >> --- >> >> Patch 3/4 — vsock/test: fix seqpacket msg bounds test (set client buf >> too) > > Please just include in the series the patch I sent to you. > Thanks. I'll use your vsock_test.c patch as-is for 3/4 >> >> After fixing TX credit bounds, the client can fill its TX window and >> block before it wakes the server. Setting the buffer on the client >> makes the test deterministic again. >> >> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/ >> vsock_test.c >> --- a/tools/testing/vsock/vsock_test.c >> +++ b/tools/testing/vsock/vsock_test.c >> @@ -353,6 +353,7 @@ static void test_stream_msg_peek_server(const >> struct test_opts *opts) >> >> static void test_seqpacket_msg_bounds_client(const struct test_opts >> *opts) >> { >> + unsigned long long sock_buf_size; >>  unsigned long curr_hash; >>  size_t max_msg_size; >>  int page_size; >> @@ -366,6 +367,18 @@ static void >> test_seqpacket_msg_bounds_client(const struct test_opts *opts) >>  exit(EXIT_FAILURE); >>  } >> >> + sock_buf_size = SOCK_BUF_SIZE; >> + >> + setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE, >> +    sock_buf_size, >> +    "setsockopt(SO_VM_SOCKETS_BUFFER_MAX_SIZE)"); >> + >> + setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE, >> +    sock_buf_size, >> +    "setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)"); >> + >>  /* Wait, until receiver sets buffer size. */ >>  control_expectln("SRVREADY"); >> >> >> --- >> >> Patch 4/4 — vsock/test: add stream TX credit bounds regression test >> >> This directly guards the original failure mode for stream sockets: if >> the peer advertises a large window but the sender’s local policy is >> small, the sender must stall quickly (hit EAGAIN in nonblocking mode) >> rather than queueing megabytes. > > Yeah, using nonblocking mode LGTM! > >> >> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/ >> vsock_test.c >> --- a/tools/testing/vsock/vsock_test.c >> +++ b/tools/testing/vsock/vsock_test.c >> @@ -349,6 +349,7 @@ >> #define SOCK_BUF_SIZE (2 * 1024 * 1024) >> +#define SMALL_SOCK_BUF_SIZE (64 * 1024ULL) >> #define MAX_MSG_PAGES 4 >> >> /* Insert new test functions after test_stream_msg_peek_server, before >>  * test_seqpacket_msg_bounds_client (around line 352) */ >> >> +static void test_stream_tx_credit_bounds_client(const struct >> test_opts *opts) >> +{ >> + ... /* full function as provided */ >> +} >> + >> +static void test_stream_tx_credit_bounds_server(const struct >> test_opts *opts) >> +{ >> + ... /* full function as provided */ >> +} >> >> @@ -2224,6 +2305,10 @@ >>  .run_client = test_stream_msg_peek_client, >>  .run_server = test_stream_msg_peek_server, >>  }, >> + { >> + .name = "SOCK_STREAM TX credit bounds", >> + .run_client = test_stream_tx_credit_bounds_client, >> + .run_server = test_stream_tx_credit_bounds_server, >> + }, > > Please put it at the bottom. Tests are skipped by index, so we don't > want to change index of old tests. > > Please fix your editor, those diffs are hard to read without tabs/spaces. seems like some issue with my email client. Hope it is okay now > > Thanks, > Stefano >