Re: [PATCH] fragment: add fast path

Changli Gao
2010-06-14T05:59:32Z

On Mon, Jun 14, 2010 at 1:35 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 14 juin 2010 à 07:16 +0800, Changli Gao a écrit :
>> add fast path
>>
>> As the fragments are usually in order, it is likely the new fragments are
>> at the end of the inet_frag_queue. In the fast path, we check if the skb at the
>> end of the inet_frag_queue is the prev we expect.
>>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>> ----
>>  include/net/inet_frag.h |    1 +
>>  net/ipv4/ip_fragment.c  |   17 +++++++++++++++++
>>  net/ipv6/reassembly.c   |   16 ++++++++++++++++
>>  3 files changed, 34 insertions(+)
>> diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
>> index 39f2dc9..16ff29a 100644
>> --- a/include/net/inet_frag.h
>> +++ b/include/net/inet_frag.h
>> @@ -20,6 +20,7 @@ struct inet_frag_queue {
>>       atomic_t                refcnt;
>>       struct timer_list       timer;      /* when will this queue expire? */
>>       struct sk_buff          *fragments; /* list of received fragments */
>> +     struct sk_buff          *fragments_tail;
>>       ktime_t                 stamp;
>>       int                     len;        /* total length of orig datagram */
>>       int                     meat;
>> diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
>> index 75347ea..d8c36d4 100644
>> --- a/net/ipv4/ip_fragment.c
>> +++ b/net/ipv4/ip_fragment.c
>> @@ -317,6 +317,7 @@ static int ip_frag_reinit(struct ipq *qp)
>>       qp->q.len = 0;
>>       qp->q.meat = 0;
>>       qp->q.fragments = NULL;
>> +     qp->q.fragments_tail = NULL;
>>       qp->iif = 0;
>>
>>       return 0;
>> @@ -389,6 +390,16 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
>>        * in the chain of fragments so far.  We must know where to put
>>        * this fragment, right?
>>        */
>> +     prev = qp->q.fragments_tail;
>> +     if (prev) {
>> +             if (FRAG_CB(prev)->offset < offset) {
>> +                     next = NULL;
>> +                     goto found;
>> +             }
>> +     } else {
>> +             next = NULL;
>
> How can this chunk be a win ? queue is empty anyway.
> You add tests and slow the 'other path'
>
>> +             goto found;
>> +     }

Without this branch. prev needs to be initialized to zero again(of
course, we can avoid this by moving prev = NULL in the previous
branch). next needs an assignment, and a duplicate check if the the
queue is empty, which is already known in the above branch. Sorry, but
I can't see which path I slow.

prev = NULL;
for (next = qp->q.fragments; next != NULL; next = next->next) {
if (FRAG_CB(next)->offset >= offset)
break; /* bingo! */
prev = next;
}

--
Regards,
Changli Gao(xiaosuo@gmail.com)