On Thu, Mar 26, 2020 at 11:22 AM Marcelo Ricardo Leitner
<marcelo.leitner@xxxxxxxxx> wrote:
>
> On Thu, Mar 26, 2020 at 09:30:08AM +0800, Qiujun Huang wrote:
> > On Thu, Mar 26, 2020 at 8:14 AM Marcelo Ricardo Leitner
> > <marcelo.leitner@xxxxxxxxx> wrote:
> > >
> > > On Sun, Mar 22, 2020 at 05:04:25PM +0800, Qiujun Huang wrote:
> > > > sctp_sock_migrate should iterate over the datamsgs to modify
> > > > all trunks(skbs) to newsk. For this, out_msg_list is added to
> > >
> > > s/trunks/chunks/
> >
> > My :p.
> >
> > >
> > > > 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.
>
> >
> > > 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 queue
>
> We always can find it through the other chunks, otherwise it's freed.
>
> >
> > 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!
>
> > 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.
