netdev - Re: [PATCH] fragment: add fast path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTimN1-uyAvpxgpno55XoyPzBptdZimz-MA-0MCWZ@mail.gmail.com>
Date:	Mon, 14 Jun 2010 13:59:11 +0800
From:	Changli Gao <xiaosuo@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Alexey Kuznetsov <kuznet@....inr.ac.ru>,
	"Pekka Savola (ipv6)" <pekkas@...core.fi>,
	James Morris <jmorris@...ei.org>,
	Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
	Patrick McHardy <kaber@...sh.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] fragment: add fast path

On Mon, Jun 14, 2010 at 1:35 PM, Eric Dumazet <eric.dumazet@...il.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@...il.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@...il.com)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html