* [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() @ 2007-12-21 6:03 Eric Dumazet 2007-12-21 6:50 ` YOSHIFUJI Hideaki / 吉藤英明 2007-12-21 7:21 ` Ilpo Järvinen 0 siblings, 2 replies; 12+ messages in thread From: Eric Dumazet @ 2007-12-21 6:03 UTC (permalink / raw) To: David S. Miller, YOSHIFUJI Hideaki / 吉藤英明 Cc: Linux Netdev List [-- 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; ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() 2007-12-21 6:03 [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() Eric Dumazet @ 2007-12-21 6:50 ` YOSHIFUJI Hideaki / 吉藤英明 2007-12-21 7:06 ` Eric Dumazet 2007-12-21 11:26 ` Jeff Garzik 2007-12-21 7:21 ` Ilpo Järvinen 1 sibling, 2 replies; 12+ messages in thread From: YOSHIFUJI Hideaki / 吉藤英明 @ 2007-12-21 6:50 UTC (permalink / raw) To: dada1; +Cc: davem, netdev, yoshfuji 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() 2007-12-21 6:50 ` YOSHIFUJI Hideaki / 吉藤英明 @ 2007-12-21 7:06 ` Eric Dumazet [not found] ` <20071221.162833.82587283.yoshfuji@linux-ipv6.org> 2007-12-21 11:26 ` Jeff Garzik 1 sibling, 1 reply; 12+ messages in thread From: Eric Dumazet @ 2007-12-21 7:06 UTC (permalink / raw) To: YOSHIFUJI Hideaki / 吉藤英明; +Cc: davem, netdev 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <20071221.162833.82587283.yoshfuji@linux-ipv6.org>]
* Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() [not found] ` <20071221.162833.82587283.yoshfuji@linux-ipv6.org> @ 2007-12-21 7:39 ` Eric Dumazet 2007-12-21 7:44 ` YOSHIFUJI Hideaki / 吉藤英明 2007-12-21 9:46 ` David Miller 2007-12-21 8:11 ` Eric Dumazet 1 sibling, 2 replies; 12+ messages in thread From: Eric Dumazet @ 2007-12-21 7:39 UTC (permalink / raw) To: YOSHIFUJI Hideaki / 吉藤英明; +Cc: davem, netdev 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() 2007-12-21 7:39 ` Eric Dumazet @ 2007-12-21 7:44 ` YOSHIFUJI Hideaki / 吉藤英明 2007-12-21 7:50 ` Eric Dumazet 2007-12-21 9:46 ` David Miller 1 sibling, 1 reply; 12+ messages in thread From: YOSHIFUJI Hideaki / 吉藤英明 @ 2007-12-21 7:44 UTC (permalink / raw) To: dada1; +Cc: davem, netdev, yoshfuji 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() 2007-12-21 7:44 ` YOSHIFUJI Hideaki / 吉藤英明 @ 2007-12-21 7:50 ` Eric Dumazet 0 siblings, 0 replies; 12+ messages in thread From: Eric Dumazet @ 2007-12-21 7:50 UTC (permalink / raw) To: YOSHIFUJI Hideaki / 吉藤英明; +Cc: davem, netdev 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 :) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() 2007-12-21 7:39 ` Eric Dumazet 2007-12-21 7:44 ` YOSHIFUJI Hideaki / 吉藤英明 @ 2007-12-21 9:46 ` David Miller 1 sibling, 0 replies; 12+ messages in thread From: David Miller @ 2007-12-21 9:46 UTC (permalink / raw) To: dada1; +Cc: yoshfuji, netdev 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() [not found] ` <20071221.162833.82587283.yoshfuji@linux-ipv6.org> 2007-12-21 7:39 ` Eric Dumazet @ 2007-12-21 8:11 ` Eric Dumazet 1 sibling, 0 replies; 12+ messages in thread From: Eric Dumazet @ 2007-12-21 8:11 UTC (permalink / raw) To: YOSHIFUJI Hideaki / 吉藤英明; +Cc: davem, netdev 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() 2007-12-21 6:50 ` YOSHIFUJI Hideaki / 吉藤英明 2007-12-21 7:06 ` Eric Dumazet @ 2007-12-21 11:26 ` Jeff Garzik 2007-12-21 11:57 ` David Miller 1 sibling, 1 reply; 12+ messages in thread From: Jeff Garzik @ 2007-12-21 11:26 UTC (permalink / raw) To: YOSHIFUJI Hideaki / 吉藤英明; +Cc: dada1, davem, netdev 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() 2007-12-21 11:26 ` Jeff Garzik @ 2007-12-21 11:57 ` David Miller 2007-12-21 12:18 ` Jeff Garzik 0 siblings, 1 reply; 12+ messages in thread From: David Miller @ 2007-12-21 11:57 UTC (permalink / raw) To: jeff; +Cc: yoshfuji, dada1, netdev 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 ;-) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() 2007-12-21 11:57 ` David Miller @ 2007-12-21 12:18 ` Jeff Garzik 0 siblings, 0 replies; 12+ messages in thread From: Jeff Garzik @ 2007-12-21 12:18 UTC (permalink / raw) To: David Miller; +Cc: yoshfuji, dada1, netdev 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() 2007-12-21 6:03 [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() Eric Dumazet 2007-12-21 6:50 ` YOSHIFUJI Hideaki / 吉藤英明 @ 2007-12-21 7:21 ` Ilpo Järvinen 1 sibling, 0 replies; 12+ messages in thread From: Ilpo Järvinen @ 2007-12-21 7:21 UTC (permalink / raw) To: Eric Dumazet Cc: David S. Miller, YOSHIFUJI Hideaki / 吉藤英明, Linux Netdev List [-- 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; ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2007-12-21 12:18 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-12-21 6:03 [TCP] IPV6 : Change a divide into a right shift in tcp_v6_send_ack() Eric Dumazet
2007-12-21 6:50 ` YOSHIFUJI Hideaki / 吉藤英明
2007-12-21 7:06 ` Eric Dumazet
[not found] ` <20071221.162833.82587283.yoshfuji@linux-ipv6.org>
2007-12-21 7:39 ` Eric Dumazet
2007-12-21 7:44 ` YOSHIFUJI Hideaki / 吉藤英明
2007-12-21 7:50 ` Eric Dumazet
2007-12-21 9:46 ` David Miller
2007-12-21 8:11 ` Eric Dumazet
2007-12-21 11:26 ` Jeff Garzik
2007-12-21 11:57 ` David Miller
2007-12-21 12:18 ` Jeff Garzik
2007-12-21 7:21 ` Ilpo Järvinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox