Re: [PATCH 2/2] tcp: cleanup tcp_try_coalesce

Alexander Duyck
2012-05-03T04:58:56Z

On 5/2/2012 9:06 PM, Eric Dumazet wrote:
> On Wed, 2012-05-02 at 20:39 -0700, Alexander Duyck wrote:
>> This change is mostly meant to help improve the readability of
>> tcp_try_coalesce. I had a few issues since there were several points when
>> the code would test for a conditional, fail, then succeed on another
>> conditional take some action, and then follow a goto back into the previous
>> conditional. I just tore all of that apart and made the whole thing one
>> linear flow with a single goto.
>>
>> Also there were multiple ways of computing the delta, the one for head_frag
>> made the least amount of sense to me since we were only dropping the
>> sk_buff so I have updated the logic for the stolen head case so that delta
>> is only truesize - sizeof(skb_buff), and for the case where we are dropping
>> the head as well it is truesize - SKB_TRUESIZE(skb_end_pointer - head).
>> This way we can also account for the head_frag with headlen == 0.
>>
>> Signed-off-by: Alexander Duyck<alexander.h.duyck@intel.com>
>> Cc: Eric Dumazet<edumazet@google.com>
>> Cc: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>> ---
>>
> Sorry I prefer you dont touch this code like this.
>
> The truesize bits must stay as is, since it'll track drivers that lies
> about truesize.

I can understand that. But from what I can tell the only way they can
lie about truesize is to actually change the value itself. The code I
modified was just tightening things up so we didn't do things like use
the length to track the truesize like we were in the original code.
From what I can tell the new code should have been more accurate.

