Re: [PATCH v4] sctp: fix refcount bug in sctp_wfree
From: Qiujun Huang <hidden>
Date: 2020-03-26 06:13:42
Also in:
linux-sctp, lkml
On Thu, Mar 26, 2020 at 11:22 AM Marcelo Ricardo Leitner [off-list ref] wrote:
On Thu, Mar 26, 2020 at 09:30:08AM +0800, Qiujun Huang wrote:quoted
On Thu, Mar 26, 2020 at 8:14 AM Marcelo Ricardo Leitner [off-list ref] wrote:quoted
On Sun, Mar 22, 2020 at 05:04:25PM +0800, Qiujun Huang wrote:quoted
sctp_sock_migrate should iterate over the datamsgs to modify all trunks(skbs) to newsk. For this, out_msg_list is added tos/trunks/chunks/My :p.quoted
quoted
sctp_outq to maintain datamsgs list.It is an interesting approach. It speeds up the migration, yes, but it will also use more memory per datamsg, for an operation that, when performed, the socket is usually calm. It's also another list to be handled, and I'm not seeing the patch here move the datamsg itself now to the new outq. It would need something along these lines:Are all the rx chunks in the rx queues?Yes, even with GSO.quoted
quoted
sctp_sock_migrate() { ... /* Move any messages in the old socket's receive queue that are for the * peeled off association to the new socket's receive queue. */ sctp_skb_for_each(skb, &oldsk->sk_receive_queue, tmp) { event = sctp_skb2event(skb); ... /* Walk through the pd_lobby, looking for skbs that * need moved to the new socket. */ sctp_skb_for_each(skb, &oldsp->pd_lobby, tmp) { event = sctp_skb2event(skb); That said, I don't think it's worth this new list.About this case: datamsg ->chunk0 chunk1 chunk2 queue ->transmitted ->retransmit ->not in any queueWe always can find it through the other chunks, otherwise it's freed.quoted
Also need to maintain a datamsg list to record which datamsg is processed avoiding repetitive processing.Right, but for that we can add a simple check on sctp_for_each_tx_datamsg() based on a parameter.
Great! I get it, thanks!
quoted hunk ↗ jump to hunk
quoted
So, list it to outq. Maybe it will be used sometime.We can change it when the time comes. For now, if we can avoid growing sctp_datamsg, it's better. With this patch, it grows from 40 to 56 bytes, leaving just 8 left before it starts using a slab of 128 bytes for it. The patched list_for_each_entry() can/should be factored out into __sctp_for_each_tx_datachunk, whose first parameter then is the queue instead the asoc. ---8<---diff --git a/net/sctp/socket.c b/net/sctp/socket.c index fed26a1e9518..62f401799709 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c@@ -148,19 +148,30 @@ static void sctp_clear_owner_w(struct sctp_chunk *chunk) } static void sctp_for_each_tx_datachunk(struct sctp_association *asoc, + bool clear, void (*cb)(struct sctp_chunk *)) { + struct sctp_datamsg *msg, *prev_msg = NULL; struct sctp_outq *q = &asoc->outqueue; + struct sctp_chunk *chunk, *c; struct sctp_transport *t; - struct sctp_chunk *chunk; list_for_each_entry(t, &asoc->peer.transport_addr_list, transports) list_for_each_entry(chunk, &t->transmitted, transmitted_list) cb(chunk); - list_for_each_entry(chunk, &q->retransmit, transmitted_list) - cb(chunk); + list_for_each_entry(chunk, &q->sacked, transmitted_list) { + msg = chunk->msg; + if (msg == prev_msg) + continue; + list_for_each_entry(c, &msg->chunks, frag_list) { + if ((clear && asoc->base.sk == c->skb->sk) || + (!clear && asoc->base.sk != c->skb->sk)) + cb(c); + } + prev_msg = msg; + } list_for_each_entry(chunk, &q->sacked, transmitted_list) cb(chunk);@@ -9574,9 +9585,9 @@ static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, * paths won't try to lock it and then oldsk. */ lock_sock_nested(newsk, SINGLE_DEPTH_NESTING); - sctp_for_each_tx_datachunk(assoc, sctp_clear_owner_w); + sctp_for_each_tx_datachunk(assoc, true, sctp_clear_owner_w); sctp_assoc_migrate(assoc, newsk); - sctp_for_each_tx_datachunk(assoc, sctp_set_owner_w); + sctp_for_each_tx_datachunk(assoc, false, sctp_set_owner_w); /* If the association on the newsk is already closed before accept() * is called, set RCV_SHUTDOWN flag.