| Message ID | 1352668801-14373-1-git-send-email-xi.wang@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
This code is a fast bit test on purpose. You're making the code slower. I'm not applying this patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/11/12 5:02 PM, David Miller wrote: > > This code is a fast bit test on purpose. You're making the > code slower. No, it's the opposite. All modern compilers optimize switch cases into a fast bit test. The original "smarter" code, however, hinders the compiler determining the mask for a fast bit test. With this patch, GCC is able to compute a better mask. You can check the assembly. - xi -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Xi Wang <xi.wang@gmail.com> Date: Sun, 11 Nov 2012 17:18:55 -0500 > On 11/11/12 5:02 PM, David Miller wrote: >> >> This code is a fast bit test on purpose. You're making the >> code slower. > > No, it's the opposite. All modern compilers optimize switch cases > into a fast bit test. Indeed, I even checked sparc64 with gcc-4.6 and it looks good. Thanks for the clarification, I'll apply this, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> This code is a fast bit test on purpose. You're making the > >> code slower. > > > > No, it's the opposite. All modern compilers optimize switch cases > > into a fast bit test. 'All modern' is probably an overstatement, 'recent gcc' might be valid. > Indeed, I even checked sparc64 with gcc-4.6 and it looks good. > > Thanks for the clarification, I'll apply this, thanks. The 'switch' version will have an extra conditional to detect 'out of range' values - even though we know they can't happen. I'm not sure you can avoid that - even for an enum. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/12/12 8:54 AM, David Laight wrote: > 'All modern' is probably an overstatement, 'recent gcc' might be valid. I agree if you consider gcc 3.4 released 8 years ago as "recent gcc", or if you use a compiler other than gcc/clang/icc to compile the kernel. > The 'switch' version will have an extra conditional to detect > 'out of range' values - even though we know they can't happen. > I'm not sure you can avoid that - even for an enum. This out-of-range check is exactly what this patch wanted to add: optname is a syscall parameter, and we should reject invalid optname values before doing (1<<optname). - xi -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c index 5eea4a8..14bbfcf 100644 --- a/net/ipv4/ip_sockglue.c +++ b/net/ipv4/ip_sockglue.c @@ -457,19 +457,28 @@ static int do_ip_setsockopt(struct sock *sk, int level, struct inet_sock *inet = inet_sk(sk); int val = 0, err; - if (((1<<optname) & ((1<<IP_PKTINFO) | (1<<IP_RECVTTL) | - (1<<IP_RECVOPTS) | (1<<IP_RECVTOS) | - (1<<IP_RETOPTS) | (1<<IP_TOS) | - (1<<IP_TTL) | (1<<IP_HDRINCL) | - (1<<IP_MTU_DISCOVER) | (1<<IP_RECVERR) | - (1<<IP_ROUTER_ALERT) | (1<<IP_FREEBIND) | - (1<<IP_PASSSEC) | (1<<IP_TRANSPARENT) | - (1<<IP_MINTTL) | (1<<IP_NODEFRAG))) || - optname == IP_UNICAST_IF || - optname == IP_MULTICAST_TTL || - optname == IP_MULTICAST_ALL || - optname == IP_MULTICAST_LOOP || - optname == IP_RECVORIGDSTADDR) { + switch (optname) { + case IP_PKTINFO: + case IP_RECVTTL: + case IP_RECVOPTS: + case IP_RECVTOS: + case IP_RETOPTS: + case IP_TOS: + case IP_TTL: + case IP_HDRINCL: + case IP_MTU_DISCOVER: + case IP_RECVERR: + case IP_ROUTER_ALERT: + case IP_FREEBIND: + case IP_PASSSEC: + case IP_TRANSPARENT: + case IP_MINTTL: + case IP_NODEFRAG: + case IP_UNICAST_IF: + case IP_MULTICAST_TTL: + case IP_MULTICAST_ALL: + case IP_MULTICAST_LOOP: + case IP_RECVORIGDSTADDR: if (optlen >= sizeof(int)) { if (get_user(val, (int __user *) optval)) return -EFAULT;
(1<<optname) is undefined behavior in C with a negative optname or optname larger than 31. In those cases the result of the shift is not necessarily zero (e.g., on x86). This patch simplifies the code with a switch statement on optname. It also allows the compiler to generate better code (e.g., using a 64-bit mask). Signed-off-by: Xi Wang <xi.wang@gmail.com> --- net/ipv4/ip_sockglue.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-)