public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: Stefano Brivio <sbrivio@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: Matej Hrica <mhrica@redhat.com>, passt-dev@passt.top
Subject: Re: [PATCH RFT 2/5] tcp: Reset STALLED flag on ACK only, check for pending socket data
Date: Wed, 27 Sep 2023 19:05:33 +0200	[thread overview]
Message-ID: <20230927190533.2fc53bbf@elisabeth> (raw)
In-Reply-To: <ZRD5bMw8Ad1/h+JA@zatzit>

On Mon, 25 Sep 2023 13:07:24 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> I think the change itself here is sound, but I have some nits to pick
> with the description and reasoning.
> 
> On Sat, Sep 23, 2023 at 12:06:07AM +0200, Stefano Brivio wrote:
> > In tcp_tap_handler(), we shouldn't reset the STALLED flag (indicating
> > that we ran out of tap-side window space, or that all available
> > socket data is already in flight -- better names welcome!  
> 
> Hmm.. when you put it like that it makes me wonder if those two quite
> different conditions really need the same handling.  Hrm.. I guess
> both conditions mean that we can't accept data from the socket, even
> if it's availble.

Right. I mean, we can also call them differently... or maybe pick a
name that reflects the outcome/handling instead of what happened.

> > ) on any
> > event: do that only if the first packet in a batch has the ACK flag
> > set.  
> 
> "First packet in a batch" may not be accurate here - we're looking at
> whichever packet we were up to before calling data_from_tap().  There
> could have been earlier packets in the receive batch that were already
> processed.

Well, it depends on what we call "batch" -- here I meant the pool of
packets (that are passed as a batch to tcp_tap_handler()). Yes, "pool"
would be more accurate.

> This also raises the question of why the first data packet should be
> particularly privileged here.

No reason other than convenience, and yes, it can be subtly wrong.

> I'm wondering if what we really want to
> check is whether data_from_tap() advanced the ack pointer at all.

Right, we probably should do that instead.

> I'm not clear on when the th->ack check would ever fail in practice:
> aren't the only normal packets in a TCP connection without ACK the
> initial SYN or an RST?  We've handled the SYN case earlier, so should
> we just have a blanket case above this that if we get a packet with
> !ACK, we reset the connection?

One thing that's legitimate (rarely seen, but I've seen it, I don't
remember if the Linux kernel ever does that) is a segment without ACK,
and without data, that just updates the window (for example after a
zero window).

If the sequence received/processed so far doesn't correspond to the
latest sequence sent, omitting the ACK flag is useful so that the
window update is not taken as duplicate ACK (that would trigger
retransmission).

> > Make sure we check for pending socket data when we reset it:
> > reverting back to level-triggered epoll events, as tcp_epoll_ctl()
> > does, isn't guaranteed to actually trigger a socket event.  
> 
> Which sure seems like a kernel bug.  Some weird edge conditions for
> edge-triggered seems expected, but this doesn't seem like valid
> level-triggered semantics.

Hmm, yes, and by doing a quick isolated test actually this seems to work
as intended in the kernel. I should drop this and try again.

> Hmmm... is toggling EPOLLET even what we want.  IIUC, the heart of
> what's going on here is that we can't take more data from the socket
> until something happens on the tap side (either the window expands, or
> it acks some data).  In which case should we be toggling EPOLLIN on
> the socket instead?  That seems more explicitly to be saying to the
> socket side "we don't currently care if you have data available".

The reason was to act on EPOLLRDHUP at the same time. But well, we
could just mask EPOLLIN and EPOLLRDHUP, then -- I guess that would make
more sense.

For the moment being, we should probably try to see what happens
if this patch simply moves conn_flag(c, conn, ~STALLED); to where 3/5
needs it (or directly incorporate that into 3/5).

-- 
Stefano


  reply	other threads:[~2023-09-27 17:05 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-22 22:06 [PATCH RFT 0/5] Fixes and a workaround for TCP stalls with small buffers Stefano Brivio
2023-09-22 22:06 ` [PATCH RFT 1/5] tcp: Fix comment to tcp_sock_consume() Stefano Brivio
2023-09-23  2:48   ` David Gibson
2023-09-22 22:06 ` [PATCH RFT 2/5] tcp: Reset STALLED flag on ACK only, check for pending socket data Stefano Brivio
2023-09-25  3:07   ` David Gibson
2023-09-27 17:05     ` Stefano Brivio [this message]
2023-09-28  1:48       ` David Gibson
2023-09-29 15:20         ` Stefano Brivio
2023-10-03  3:20           ` David Gibson
2023-10-05  6:18             ` Stefano Brivio
2023-10-05  7:36               ` David Gibson
2023-09-22 22:06 ` [PATCH RFT 3/5] tcp: Force TCP_WINDOW_CLAMP before resetting STALLED flag Stefano Brivio
2023-09-22 22:31   ` Stefano Brivio
2023-09-23  7:55   ` David Gibson
2023-09-25  4:09   ` David Gibson
2023-09-25  4:10     ` David Gibson
2023-09-25  4:21     ` David Gibson
2023-09-27 17:05       ` Stefano Brivio
2023-09-28  1:51         ` David Gibson
2023-09-22 22:06 ` [PATCH RFT 4/5] tcp, tap: Don't increase tap-side sequence counter for dropped frames Stefano Brivio
2023-09-25  4:47   ` David Gibson
2023-09-27 17:06     ` Stefano Brivio
2023-09-28  1:58       ` David Gibson
2023-09-29 15:19         ` Stefano Brivio
2023-10-03  3:22           ` David Gibson
2023-10-05  6:19             ` Stefano Brivio
2023-10-05  7:38               ` David Gibson
2023-09-22 22:06 ` [PATCH RFT 5/5] passt.1: Add note about tuning rmem_max and wmem_max for throughput Stefano Brivio
2023-09-25  4:57   ` David Gibson
2023-09-27 17:06     ` Stefano Brivio
2023-09-28  2:02       ` David Gibson
2023-09-25  5:52 ` [PATCH RFT 0/5] Fixes and a workaround for TCP stalls with small buffers David Gibson

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=20230927190533.2fc53bbf@elisabeth \
    --to=sbrivio@redhat.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=mhrica@redhat.com \
    --cc=passt-dev@passt.top \
    /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.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).