On 7/3/15, 1:30 PM, "Neal Cardwell" <ncardw...@google.com> wrote:
>> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
>> index 48c3696..05e0da5 100644
>> --- a/include/linux/tcp.h
>> +++ b/include/linux/tcp.h
>> @@ -254,6 +254,10 @@ struct tcp_sock {
>> u32 lost_out; /* Lost packets */
>> u32 sacked_out; /* SACK'd packets
>>*/
>> u32 fackets_out; /* FACK'd packets
>>*/
>> + u32 ack_in_flight; /* This field is populated when new acks
>> + * are received. It contains the number
>>of
>> + * bytes in flight when the last packet
>> + * acked was sent. Used by tcp-nv. */
>
>AFAICT the tcp_sock struct does not really need to grow to hold the
>ack_in_flight field, because this field does not need to be remembered
>between ACKs. I would recommend putting it in a small struct ("struct
>ack_sample"?) that is allocated on the stack in tcp_ack() and passed
>into the pkts_acked() function for congestion control modules that
>want extra info beyond the number of packets ACKed and the RTT.
>
>In fact it might be cleaner to put the number of packets ACKed and the
>RTT in that struct as well, so in the future we don't have to modify
>all the congestion control modules' pkts_acked() function
>every time a new piece of info is provided by the core TCP stack.
It makes sense. I just wasn¹t sure if it made sense to add one more
parameter
to pkts_acked() until I heard if there was interest to have it in the
kernel.
However, I like your idea of passing a structure since it would simplify
future changes. I¹ll get to it.
>
>> /* This is what the send packet queuing engine uses to pass
>> * TCP per-packet control information to the transmission code.
>> * We also store the host-order sequence numbers in here too.
>> - * This is 44 bytes if IPV6 is enabled.
>> + * This is 48 bytes if IPV6 is enabled.
>> * If this grows please adjust skbuff.h:skbuff->cb[xxx] size
>>appropriately.
>> */
>> struct tcp_skb_cb {
>> __u32 seq; /* Starting sequence number
>>*/
>> __u32 end_seq; /* SEQ + FIN + SYN + datalen
>>*/
>> + __u32 in_flight; /* bytes in flight when this
>>packet
>> + * was sent. */
>
>AFAICT this patch would not require an increase in the size of sk_buff
>cb[] if it were to take advantage of the fact that the tcp_skb_cb
>header.h4 and header.h6 fields are only used in the packet reception
>code path, and this in_flight field is only used on the transmit
>side. So the in_flight field could be placed in a struct that is
>itself placed in a union with the "header" union. Like this:
>
> union {
> struct {
> /* bytes in flight when this packet was sent */
> __u32 in_flight;
> } tx; /* only used for outgoing skbs */
>
> union {
> struct inet_skb_parm h4;
>#if IS_ENABLED(CONFIG_IPV6)
> struct inet6_skb_parm h6;
>#endif
> } header; /* only used for incoming skbs */
> };
>
>That way the sender code can remember the in_flight value
>without requiring any extra space. And in the future other
>sender-side info could be stored in the "tx" struct, if needed.
>
>neal
Great idea, thanks Neal! I¹ll look into it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