>> net/ipv4/tcp_input.c | 80 +++++++++++++++++++++++++++-----------------------
>> 1 files changed, 43 insertions(+), 37 deletions(-)
>>
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> index c6f78e2..23bc3ff 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -4548,62 +4548,68 @@ static bool tcp_try_coalesce(struct sock *sk,
>> int i, delta, len = from->len;
>>
>> *fragstolen = false;
>> +
>> if (tcp_hdr(from)->fin || skb_cloned(to))
>> return false;
>> +
>> if (len<= skb_tailroom(to)) {
>> BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
>> -merge:
>> - NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPRCVCOALESCE);
>> - TCP_SKB_CB(to)->end_seq = TCP_SKB_CB(from)->end_seq;
>> - TCP_SKB_CB(to)->ack_seq = TCP_SKB_CB(from)->ack_seq;
>> - return true;
>> + goto merge;
>> }
>>
>> if (skb_has_frag_list(to) || skb_has_frag_list(from))
>> return false;
>>
>> - if (skb_headlen(from) == 0&&
>> - (skb_shinfo(to)->nr_frags +
>> - skb_shinfo(from)->nr_frags<= MAX_SKB_FRAGS)) {
>> - WARN_ON_ONCE(from->head_frag);
>> - delta = from->truesize - ksize(from->head) -
>> - SKB_DATA_ALIGN(sizeof(struct sk_buff));
>
> This delta was done on purpose. We want the ksize()
The question I have is how can you get into a case where the ksize is
different from the end offset plus the aligned size of skb_shared_info?
>From what I can tell it looks like the only place we can lie is if we
use build_skb with the frag_size option, and in that case we are using a
page, not kmalloc memory. Otherwise in all other cases __alloc_skb or
build_skb is using ksize(skb->head) - SKB_DATA_ALIGN(struct
skb_shared_info) to set the end pointer, so reversing that should give
us the same value as ksize(skb->head).

>> -
>> - WARN_ON_ONCE(delta< len);
>> -copyfrags:
>> - memcpy(skb_shinfo(to)->frags + skb_shinfo(to)->nr_frags,
>> - skb_shinfo(from)->frags,
>> - skb_shinfo(from)->nr_frags * sizeof(skb_frag_t));
>> - skb_shinfo(to)->nr_frags += skb_shinfo(from)->nr_frags;
>> -
>> - if (skb_cloned(from))
>> - for (i = 0; i< skb_shinfo(from)->nr_frags; i++)
>> - skb_frag_ref(from, i);
>> - else
>> - skb_shinfo(from)->nr_frags = 0;
>> -
>> - to->truesize += delta;
>> - atomic_add(delta,&sk->sk_rmem_alloc);
>> - sk_mem_charge(sk, delta);
>> - to->len += len;
>> - to->data_len += len;
>> - goto merge;
>> - }
>> - if (from->head_frag&& !skb_cloned(from)) {
>> + if (skb_headlen(from) != 0) {
>> struct page *page;
>> unsigned int offset;
>>
>> - if (skb_shinfo(to)->nr_frags + skb_shinfo(from)->nr_frags>= MAX_SKB_FRAGS)
>> + if (skb_shinfo(to)->nr_frags +
>> + skb_shinfo(from)->nr_frags>= MAX_SKB_FRAGS)
>> + return false;
>> +
>> + if (!from->head_frag || skb_cloned(from))
>> return false;
>> +
>> + delta = from->truesize - sizeof(struct sk_buff);
>>
It looks like you were thinking of something here since the quote lines
were broken. This was the piece I saw in the original code that caught
my eye as probably being wrong. We are letting packets using head_frag
just report length as the truesize. This is why I favored using the end
offset. In the case of us using the head_frag we should only be
deducting SKB_DATA_ALIGN(sizeof(struct sk_buff)) (I just relized the
line above is incorrect), and in the case of us dropping the head_frag
as well we should be able to also deduct the size of the shared_info and
everything up to the offset of end.

>> +
>> page = virt_to_head_page(from->head);
>> offset = from->data - (unsigned char *)page_address(page);
>> +
>> skb_fill_page_desc(to, skb_shinfo(to)->nr_frags,
>> page, offset, skb_headlen(from));
>> *fragstolen = true;
>> - delta = len; /* we dont know real truesize... */
>> - goto copyfrags;
>> + } else {
>> + if (skb_shinfo(to)->nr_frags +
>> + skb_shinfo(from)->nr_frags> MAX_SKB_FRAGS)
>> + return false;
>> +
>> + delta = from->truesize -
>> + SKB_TRUESIZE(skb_end_pointer(from) - from->head);
> No... SKB_TRUESIZE() doesnt account of power-of-two roundings in
> kmalloc()
You used ksize in alloc_skb or build_skb to set the end pointer. All I
am doing by using the end pointer is getting the value you had already
extracted. The end pointer should be unmovable once it is set so I
wouldn't think it would be an issue to use it to compute the truesize
for the section including the sk_buff and skb->head.

>> }
>> - return false;
>> +
>> + memcpy(skb_shinfo(to)->frags + skb_shinfo(to)->nr_frags,
>> + skb_shinfo(from)->frags,
>> + skb_shinfo(from)->nr_frags * sizeof(skb_frag_t));
>> + skb_shinfo(to)->nr_frags += skb_shinfo(from)->nr_frags;
>> +
>> + if (!skb_cloned(from))
>> + skb_shinfo(from)->nr_frags = 0;
>> +
> You break the code here... we had an else.
Yes and no. I just realized that I didn't need the else because the for
loop below does nothing if nr_frags is 0. I suspect gcc is probably
smart enough to just skip the loop if the skb is cloned. I should
probably have added a one line comment explaining that above the loop.

>> + for (i = 0; i< skb_shinfo(from)->nr_frags; i++)
>> + skb_frag_ref(from, i);
>> +
>> + to->truesize += delta;
>> + atomic_add(delta,&sk->sk_rmem_alloc);
>> + sk_mem_charge(sk, delta);
>> + to->len += len;
>> + to->data_len += len;
>> +
>> +merge:
>> + NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPRCVCOALESCE);
>> + TCP_SKB_CB(to)->end_seq = TCP_SKB_CB(from)->end_seq;
>> + TCP_SKB_CB(to)->ack_seq = TCP_SKB_CB(from)->ack_seq;
>> + return true;
>> }
>>
>> static void kfree_skb_partial(struct sk_buff *skb, bool head_stolen)
>>
> Really this patch is too hard to review.
Yeah, it is a bit difficult. After Dave pulled your patches it ended up
pushing most of the changes into this one. I'll see if I can break this
back up into smaller bits. I just needed to flush the patches I had so
I could get some feedback and stop talking in circles about the other patch.

Thanks,

Alex