| Message ID | 20240503150749.1001323-1-dhowells@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | rxrpc: Miscellaneous fixes | expand |
On Fri, 3 May 2024 16:07:38 +0100 David Howells wrote: > Here some miscellaneous fixes for AF_RXRPC: > > (1) Fix the congestion control algorithm to start cwnd at 4 and to not cut > ssthresh when the peer cuts its rwind size. > > (2) Only transmit a single ACK for all the DATA packets glued together > into a jumbo packet to reduce the number of ACKs being generated. > > (3) Clean up the generation of flags in the protocol header when creating > a packet for transmission. This means we don't carry the old > REQUEST-ACK bit around from previous transmissions, will make it > easier to fix the MORE-PACKETS flag and make it easier to do jumbo > packet assembly in future. > > (4) Fix how the MORE-PACKETS flag is driven. We shouldn't be setting it > in sendmsg() as the packet is then queued and the bit is left in that > state, no matter how long it takes us to transmit the packet - and > will still be in that state if the packet is retransmitted. > > (5) Request an ACK on an impending transmission stall due to the app layer > not feeding us new data fast enough. If we don't request an ACK, we > may have to hold on to the packet buffers for a significant amount of > time until the receiver gets bored and sends us an ACK anyway. Looks like these got marked as Rejected in patchwork. I think either because lore is confused and attaches an exchange with DaveM from 2022 to them (?) or because I mentioned to DaveM that I'm not sure these are fixes. So let me ask - on a scale of 1 to 10, how convinced are you that these should go to Linus this week rather than being categorized as general improvements and go during the merge window (without the Fixes tags)?
> On May 7, 2024, at 8:44 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 3 May 2024 16:07:38 +0100 David Howells wrote: >> Here some miscellaneous fixes for AF_RXRPC: >> >> (1) Fix the congestion control algorithm to start cwnd at 4 and to not cut >> ssthresh when the peer cuts its rwind size. >> >> (2) Only transmit a single ACK for all the DATA packets glued together >> into a jumbo packet to reduce the number of ACKs being generated. >> >> (3) Clean up the generation of flags in the protocol header when creating >> a packet for transmission. This means we don't carry the old >> REQUEST-ACK bit around from previous transmissions, will make it >> easier to fix the MORE-PACKETS flag and make it easier to do jumbo >> packet assembly in future. >> >> (4) Fix how the MORE-PACKETS flag is driven. We shouldn't be setting it >> in sendmsg() as the packet is then queued and the bit is left in that >> state, no matter how long it takes us to transmit the packet - and >> will still be in that state if the packet is retransmitted. >> >> (5) Request an ACK on an impending transmission stall due to the app layer >> not feeding us new data fast enough. If we don't request an ACK, we >> may have to hold on to the packet buffers for a significant amount of >> time until the receiver gets bored and sends us an ACK anyway. > > Looks like these got marked as Rejected in patchwork. > I think either because lore is confused and attaches an exchange with > DaveM from 2022 to them (?) or because I mentioned to DaveM that I'm > not sure these are fixes. So let me ask - on a scale of 1 to 10, how > convinced are you that these should go to Linus this week rather than > being categorized as general improvements and go during the merge > window (without the Fixes tags)? Jakub, In my opinion, the first two patches in the series I believe are important to back port to the stable branches. Reviewed-by: Jeffrey Altman <jaltman@auristor.com <mailto:jaltman@auristor.com>> Jeffrey
On Wed, 8 May 2024 01:57:43 -0600 Jeffrey Altman wrote: > > Looks like these got marked as Rejected in patchwork. > > I think either because lore is confused and attaches an exchange with > > DaveM from 2022 to them (?) or because I mentioned to DaveM that I'm > > not sure these are fixes. So let me ask - on a scale of 1 to 10, how > > convinced are you that these should go to Linus this week rather than > > being categorized as general improvements and go during the merge > > window (without the Fixes tags)? > > Jakub, > > In my opinion, the first two patches in the series I believe are important to back port to the stable branches. > > Reviewed-by: Jeffrey Altman <jaltman@auristor.com <mailto:jaltman@auristor.com>> Are they regressions? Seems possible from the Fixes tag but unclear from the text of the commit messages. In any case, taking the first two may be a reasonable compromise. Does it sounds good to you, David?
Jakub Kicinski <kuba@kernel.org> wrote: > Looks like these got marked as Rejected in patchwork. > I think either because lore is confused and attaches an exchange with > DaveM from 2022 to them (?) or because I mentioned to DaveM that I'm > not sure these are fixes. So let me ask - on a scale of 1 to 10, how > convinced are you that these should go to Linus this week rather than > being categorized as general improvements and go during the merge > window (without the Fixes tags)? Ah, sorry. I marked them rejected as I put myself as cc: not S-o-b on one of them, but then got distracted and didn't get around to reposting them. And Jeff mentioned that the use of the MORE-PACKETS flag is not exactly consistent between various implementations. So if you could take just the first two for the moment? Thanks, David
On Wed, 08 May 2024 15:00:28 +0100 David Howells wrote: > Jakub Kicinski <kuba@kernel.org> wrote: > > > Looks like these got marked as Rejected in patchwork. > > I think either because lore is confused and attaches an exchange with > > DaveM from 2022 to them (?) or because I mentioned to DaveM that I'm > > not sure these are fixes. So let me ask - on a scale of 1 to 10, how > > convinced are you that these should go to Linus this week rather than > > being categorized as general improvements and go during the merge > > window (without the Fixes tags)? > > Ah, sorry. I marked them rejected as I put myself as cc: not S-o-b on one of > them, but then got distracted and didn't get around to reposting them. And > Jeff mentioned that the use of the MORE-PACKETS flag is not exactly > consistent between various implementations. Ah, mystery solved :) > So if you could take just the first two for the moment? Done!
Hello: This series was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Fri, 3 May 2024 16:07:38 +0100 you wrote: > Here some miscellaneous fixes for AF_RXRPC: > > (1) Fix the congestion control algorithm to start cwnd at 4 and to not cut > ssthresh when the peer cuts its rwind size. > > (2) Only transmit a single ACK for all the DATA packets glued together > into a jumbo packet to reduce the number of ACKs being generated. > > [...] Here is the summary with links: - [net,1/5] rxrpc: Fix congestion control algorithm https://git.kernel.org/netdev/net/c/ba4e103848d3 - [net,2/5] rxrpc: Only transmit one ACK per jumbo packet received https://git.kernel.org/netdev/net/c/012b7206918d - [net,3/5] rxrpc: Clean up Tx header flags generation handling (no matching commit) - [net,4/5] rxrpc: Change how the MORE-PACKETS rxrpc wire header flag is driven (no matching commit) - [net,5/5] rxrpc: Request an ACK on impending Tx stall (no matching commit) You are awesome, thank you!
Here some miscellaneous fixes for AF_RXRPC: (1) Fix the congestion control algorithm to start cwnd at 4 and to not cut ssthresh when the peer cuts its rwind size. (2) Only transmit a single ACK for all the DATA packets glued together into a jumbo packet to reduce the number of ACKs being generated. (3) Clean up the generation of flags in the protocol header when creating a packet for transmission. This means we don't carry the old REQUEST-ACK bit around from previous transmissions, will make it easier to fix the MORE-PACKETS flag and make it easier to do jumbo packet assembly in future. (4) Fix how the MORE-PACKETS flag is driven. We shouldn't be setting it in sendmsg() as the packet is then queued and the bit is left in that state, no matter how long it takes us to transmit the packet - and will still be in that state if the packet is retransmitted. (5) Request an ACK on an impending transmission stall due to the app layer not feeding us new data fast enough. If we don't request an ACK, we may have to hold on to the packet buffers for a significant amount of time until the receiver gets bored and sends us an ACK anyway. David --- The patches can be found here also: http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes David Howells (5): rxrpc: Fix congestion control algorithm rxrpc: Only transmit one ACK per jumbo packet received rxrpc: Clean up Tx header flags generation handling rxrpc: Change how the MORE-PACKETS rxrpc wire header flag is driven rxrpc: Request an ACK on impending Tx stall include/trace/events/rxrpc.h | 2 +- net/rxrpc/ar-internal.h | 2 +- net/rxrpc/call_object.c | 7 +----- net/rxrpc/input.c | 49 +++++++++++++++++++++++++----------- net/rxrpc/output.c | 26 ++++++++++++++----- net/rxrpc/proc.c | 6 ++--- net/rxrpc/sendmsg.c | 3 --- 7 files changed, 61 insertions(+), 34 deletions(-)