[TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack()

[TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack()

[Eric Dumazet]

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

Because tot_len is signed in tcp_v6_send_ack(), tot_len/4 forces compiler
to emit an integer divide, while we can help it to use a right shift,
less expensive.

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

[-- Attachment #2: tcp_ipv6.patch --]
[-- Type: text/plain, Size: 415 bytes --]

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 0268e11..92f0fda 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1124,7 +1124,7 @@ static void tcp_v6_send_ack(struct tcp_timewait_sock *tw,
 	memset(t1, 0, sizeof(*t1));
 	t1->dest = th->source;
 	t1->source = th->dest;
-	t1->doff = tot_len/4;
+	t1->doff = tot_len >> 2;
 	t1->seq = htonl(seq);
 	t1->ack_seq = htonl(ack);
 	t1->ack = 1;

Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack()

[YOSHIFUJI Hideaki / 吉藤英明]

In article <476B574E.80601@cosmosbay.com> (at Fri, 21 Dec 2007 07:03:58 +0100), Eric Dumazet <dada1@cosmosbay.com> says:

> Because tot_len is signed in tcp_v6_send_ack(), tot_len/4 forces compiler
> to emit an integer divide, while we can help it to use a right shift,
> less expensive.

Are you really sure?
At least, gcc-4.1.2-20061115 (debian) does not make any difference.

And, IMHO, because shift for signed variable is fragile, so we should
avoid using it.

--yoshfuji

Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack()

[Eric Dumazet]

YOSHIFUJI Hideaki / 吉藤英明 a écrit :
> In article <476B574E.80601@cosmosbay.com> (at Fri, 21 Dec 2007 07:03:58 +0100), Eric Dumazet <dada1@cosmosbay.com> says:
> 
>> Because tot_len is signed in tcp_v6_send_ack(), tot_len/4 forces compiler
>> to emit an integer divide, while we can help it to use a right shift,
>> less expensive.
> 
> Are you really sure?
> At least, gcc-4.1.2-20061115 (debian) does not make any difference.
> 
> And, IMHO, because shift for signed variable is fragile, so we should
> avoid using it.
> 

Yes I am sure, but maybe you are on x86_64 ?

gcc-4.2.2 on x86

# objdump --disassemble net/ipv6/tcp_ipv6.o|grep -6 idiv
       b2:       66 8b 42 02             mov    0x2(%edx),%ax
       b6:       ba 04 00 00 00          mov    $0x4,%edx
       bb:       89 d7                   mov    %edx,%edi
       bd:       66 89 45 00             mov    %ax,0x0(%ebp)
       c1:       89 d8                   mov    %ebx,%eax
       c3:       99                      cltd
       c4:       f7 ff                   idiv   %edi
       c6:       88 c2                   mov    %al,%dl
       c8:       8a 45 0c                mov    0xc(%ebp),%al
       cb:       c1 e2 04                shl    $0x4,%edx
       ce:       83 e0 0f                and    $0xf,%eax
       d1:       09 d0                   or     %edx,%eax
       d3:       88 45 0c                mov    %al,0xc(%ebp)


If you think tot_len can be negative, I understand you can be against this 
patch. But I am sure it's allways > 0, even if I am a total ipv6 newbie :)

Thank you

Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack()

[Ilpo Järvinen]

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

On Fri, 21 Dec 2007, Eric Dumazet wrote:

> Because tot_len is signed in tcp_v6_send_ack(), tot_len/4 forces compiler
> to emit an integer divide, while we can help it to use a right shift,
> less expensive.

Can't you just change tot_len to unsigned here? It's just sizeof and some 
positive constants added...

-- 
 i.

[-- Attachment #2: Type: text/plain, Size: 415 bytes --]

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 0268e11..92f0fda 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1124,7 +1124,7 @@ static void tcp_v6_send_ack(struct tcp_timewait_sock *tw,
 	memset(t1, 0, sizeof(*t1));
 	t1->dest = th->source;
 	t1->source = th->dest;
-	t1->doff = tot_len/4;
+	t1->doff = tot_len >> 2;
 	t1->seq = htonl(seq);
 	t1->ack_seq = htonl(ack);
 	t1->ack = 1;

Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack()

[Eric Dumazet]

YOSHIFUJI Hideaki / 吉藤英明 a écrit :
> In article <476B65F8.10201@cosmosbay.com> (at Fri, 21 Dec 2007 08:06:32 +0100), Eric Dumazet <dada1@cosmosbay.com> says:
> 
>> YOSHIFUJI Hideaki / 吉藤英明 a écrit :
>>> In article <476B574E.80601@cosmosbay.com> (at Fri, 21 Dec 2007 07:03:58 +0100), Eric Dumazet <dada1@cosmosbay.com> says:
>>>
>>>> Because tot_len is signed in tcp_v6_send_ack(), tot_len/4 forces compiler
>>>> to emit an integer divide, while we can help it to use a right shift,
>>>> less expensive.
>>> Are you really sure?
>>> At least, gcc-4.1.2-20061115 (debian) does not make any difference.
>>>
>>> And, IMHO, because shift for signed variable is fragile, so we should
>>> avoid using it.
>>>
>> Yes I am sure, but maybe you are on x86_64 ?
>>
>> gcc-4.2.2 on x86
> 
> I'm on gcc-4.1.2 20061115 (prerelease) (Debian 4.1.1-21), on x86 (i686).
> Maybe compiler difference?!
> 
>> If you think tot_len can be negative, I understand you can be against this 
>> patch. But I am sure it's allways > 0, even if I am a total ipv6 newbie :)
> 
> Okay, anyway, I'll convert them to unsigned int, which is more
> appropriate.

I didnt chose this path, because David was against changing some fields from 
'int' to 'unsigned'. If you look in other parts of networking, we have many >> 
1 or >> 2 already there.

Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack()

[YOSHIFUJI Hideaki / 吉藤英明]

In article <476B6DAC.2030102@cosmosbay.com> (at Fri, 21 Dec 2007 08:39:24 +0100), Eric Dumazet <dada1@cosmosbay.com> says:

> > Okay, anyway, I'll convert them to unsigned int, which is more
> > appropriate.
> 
> I didnt chose this path, because David was against changing some fields from 
> 'int' to 'unsigned'. If you look in other parts of networking, we have many >> 
> 1 or >> 2 already there.

I do think it is safe to convert them here.

--yoshfuji

Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack()

[Eric Dumazet]

YOSHIFUJI Hideaki / 吉藤英明 a écrit :
> In article <476B6DAC.2030102@cosmosbay.com> (at Fri, 21 Dec 2007 08:39:24 +0100), Eric Dumazet <dada1@cosmosbay.com> says:
> 
>>> Okay, anyway, I'll convert them to unsigned int, which is more
>>> appropriate.
>> I didnt chose this path, because David was against changing some fields from 
>> 'int' to 'unsigned'. If you look in other parts of networking, we have many >> 
>> 1 or >> 2 already there.
> 
> I do think it is safe to convert them here.
> 

Sure :)

Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack()

[Eric Dumazet]

YOSHIFUJI Hideaki / 吉藤英明 a écrit :
> In article <476B65F8.10201@cosmosbay.com> (at Fri, 21 Dec 2007 08:06:32 +0100), Eric Dumazet <dada1@cosmosbay.com> says:
> 
>> YOSHIFUJI Hideaki / 吉藤英明 a écrit :
>>> In article <476B574E.80601@cosmosbay.com> (at Fri, 21 Dec 2007 07:03:58 +0100), Eric Dumazet <dada1@cosmosbay.com> says:
>>>
>>>> Because tot_len is signed in tcp_v6_send_ack(), tot_len/4 forces compiler
>>>> to emit an integer divide, while we can help it to use a right shift,
>>>> less expensive.
>>> Are you really sure?
>>> At least, gcc-4.1.2-20061115 (debian) does not make any difference.
>>>
>>> And, IMHO, because shift for signed variable is fragile, so we should
>>> avoid using it.
>>>
>> Yes I am sure, but maybe you are on x86_64 ?
>>
>> gcc-4.2.2 on x86
> 
> I'm on gcc-4.1.2 20061115 (prerelease) (Debian 4.1.1-21), on x86 (i686).
> Maybe compiler difference?!

hum, more probably a .config difference. Try with

CONFIG_CC_OPTIMIZE_FOR_SIZE=Y

(default config)

Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack()

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Fri, 21 Dec 2007 08:39:24 +0100

> I didnt chose this path, because David was against changing some
> fields from 'int' to 'unsigned'. If you look in other parts of
> networking, we have many >> 1 or >> 2 already there.

I don't remember making this statement, where did I say this?
I'm genuinely curious :-)

But I am indeed against it in cases where the variable is compared
against signed quantities.

I think the shifts are more pretty and more closely match what the
programmer wants to come out of the compiler.  Getting all of these
divides is an awful surprise for me.

I've learned over the years to never explicitly code divides
or multiplies by powers of two and to always use shifts.  As
a result I am never surprised.  In fact I've been burnt every
time I mistakedly didn't use a shift.

Nevertheless, this tcplen arg is always assigned to and used
with unsigned quantities so I'll apply Yoshifuji's version of
the fix.

Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack()

[Jeff Garzik]

YOSHIFUJI Hideaki / 吉藤英明 wrote:
> In article <476B574E.80601@cosmosbay.com> (at Fri, 21 Dec 2007 07:03:58 +0100), Eric Dumazet <dada1@cosmosbay.com> says:
> 
>> Because tot_len is signed in tcp_v6_send_ack(), tot_len/4 forces compiler
>> to emit an integer divide, while we can help it to use a right shift,
>> less expensive.
> 
> Are you really sure?
> At least, gcc-4.1.2-20061115 (debian) does not make any difference.

Quite true -- thus it is a matter of taste to the programmer.  Constant 
folding inside the compiler ensures that "foo / 4" asm output is just as 
optimal as a shift.


> And, IMHO, because shift for signed variable is fragile, so we should
> avoid using it.

I respectfully disagree, but this is an unrelated matter.  As you say, 
"/4" is fine as-is.

	Jeff

Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack()

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Fri, 21 Dec 2007 06:26:48 -0500

> YOSHIFUJI Hideaki / 吉藤英明 wrote:
> > In article <476B574E.80601@cosmosbay.com> (at Fri, 21 Dec 2007 07:03:58 +0100), Eric Dumazet <dada1@cosmosbay.com> says:
> > 
> >> Because tot_len is signed in tcp_v6_send_ack(), tot_len/4 forces compiler
> >> to emit an integer divide, while we can help it to use a right shift,
> >> less expensive.
> > 
> > Are you really sure?
> > At least, gcc-4.1.2-20061115 (debian) does not make any difference.
> 
> Quite true -- thus it is a matter of taste to the programmer.

Not true, the code output does change, check your optimize-for-size
kernel config setting.

This was discussed and explained later in this thread, and I also
explained it to you on IRC Jeff ;-)

Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack()

[Jeff Garzik]

David Miller wrote:
> From: Jeff Garzik <jeff@garzik.org>
> Date: Fri, 21 Dec 2007 06:26:48 -0500
> 
>> YOSHIFUJI Hideaki / 吉藤英明 wrote:
>>> In article <476B574E.80601@cosmosbay.com> (at Fri, 21 Dec 2007 07:03:58 +0100), Eric Dumazet <dada1@cosmosbay.com> says:
>>>
>>>> Because tot_len is signed in tcp_v6_send_ack(), tot_len/4 forces compiler
>>>> to emit an integer divide, while we can help it to use a right shift,
>>>> less expensive.
>>> Are you really sure?
>>> At least, gcc-4.1.2-20061115 (debian) does not make any difference.
>> Quite true -- thus it is a matter of taste to the programmer.
> 
> Not true, the code output does change, check your optimize-for-size
> kernel config setting.
> 
> This was discussed and explained later in this thread, and I also
> explained it to you on IRC Jeff ;-)

[looks]

For signed integers, this is true.

For unsigned integers, the code output is the same, regardless of 
optimization setting.

	Jeff