The Linux Kernel Mailing List
 help / color / mirror / Atom feed
From: Stefano Garzarella <sgarzare@redhat.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	netdev@vger.kernel.org, "Eric Dumazet" <edumazet@google.com>,
	"Stefan Hajnoczi" <stefanha@redhat.com>,
	virtualization@lists.linux.dev,
	"David S. Miller" <davem@davemloft.net>,
	"Jason Wang" <jasowang@redhat.com>,
	"Simon Horman" <horms@kernel.org>,
	linux-kernel@vger.kernel.org, "Paolo Abeni" <pabeni@redhat.com>,
	"Xuan Zhuo" <xuanzhuo@linux.alibaba.com>,
	kvm@vger.kernel.org, "Eugenio Pérez" <eperezma@redhat.com>
Subject: Re: [PATCH net] vsock/virtio: fix skb overhead accounting to preserve full buf_alloc
Date: Tue, 12 May 2026 09:19:58 +0200	[thread overview]
Message-ID: <agLR7RkX_3kvL_qw@sgarzare-redhat> (raw)
In-Reply-To: <20260511164841.7f7ff557@kernel.org>

On Mon, May 11, 2026 at 04:48:41PM -0700, Jakub Kicinski wrote:
>On Mon, 11 May 2026 15:17:56 +0200 Stefano Garzarella wrote:
>> > > Okay, I thought it over over the weekend, and I agree that this patch
>> > > still doesn't solve the problem and would still result in packet loss.
>> > > So, until we resolve the issue permanently, and since 059b7dbd20a6 is
>> > > coming to stable, I'd like to rework this patch so that we only start
>> > > dropping packets when the overhead plus the queued bytes exceeds
>> > > `buf_alloc * 2`.
>> > > Removing the other changes to reduce the buf_alloc advertised, but
>> > > terminating the connection so that both peers are aware that something
>> > > went wrong.
>> > >
>> > > Any objections?
>> > >
>> > > Stefano
>> >
>> > Let's try to first fix it upstream properly please. Discuss backporting
>> > later.
>>
>> Commit 059b7dbd20a6 ("vsock/virtio: fix potential unbounded skb
>> queue") is already in Linus tree and will land soon on stable. Which
>> issue do you see on having a patch on top of that to close the
>> connection instead of losing data and breaking our test suite?
>>
>> IMO we need that change in any case, because the previous code also
>> discarded packets without any notification, whereas breaking the
>> connection would be better in that case.
>
>Sorry if I'm speaking out of turn or misunderstanding but, like Michael
>says, let's focus on fixing this the way we want it to be fixed?

IMHO, these are two separate issues (related but still separated):

1. taking overhead into account and setting a limit (which Eric started 
doing in 059b7dbd20a6)

2. reducing overhead by also merging SEQPACKET as Michael proposed (we 
already do this for STREAM).

Even with option 2, the overhead won’t disappear, and we should maintain 
a limit as Eric rightly pointed out, especially since the initial 
problem was a flood of EOMs with 0 or 1 bytes, which option 2 doesn’t 
solve, since the overhead will eventually explode.

So I agree that we should implement 2 as Michael proposed, but I think 
we also need to fix 1 by improving it slightly (as I’ve tried here, and 
would like to do in v2).

Since I have v2 ready and tested, I'll send it because I think we should 
have it in any case, even with option 2 implemented.

>IIUC you are trying to minimize the size of the fix, please don't worry
>about the LoC in the diff at this stage.
>

Okay, I'll try to work on it, but this week is a nightmare. I hope to 
come up with something by building on Michael's idea.

Thanks,
Stefano


      reply	other threads:[~2026-05-12  7:20 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-08  9:23 [PATCH net] vsock/virtio: fix skb overhead accounting to preserve full buf_alloc Stefano Garzarella
2026-05-08  9:53 ` Michael S. Tsirkin
2026-05-08 10:01   ` Stefano Garzarella
2026-05-08 10:33     ` Michael S. Tsirkin
2026-05-08 10:38       ` Stefano Garzarella
2026-05-11 10:54         ` Stefano Garzarella
2026-05-11 12:46           ` Michael S. Tsirkin
2026-05-11 13:17             ` Stefano Garzarella
2026-05-11 23:48               ` Jakub Kicinski
2026-05-12  7:19                 ` Stefano Garzarella [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=agLR7RkX_3kvL_qw@sgarzare-redhat \
    --to=sgarzare@redhat.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=eperezma@redhat.com \
    --cc=horms@kernel.org \
    --cc=jasowang@redhat.com \
    --cc=kuba@kernel.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mst@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=stefanha@redhat.com \
    --cc=virtualization@lists.linux.dev \
    --cc=xuanzhuo@linux.alibaba.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox