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(-)
Currently, the MORE-PACKETS rxrpc header flag is set by sendmsg trying to
guess how it should be set by looking to see if there's space in the Tx
window and setting it if there is - long before the packet gets
transmitted (and it gets left in this state). As a consequence, it's not
very meaningful.
Change this such that it is turned on at the point of transmission if we
have more packets after it in the send buffers and it is left clear if we
don't yet.
Fixes: 17926a79320a ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both")
cc: David Howells <[email protected]>
cc: Marc Dionne <[email protected]>
cc: "David S. Miller" <[email protected]>
cc: Eric Dumazet <[email protected]>
cc: Jakub Kicinski <[email protected]>
cc: Paolo Abeni <[email protected]>
cc: [email protected]
cc: [email protected]
---
net/rxrpc/output.c | 8 +++++++-
net/rxrpc/sendmsg.c | 3 ---
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/rxrpc/output.c b/net/rxrpc/output.c
index bf2d0f847cdb..4ebd0bd40a02 100644
--- a/net/rxrpc/output.c
+++ b/net/rxrpc/output.c
@@ -330,7 +330,7 @@ static void rxrpc_prepare_data_subpacket(struct rxrpc_call *call, struct rxrpc_t
struct rxrpc_wire_header *whdr = txb->kvec[0].iov_base;
enum rxrpc_req_ack_trace why;
struct rxrpc_connection *conn = call->conn;
- bool last;
+ bool more, last;
u8 flags;
_enter("%x,{%d}", txb->seq, txb->len);
@@ -345,6 +345,12 @@ static void rxrpc_prepare_data_subpacket(struct rxrpc_call *call, struct rxrpc_t
flags = txb->flags & RXRPC_TXBUF_WIRE_FLAGS;
last = txb->flags & RXRPC_LAST_PACKET;
+ more = (!last &&
+ (!list_is_last(&txb->call_link, &call->tx_buffer) ||
+ !list_empty(&call->tx_sendmsg)));
+ if (more)
+ flags |= RXRPC_MORE_PACKETS;
+
/* If our RTT cache needs working on, request an ACK. Also request
* ACKs if a DATA packet appears to have been lost.
*
diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index 894b8fa68e5e..eaf4441a340b 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -384,9 +384,6 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
(msg_data_left(msg) == 0 && !more)) {
if (msg_data_left(msg) == 0 && !more)
txb->flags |= RXRPC_LAST_PACKET;
- else if (call->tx_top - call->acks_hard_ack <
- call->tx_winsize)
- txb->flags |= RXRPC_MORE_PACKETS;
ret = call->security->secure_packet(call, txb);
if (ret < 0)
Make the following fixes to the congestion control algorithm:
(1) Don't vary the cwnd starting value by the size of RXRPC_TX_SMSS since
that's currently held constant - set to the size of a jumbo subpacket
payload so that we can create jumbo packets on the fly. The current
code invariably picks 3 as the starting value.
Further, the starting cwnd needs to be an even number because we ack
every other packet, so set it to 4.
(2) Don't cut ssthresh when we see an ACK come from the peer with a
receive window (rwind) less than ssthresh. ssthresh keeps track of
characteristics of the connection whereas rwind may be reduced by the
peer for any reason - and may be reduced to 0.
Fixes: 1fc4fa2ac93d ("rxrpc: Fix congestion management")
Fixes: 0851115090a3 ("rxrpc: Reduce ssthresh to peer's receive window")
Signed-off-by: David Howells <[email protected]>
Suggested-by: Simon Wilkinson <[email protected]>
cc: Marc Dionne <[email protected]>
cc: "David S. Miller" <[email protected]>
cc: Eric Dumazet <[email protected]>
cc: Jakub Kicinski <[email protected]>
cc: Paolo Abeni <[email protected]>
cc: [email protected]
cc: [email protected]
---
net/rxrpc/ar-internal.h | 2 +-
net/rxrpc/call_object.c | 7 +------
net/rxrpc/input.c | 3 ---
3 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 08c0a32db8c7..08de24658f4f 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -697,7 +697,7 @@ struct rxrpc_call {
* packets) rather than bytes.
*/
#define RXRPC_TX_SMSS RXRPC_JUMBO_DATALEN
-#define RXRPC_MIN_CWND (RXRPC_TX_SMSS > 2190 ? 2 : RXRPC_TX_SMSS > 1095 ? 3 : 4)
+#define RXRPC_MIN_CWND 4
u8 cong_cwnd; /* Congestion window size */
u8 cong_extra; /* Extra to send for congestion management */
u8 cong_ssthresh; /* Slow-start threshold */
diff --git a/net/rxrpc/call_object.c b/net/rxrpc/call_object.c
index 01fa71e8b1f7..f9e983a12c14 100644
--- a/net/rxrpc/call_object.c
+++ b/net/rxrpc/call_object.c
@@ -174,12 +174,7 @@ struct rxrpc_call *rxrpc_alloc_call(struct rxrpc_sock *rx, gfp_t gfp,
call->rx_winsize = rxrpc_rx_window_size;
call->tx_winsize = 16;
- if (RXRPC_TX_SMSS > 2190)
- call->cong_cwnd = 2;
- else if (RXRPC_TX_SMSS > 1095)
- call->cong_cwnd = 3;
- else
- call->cong_cwnd = 4;
+ call->cong_cwnd = RXRPC_MIN_CWND;
call->cong_ssthresh = RXRPC_TX_MAX_WINDOW;
call->rxnet = rxnet;
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index 3dedb8c0618c..860075f1290b 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -685,9 +685,6 @@ static void rxrpc_input_ack_trailer(struct rxrpc_call *call, struct sk_buff *skb
call->tx_winsize = rwind;
}
- if (call->cong_ssthresh > rwind)
- call->cong_ssthresh = rwind;
-
mtu = min(ntohl(trailer->maxMTU), ntohl(trailer->ifMTU));
peer = call->peer;
Only generate one ACK packet for all the subpackets in a jumbo packet. If
we would like to generate more than one ACK, we prioritise them base on
their reason code, in the order, highest first:
OutOfSeq > NoSpace > ExceedsWin > Duplicate > Requested > Delay > Idle
For the first four, we reference the lowest offending subpacket; for the
last three, the highest.
This reduces the number of ACKs we end up transmitting to one per UDP
packet transmitted to reduce network loading and packet parsing.
Fixes: 5d7edbc9231e ("rxrpc: Get rid of the Rx ring")
Signed-off-by: David Howells <[email protected]>
cc: Marc Dionne <[email protected]>
cc: "David S. Miller" <[email protected]>
cc: Eric Dumazet <[email protected]>
cc: Jakub Kicinski <[email protected]>
cc: Paolo Abeni <[email protected]>
cc: [email protected]
cc: [email protected]
---
net/rxrpc/input.c | 46 +++++++++++++++++++++++++++++++++++-----------
1 file changed, 35 insertions(+), 11 deletions(-)
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index 860075f1290b..16d49a861dbb 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -9,6 +9,17 @@
#include "ar-internal.h"
+/* Override priority when generating ACKs for received DATA */
+static const u8 rxrpc_ack_priority[RXRPC_ACK__INVALID] = {
+ [RXRPC_ACK_IDLE] = 1,
+ [RXRPC_ACK_DELAY] = 2,
+ [RXRPC_ACK_REQUESTED] = 3,
+ [RXRPC_ACK_DUPLICATE] = 4,
+ [RXRPC_ACK_EXCEEDS_WINDOW] = 5,
+ [RXRPC_ACK_NOSPACE] = 6,
+ [RXRPC_ACK_OUT_OF_SEQUENCE] = 7,
+};
+
static void rxrpc_proto_abort(struct rxrpc_call *call, rxrpc_seq_t seq,
enum rxrpc_abort_reason why)
{
@@ -365,7 +376,7 @@ static void rxrpc_input_queue_data(struct rxrpc_call *call, struct sk_buff *skb,
* Process a DATA packet.
*/
static void rxrpc_input_data_one(struct rxrpc_call *call, struct sk_buff *skb,
- bool *_notify)
+ bool *_notify, rxrpc_serial_t *_ack_serial, int *_ack_reason)
{
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
struct sk_buff *oos;
@@ -418,8 +429,6 @@ static void rxrpc_input_data_one(struct rxrpc_call *call, struct sk_buff *skb,
/* Send an immediate ACK if we fill in a hole */
else if (!skb_queue_empty(&call->rx_oos_queue))
ack_reason = RXRPC_ACK_DELAY;
- else
- call->ackr_nr_unacked++;
window++;
if (after(window, wtop)) {
@@ -497,12 +506,16 @@ static void rxrpc_input_data_one(struct rxrpc_call *call, struct sk_buff *skb,
}
send_ack:
- if (ack_reason >= 0)
- rxrpc_send_ACK(call, ack_reason, serial,
- rxrpc_propose_ack_input_data);
- else
- rxrpc_propose_delay_ACK(call, serial,
- rxrpc_propose_ack_input_data);
+ if (ack_reason >= 0) {
+ if (rxrpc_ack_priority[ack_reason] > rxrpc_ack_priority[*_ack_reason]) {
+ *_ack_serial = serial;
+ *_ack_reason = ack_reason;
+ } else if (rxrpc_ack_priority[ack_reason] == rxrpc_ack_priority[*_ack_reason] &&
+ ack_reason == RXRPC_ACK_REQUESTED) {
+ *_ack_serial = serial;
+ *_ack_reason = ack_reason;
+ }
+ }
}
/*
@@ -513,9 +526,11 @@ static bool rxrpc_input_split_jumbo(struct rxrpc_call *call, struct sk_buff *skb
struct rxrpc_jumbo_header jhdr;
struct rxrpc_skb_priv *sp = rxrpc_skb(skb), *jsp;
struct sk_buff *jskb;
+ rxrpc_serial_t ack_serial = 0;
unsigned int offset = sizeof(struct rxrpc_wire_header);
unsigned int len = skb->len - offset;
bool notify = false;
+ int ack_reason = 0;
while (sp->hdr.flags & RXRPC_JUMBO_PACKET) {
if (len < RXRPC_JUMBO_SUBPKTLEN)
@@ -535,7 +550,7 @@ static bool rxrpc_input_split_jumbo(struct rxrpc_call *call, struct sk_buff *skb
jsp = rxrpc_skb(jskb);
jsp->offset = offset;
jsp->len = RXRPC_JUMBO_DATALEN;
- rxrpc_input_data_one(call, jskb, ¬ify);
+ rxrpc_input_data_one(call, jskb, ¬ify, &ack_serial, &ack_reason);
rxrpc_free_skb(jskb, rxrpc_skb_put_jumbo_subpacket);
sp->hdr.flags = jhdr.flags;
@@ -548,7 +563,16 @@ static bool rxrpc_input_split_jumbo(struct rxrpc_call *call, struct sk_buff *skb
sp->offset = offset;
sp->len = len;
- rxrpc_input_data_one(call, skb, ¬ify);
+ rxrpc_input_data_one(call, skb, ¬ify, &ack_serial, &ack_reason);
+
+ if (ack_reason > 0) {
+ rxrpc_send_ACK(call, ack_reason, ack_serial,
+ rxrpc_propose_ack_input_data);
+ } else {
+ call->ackr_nr_unacked++;
+ rxrpc_propose_delay_ACK(call, sp->hdr.serial,
+ rxrpc_propose_ack_input_data);
+ }
if (notify) {
trace_rxrpc_notify_socket(call->debug_id, sp->hdr.serial);
rxrpc_notify_socket(call);
Clean up the generation of the header flags when building packet headers
for transmission:
(1) Assemble the flags in a local variable rather than in the txb->flags.
(2) Do the flags masking and JUMBO-PACKET setting in one bit of code for
both the main header and the jumbo headers.
(3) Generate the REQUEST-ACK flag afresh each time. There's a possibility
we might want to do jumbo retransmission packets in future.
(4) Pass the local flags variable to the rxrpc_tx_data tracepoint rather
than the combination of the txb flags and the wire header flags (the
latter belong only to the first subpacket).
This makes it easier to clean up setting the MORE-PACKETS flag
Fixes: 44125d5aadda ("rxrpc: Split up the DATA packet transmission function")
Signed-off-by: David Howells <[email protected]>
cc: Marc Dionne <[email protected]>
cc: "David S. Miller" <[email protected]>
cc: Eric Dumazet <[email protected]>
cc: Jakub Kicinski <[email protected]>
cc: Paolo Abeni <[email protected]>
cc: [email protected]
cc: [email protected]
---
include/trace/events/rxrpc.h | 1 -
net/rxrpc/ar-internal.h | 2 +-
net/rxrpc/output.c | 18 ++++++++++++------
net/rxrpc/proc.c | 3 +--
4 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/include/trace/events/rxrpc.h b/include/trace/events/rxrpc.h
index a1b126a6b0d7..7b6c1db53401 100644
--- a/include/trace/events/rxrpc.h
+++ b/include/trace/events/rxrpc.h
@@ -449,7 +449,6 @@
#define rxrpc_req_ack_traces \
EM(rxrpc_reqack_ack_lost, "ACK-LOST ") \
- EM(rxrpc_reqack_already_on, "ALREADY-ON") \
EM(rxrpc_reqack_more_rtt, "MORE-RTT ") \
EM(rxrpc_reqack_no_srv_last, "NO-SRVLAST") \
EM(rxrpc_reqack_old_rtt, "OLD-RTT ") \
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 08de24658f4f..c11a6043c8f2 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -110,7 +110,7 @@ struct rxrpc_net {
atomic_t stat_tx_acks[256];
atomic_t stat_rx_acks[256];
- atomic_t stat_why_req_ack[8];
+ atomic_t stat_why_req_ack[7];
atomic_t stat_io_loop;
};
diff --git a/net/rxrpc/output.c b/net/rxrpc/output.c
index 5ea9601efd05..bf2d0f847cdb 100644
--- a/net/rxrpc/output.c
+++ b/net/rxrpc/output.c
@@ -330,6 +330,8 @@ static void rxrpc_prepare_data_subpacket(struct rxrpc_call *call, struct rxrpc_t
struct rxrpc_wire_header *whdr = txb->kvec[0].iov_base;
enum rxrpc_req_ack_trace why;
struct rxrpc_connection *conn = call->conn;
+ bool last;
+ u8 flags;
_enter("%x,{%d}", txb->seq, txb->len);
@@ -339,6 +341,10 @@ static void rxrpc_prepare_data_subpacket(struct rxrpc_call *call, struct rxrpc_t
txb->seq == 1)
whdr->userStatus = RXRPC_USERSTATUS_SERVICE_UPGRADE;
+ txb->flags &= ~RXRPC_REQUEST_ACK;
+ flags = txb->flags & RXRPC_TXBUF_WIRE_FLAGS;
+ last = txb->flags & RXRPC_LAST_PACKET;
+
/* If our RTT cache needs working on, request an ACK. Also request
* ACKs if a DATA packet appears to have been lost.
*
@@ -346,9 +352,7 @@ static void rxrpc_prepare_data_subpacket(struct rxrpc_call *call, struct rxrpc_t
* service call, lest OpenAFS incorrectly send us an ACK with some
* soft-ACKs in it and then never follow up with a proper hard ACK.
*/
- if (txb->flags & RXRPC_REQUEST_ACK)
- why = rxrpc_reqack_already_on;
- else if ((txb->flags & RXRPC_LAST_PACKET) && rxrpc_sending_to_client(txb))
+ if (last && rxrpc_sending_to_client(txb))
why = rxrpc_reqack_no_srv_last;
else if (test_and_clear_bit(RXRPC_CALL_EV_ACK_LOST, &call->events))
why = rxrpc_reqack_ack_lost;
@@ -367,15 +371,17 @@ static void rxrpc_prepare_data_subpacket(struct rxrpc_call *call, struct rxrpc_t
rxrpc_inc_stat(call->rxnet, stat_why_req_ack[why]);
trace_rxrpc_req_ack(call->debug_id, txb->seq, why);
- if (why != rxrpc_reqack_no_srv_last)
+ if (why != rxrpc_reqack_no_srv_last) {
txb->flags |= RXRPC_REQUEST_ACK;
+ flags |= RXRPC_REQUEST_ACK;
+ }
dont_set_request_ack:
- whdr->flags = txb->flags & RXRPC_TXBUF_WIRE_FLAGS;
+ whdr->flags = flags;
whdr->serial = htonl(txb->serial);
whdr->cksum = txb->cksum;
- trace_rxrpc_tx_data(call, txb->seq, txb->serial, txb->flags, false);
+ trace_rxrpc_tx_data(call, txb->seq, txb->serial, flags, false);
}
/*
diff --git a/net/rxrpc/proc.c b/net/rxrpc/proc.c
index 263a2251e3d2..3b7e34dd4385 100644
--- a/net/rxrpc/proc.c
+++ b/net/rxrpc/proc.c
@@ -519,9 +519,8 @@ int rxrpc_stats_show(struct seq_file *seq, void *v)
atomic_read(&rxnet->stat_rx_acks[RXRPC_ACK_DELAY]),
atomic_read(&rxnet->stat_rx_acks[RXRPC_ACK_IDLE]));
seq_printf(seq,
- "Why-Req-A: acklost=%u already=%u mrtt=%u ortt=%u\n",
+ "Why-Req-A: acklost=%u mrtt=%u ortt=%u\n",
atomic_read(&rxnet->stat_why_req_ack[rxrpc_reqack_ack_lost]),
- atomic_read(&rxnet->stat_why_req_ack[rxrpc_reqack_already_on]),
atomic_read(&rxnet->stat_why_req_ack[rxrpc_reqack_more_rtt]),
atomic_read(&rxnet->stat_why_req_ack[rxrpc_reqack_old_rtt]));
seq_printf(seq,
Set the REQUEST-ACK flag on the DATA packet we're about to send if we're
about to stall transmission because the app layer isn't keeping up
supplying us with data to transmit.
Fixes: 17926a79320a ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both")
Signed-off-by: David Howells <[email protected]>
cc: Marc Dionne <[email protected]>
cc: "David S. Miller" <[email protected]>
cc: Eric Dumazet <[email protected]>
cc: Jakub Kicinski <[email protected]>
cc: Paolo Abeni <[email protected]>
cc: [email protected]
cc: [email protected]
---
include/trace/events/rxrpc.h | 1 +
net/rxrpc/ar-internal.h | 2 +-
net/rxrpc/output.c | 2 ++
net/rxrpc/proc.c | 5 +++--
4 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/include/trace/events/rxrpc.h b/include/trace/events/rxrpc.h
index 7b6c1db53401..dca9f4759dcb 100644
--- a/include/trace/events/rxrpc.h
+++ b/include/trace/events/rxrpc.h
@@ -449,6 +449,7 @@
#define rxrpc_req_ack_traces \
EM(rxrpc_reqack_ack_lost, "ACK-LOST ") \
+ EM(rxrpc_reqack_app_stall, "APP-STALL ") \
EM(rxrpc_reqack_more_rtt, "MORE-RTT ") \
EM(rxrpc_reqack_no_srv_last, "NO-SRVLAST") \
EM(rxrpc_reqack_old_rtt, "OLD-RTT ") \
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index c11a6043c8f2..08de24658f4f 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -110,7 +110,7 @@ struct rxrpc_net {
atomic_t stat_tx_acks[256];
atomic_t stat_rx_acks[256];
- atomic_t stat_why_req_ack[7];
+ atomic_t stat_why_req_ack[8];
atomic_t stat_io_loop;
};
diff --git a/net/rxrpc/output.c b/net/rxrpc/output.c
index 4ebd0bd40a02..32626ff377e1 100644
--- a/net/rxrpc/output.c
+++ b/net/rxrpc/output.c
@@ -372,6 +372,8 @@ static void rxrpc_prepare_data_subpacket(struct rxrpc_call *call, struct rxrpc_t
why = rxrpc_reqack_more_rtt;
else if (ktime_before(ktime_add_ms(call->peer->rtt_last_req, 1000), ktime_get_real()))
why = rxrpc_reqack_old_rtt;
+ else if (!more && !last)
+ why = rxrpc_reqack_app_stall;
else
goto dont_set_request_ack;
diff --git a/net/rxrpc/proc.c b/net/rxrpc/proc.c
index 3b7e34dd4385..1bab7f5a7d0f 100644
--- a/net/rxrpc/proc.c
+++ b/net/rxrpc/proc.c
@@ -519,10 +519,11 @@ int rxrpc_stats_show(struct seq_file *seq, void *v)
atomic_read(&rxnet->stat_rx_acks[RXRPC_ACK_DELAY]),
atomic_read(&rxnet->stat_rx_acks[RXRPC_ACK_IDLE]));
seq_printf(seq,
- "Why-Req-A: acklost=%u mrtt=%u ortt=%u\n",
+ "Why-Req-A: acklost=%u mrtt=%u ortt=%u stall=%u\n",
atomic_read(&rxnet->stat_why_req_ack[rxrpc_reqack_ack_lost]),
atomic_read(&rxnet->stat_why_req_ack[rxrpc_reqack_more_rtt]),
- atomic_read(&rxnet->stat_why_req_ack[rxrpc_reqack_old_rtt]));
+ atomic_read(&rxnet->stat_why_req_ack[rxrpc_reqack_old_rtt]),
+ atomic_read(&rxnet->stat_why_req_ack[rxrpc_reqack_app_stall]));
seq_printf(seq,
"Why-Req-A: nolast=%u retx=%u slows=%u smtxw=%u\n",
atomic_read(&rxnet->stat_why_req_ack[rxrpc_reqack_no_srv_last]),
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 <[email protected]> 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 <[email protected] <mailto:[email protected]>>
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 <[email protected] <mailto:[email protected]>>
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 <[email protected]> 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 <[email protected]> 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 <[email protected]>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html