Re: [MPTCP] [PATCH 1/2] skbuff: Add shared control buffer

[Christoph Paasch <cpaasch at apple.com>]

[-- Attachment #1: Type: text/plain, Size: 4041 bytes --]

On 07/11/17 - 09:13:21, Rao Shoaib wrote:
> 
> 
> On 11/06/2017 08:09 PM, Christoph Paasch wrote:
> > 
> > Maybe to clarify, I meant TCP-level retransmissions (e.g., due to 3
> > duplicate acks). Not MPTCP-level retransmissions that are triggered through
> > the meta-level retransmission timer.
> > 
> > TCP-level retransmissions don't go through mptcp_skb_entail().
> Actually they do. When TCP timeout occurs tcp_write_timer_handler() is
> called
> 
>         case ICSK_TIME_RETRANS:
>                 icsk->icsk_pending = 0;
>                 tcp_sk(sk)->ops->retransmit_timer(sk);
>                 break;
> 
> retransmit_timer() for sub sockets is initialized to
> mptcp_sub_retransmit_timer() which will call mptcp_reinject() except in the
> fast open case (something that I need to look at). From what I have seen
> there is no case where a packet is [re]transmitted without going  through
> mptcp_skb_entail() or else dss will not be updated and the current code will
> also not work. Anyways, I will try to find some time and test with some
> packet loss.

These are meta-level retransmissions that are being sent on a different
subflow and/or on the same subflow but with new TCP sequence numbers and a
new DSS-mapping. These indeed end up going through mptcp_skb_entail().

The retransmissions I mean are the TCP-level retransmissions (aka.,
fast-retransmits, tail-loss-probe, RTO,...). They don't go through
mptcp_skb_entail again.

I will take a look at the trace in the other mail.


Christoph


> 
> If we do find any corner cases I prefer fixing them without exploding the
> size of skb.
> 
> Shoaib
> > 
> > > Perhaps I should provide you a patch that you can apply and play with. If
> > > there are any corner case issues,  I think they can be resolved in the
> > > retransmission code etc without requiring any change to the size of skb. Is
> > > providing a patch for the latest and greatest MPTCP good enough ?
> > Yes, patch would be great! Based on either mptcp_trunk or mptcp_v0.93.
> > 
> > I actually, would love to see mptcp_trunk no more bump up sk_buff->cb to 80
> > bytes. So, you can post it also on the mptcp-dev mailing-list if you think
> > it is all fine. Make sure to test it with packet-loss, because that's where
> > I feel is the culprit here.
> > 
> > 
> > Christoph
> > 
> > > Shoaib
> > > 
> > > > Christoph
> > > > 
> > > > > 15c16
> > > > > <     if (skb->mptcp_flags & MPTCPHDR_INF)
> > > > > ---
> > > > > >       if (tcb->mptcp_flags & MPTCPHDR_INF)
> > > > > 17c18
> > > > > <     else {
> > > > > ---
> > > > > >       else
> > > > > 19,22d19
> > > > > <         /* mptcp_entail_skb adds one for FIN */
> > > > > <         if (tcb->tcp_flags & TCPHDR_FIN)
> > > > > <             data_len -= 1;
> > > > > <     }
> > > > > 41c38
> > > > > <     return mptcp_dss_len/sizeof(*ptr);
> > > > > ---
> > > > > >       return ptr - start;
> > > > > 44,45c41,42
> > > > > < static int mptcp_write_dss_data_ack(const struct tcp_sock *tp,
> > > > > <     const struct sk_buff *skb, __be32 *ptr)
> > > > > ---
> > > > > > static int mptcp_write_dss_data_ack(const struct tcp_sock *tp, const
> > > > > struct sk_buff *skb,
> > > > > >                       __be32 *ptr)
> > > > > 62d58
> > > > > <     /* data_ack */
> > > > > 
> > > > > And mptcp_options_write() is now:
> > > > > 
> > > > >           if (OPTION_DATA_ACK & opts->mptcp_options) {
> > > > >                   ptr += mptcp_write_dss_data_ack(tp, skb, ptr);
> > > > >                   if (mptcp_is_data_seq(skb)) {
> > > > >                           ptr += mptcp_write_dss_mapping(tp, skb, ptr);
> > > > >                   }
> > > > >                   skb->dev = NULL;
> > > > >           }
> > > > > 
> > > > > 
>