udp: Neaten and reduce size of compute_score functions
Subsystems (3)
PATCH 1/1 udp: Neaten and reduce size of compute_score functions
The compute_score functions are a bit difficult to read.
Neaten them a bit to reduce object sizes and make them a bit more intelligible.
Return early to avoid indentation and avoid unnecessary initializations.
(allyesconfig, but w/ -O2 and no profiling)
$ size net/ipv[46]/udp.o.* text data bss dec hex filename 28680 1184 25 29889 74c1 net/ipv4/udp.o.new 28756 1184 25 29965 750d net/ipv4/udp.o.old 17600 1010 2 18612 48b4 net/ipv6/udp.o.new 17632 1010 2 18644 48d4 net/ipv6/udp.o.old
--- net/ipv4/udp.c | 111 +++++++++++++++++++++++++++++++------------------------- net/ipv6/udp.c | 113 ++++++++++++++++++++++++++++++++------------------------- 2 files changed, 125 insertions(+), 99 deletions(-)
net/ipv4/udp.c +62 -49
@@ -336,38 +336,45 @@ int udp_v4_get_port(struct sock *sk, unsigned short snum)return udp_lib_get_port (sk, snum, ipv4_rcv_saddr_equal, hash2_nulladdr); } -static inline int compute_score (struct sock *sk,struct net *net,__be32 saddr, -unsigned short hnum, -__be16 sport,__be32 daddr,__be16 dport,int dif) +static inline int compute_score (struct sock *sk,struct net *net, +__be32 saddr,unsigned short hnum,__be16 sport, +__be32 daddr,__be16 dport,int dif) { -int score =-1 ; +int score; +struct inet_sock *inet; -if (net_eq (sock_net (sk), net) &&udp_sk (sk)->udp_port_hash == hnum && - !ipv6_only_sock (sk)) { -struct inet_sock *inet =inet_sk (sk); +if (!(net_eq (sock_net (sk), net) && +udp_sk (sk)->udp_port_hash == hnum && + !ipv6_only_sock (sk))) +return -1 ; - score = (sk->sk_family ==PF_INET ?2 :1 ); -if (inet->inet_rcv_saddr) { -if (inet->inet_rcv_saddr != daddr) -return -1 ; - score +=4 ; - } -if (inet->inet_daddr) { -if (inet->inet_daddr != saddr) -return -1 ; - score +=4 ; - } -if (inet->inet_dport) { -if (inet->inet_dport != sport) -return -1 ; - score +=4 ; - } -if (sk->sk_bound_dev_if) { -if (sk->sk_bound_dev_if != dif) -return -1 ; - score +=4 ; - } + score = (sk->sk_family ==PF_INET ) ?2 :1 ; + inet =inet_sk (sk); + +if (inet->inet_rcv_saddr) { +if (inet->inet_rcv_saddr != daddr) +return -1 ; + score +=4 ; + } + +if (inet->inet_daddr) { +if (inet->inet_daddr != saddr) +return -1 ; + score +=4 ; + } + +if (inet->inet_dport) { +if (inet->inet_dport != sport) +return -1 ; + score +=4 ; + } + +if (sk->sk_bound_dev_if) { +if (sk->sk_bound_dev_if != dif) +return -1 ; + score +=4 ; } +return score; }
@@ -378,33 +385,39 @@ static inline int compute_score2(struct sock *sk, struct net *net, __be32 saddr, __be16 sport, __be32 daddr, unsigned int hnum, int dif) { - int score = -1; + int score; +struct inet_sock *inet; -if (net_eq (sock_net (sk), net) && !ipv6_only_sock (sk)) { -struct inet_sock *inet =inet_sk (sk); +if (!(net_eq (sock_net (sk), net) && !ipv6_only_sock (sk))) +return -1 ; -if (inet->inet_rcv_saddr != daddr) + inet =inet_sk (sk); + +if (inet->inet_rcv_saddr != daddr) +return -1 ; +if (inet->inet_num != hnum) +return -1 ; + + score = (sk->sk_family ==PF_INET ) ?2 :1 ; + +if (inet->inet_daddr) { +if (inet->inet_daddr != saddr)return -1 ; -if (inet->inet_num != hnum) + score +=4 ; + } + +if (inet->inet_dport) { +if (inet->inet_dport != sport)return -1 ; + score +=4 ; + } - score = (sk->sk_family ==PF_INET ?2 :1 ); -if (inet->inet_daddr) { -if (inet->inet_daddr != saddr) -return -1 ; - score +=4 ; - } -if (inet->inet_dport) { -if (inet->inet_dport != sport) -return -1 ; - score +=4 ; - } -if (sk->sk_bound_dev_if) { -if (sk->sk_bound_dev_if != dif) -return -1 ; - score +=4 ; - } +if (sk->sk_bound_dev_if) { +if (sk->sk_bound_dev_if != dif) +return -1 ; + score +=4 ; } +return score; }
net/ipv6/udp.c +63 -50
135 lines not on this page · raw message
Quoted 30 lines · Joe Perches
The
compute_scorefunctions are a bit difficult to read.Neaten them a bit to reduce object sizes and make them a bit more intelligible.
Return early to avoid indentation and avoid unnecessary initializations.
Signed-off-by Joe Perches <[email protected]>--- net/ipv4/udp.c | 111 +++++++++++++++++++++++++++++++------------------------- net/ipv6/udp.c | 113 ++++++++++++++++++++++++++++++++------------------------- 2 files changed, 125 insertions(+), 99 deletions(-)net/ipv4/udp.c +3
-4@@ -336,38 +336,45 @@ int udp_v4_get_port(struct sock *sk, unsigned short snum)return udp_lib_get_port (sk, snum, ipv4_rcv_saddr_equal, hash2_nulladdr); } -static inline int compute_score (struct sock *sk,struct net *net,__be32 saddr, -unsigned short hnum, -__be16 sport,__be32 daddr,__be16 dport,int dif) +static inline int compute_score (struct sock *sk,struct net *net, +__be32 saddr,unsigned short hnum,__be16 sport, +__be32 daddr,__be16 dport,int dif) { -int score =-1 ;
net/ipv4/udp.c int udp_v4_get_port(struct sock *sk, unsigned short snum) · line 349
Quoted 10 lines
net/ipv4/udp.c +6
-3+int score; +struct inet_sock *inet; -if (net_eq (sock_net (sk), net) &&udp_sk (sk)->udp_port_hash == hnum && - !ipv6_only_sock (sk)) { -struct inet_sock *inet =inet_sk (sk); +if (!(net_eq (sock_net (sk), net) && +udp_sk (sk)->udp_port_hash == hnum && + !ipv6_only_sock (sk))) +return -1
Or even better :
if (!net_eq(sock_net(sk), net) || udp_sk(sk)->udp_port_hash != hnum || ipv6_only_sock(sk)) return -1;
Quoted 8 lines · Eric Dumazet
Quoted 7 lines · Joe Perches
The
compute_scorefunctions are a bit difficult to read.Neaten them a bit to reduce object sizes and make them a bit more intelligible.
Return early to avoid indentation and avoid unnecessary initializations.
[]
Quoted 8 lines
net/ipv4/udp.c int udp_v4_get_port(struct sock *sk, unsigned short snum)
Quoted 4 lines
net/ipv4/udp.c +4
-0+if (!(net_eq (sock_net (sk), net) && +udp_sk (sk)->udp_port_hash == hnum && + !ipv6_only_sock (sk))) +return -1 Or even better :
Quoted 4 lines
if (!net_eq(sock_net(sk), net) || udp_sk(sk)->udp_port_hash != hnum || ipv6_only_sock(sk)) return -1;
Hi Eric.
Yeah, I thought about it but thought it
simpler to not change the logic.
Either way is fine with me.
David?
btw: the same thing can be done for the v6 block too:
+ if (!(net_eq(sock_net(sk), net) && !ipv6_only_sock(sk)))
+ return -1;
- if (inet->inet_rcv_saddr != daddr)
+ inet = inet_sk(sk);
+
+ if (inet->inet_rcv_saddr != daddr)
+ return -1;
+ if (inet->inet_num != hnum)
+ return -1;
to:
if (!net_eq(sock_net(sk, net) || ipv6_only_sock(sk)) return -1; inet = inet_sk(sk); if (inet->inet_rcv_saddr != daddr || inet->inet_num != hnum) return -1;
PATCH v2 1/1 udp: Neaten and reduce size of compute_score functions
The compute_score functions are a bit difficult to read.
Neaten them a bit to reduce object sizes and make them a bit more intelligible.
Return early to avoid indentation and avoid unnecessary initializations.
(allyesconfig, but w/ -O2 and no profiling)
$ size net/ipv[46]/udp.o.* text data bss dec hex filename 28680 1184 25 29889 74c1 net/ipv4/udp.o.new 28756 1184 25 29965 750d net/ipv4/udp.o.old 17600 1010 2 18612 48b4 net/ipv6/udp.o.new 17632 1010 2 18644 48d4 net/ipv6/udp.o.old
---
Change the && == blocks to || !=
No change in compiled object files.
Keeps Eric happy too.
net/ipv4/udp.c | 111 +++++++++++++++++++++++++++++++------------------------- net/ipv6/udp.c | 113 ++++++++++++++++++++++++++++++++------------------------- 2 files changed, 125 insertions(+), 99 deletions(-)
net/ipv4/udp.c +62 -49
@@ -336,38 +336,45 @@ int udp_v4_get_port(struct sock *sk, unsigned short snum)return udp_lib_get_port (sk, snum, ipv4_rcv_saddr_equal, hash2_nulladdr); } -static inline int compute_score (struct sock *sk,struct net *net,__be32 saddr, -unsigned short hnum, -__be16 sport,__be32 daddr,__be16 dport,int dif) +static inline int compute_score (struct sock *sk,struct net *net, +__be32 saddr,unsigned short hnum,__be16 sport, +__be32 daddr,__be16 dport,int dif) { -int score =-1 ; +int score; +struct inet_sock *inet; -if (net_eq (sock_net (sk), net) &&udp_sk (sk)->udp_port_hash == hnum && - !ipv6_only_sock (sk)) { -struct inet_sock *inet =inet_sk (sk); +if (!net_eq (sock_net (sk), net) || +udp_sk (sk)->udp_port_hash != hnum || +ipv6_only_sock (sk)) +return -1 ; - score = (sk->sk_family ==PF_INET ?2 :1 ); -if (inet->inet_rcv_saddr) { -if (inet->inet_rcv_saddr != daddr) -return -1 ; - score +=4 ; - } -if (inet->inet_daddr) { -if (inet->inet_daddr != saddr) -return -1 ; - score +=4 ; - } -if (inet->inet_dport) { -if (inet->inet_dport != sport) -return -1 ; - score +=4 ; - } -if (sk->sk_bound_dev_if) { -if (sk->sk_bound_dev_if != dif) -return -1 ; - score +=4 ; - } + score = (sk->sk_family ==PF_INET ) ?2 :1 ; + inet =inet_sk (sk); + +if (inet->inet_rcv_saddr) { +if (inet->inet_rcv_saddr != daddr) +return -1 ; + score +=4 ; + } + +if (inet->inet_daddr) { +if (inet->inet_daddr != saddr) +return -1 ; + score +=4 ; } + +if (inet->inet_dport) { +if (inet->inet_dport != sport) +return -1 ; + score +=4 ; + } + +if (sk->sk_bound_dev_if) { +if (sk->sk_bound_dev_if != dif) +return -1 ; + score +=4 ; + } +return score; }
@@ -378,33 +385,39 @@ static inline int compute_score2(struct sock *sk, struct net *net, __be32 saddr, __be16 sport, __be32 daddr, unsigned int hnum, int dif) { - int score = -1; + int score; +struct inet_sock *inet; + +if (!net_eq (sock_net (sk), net) || +ipv6_only_sock (sk)) +return -1 ; -if (net_eq (sock_net (sk), net) && !ipv6_only_sock (sk)) { -struct inet_sock *inet =inet_sk (sk); + inet =inet_sk (sk); -if (inet->inet_rcv_saddr != daddr) +if (inet->inet_rcv_saddr != daddr || + inet->inet_num != hnum) +return -1 ; + + score = (sk->sk_family ==PF_INET ) ?2 :1 ; + +if (inet->inet_daddr) { +if (inet->inet_daddr != saddr)return -1 ; -if (inet->inet_num != hnum) + score +=4 ; + } + +if (inet->inet_dport) { +if (inet->inet_dport != sport)return -1 ; + score +=4 ; + } - score = (sk->sk_family ==PF_INET ?2 :1 ); -if (inet->inet_daddr) { -if (inet->inet_daddr != saddr) -return -1 ; - score +=4 ; - } -if (inet->inet_dport) { -if (inet->inet_dport != sport) -return -1 ; - score +=4 ; - } -if (sk->sk_bound_dev_if) { -if (sk->sk_bound_dev_if != dif) -return -1 ; - score +=4 ; - } +if (sk->sk_bound_dev_if) { +if (sk->sk_bound_dev_if != dif) +return -1 ; + score +=4 ; } +return score; }
net/ipv6/udp.c +63 -50
135 lines not on this page · raw message
Quoted 23 lines · Joe Perches
Quoted 8 lines · Eric Dumazet
Quoted 7 lines · Joe Perches
The
compute_scorefunctions are a bit difficult to read.Neaten them a bit to reduce object sizes and make them a bit more intelligible.
Return early to avoid indentation and avoid unnecessary initializations.
[]
Quoted 8 lines
net/ipv4/udp.c int udp_v4_get_port(struct sock *sk, unsigned short snum)
Quoted 4 lines
net/ipv4/udp.c +4
-0+if (!(net_eq (sock_net (sk), net) && +udp_sk (sk)->udp_port_hash == hnum && + !ipv6_only_sock (sk))) +return -1 Or even better :
Quoted 4 lines
if (!net_eq(sock_net(sk), net) || udp_sk(sk)->udp_port_hash != hnum || ipv6_only_sock(sk)) return -1;
Quoted 6 lines
Hi Eric.
Yeah, I thought about it but thought it
simpler to not change the logic.Either way is fine with me.
Your patch does not change the logic at all.
My suggestion is about avoiding double negates which are really hard to read. This is simple boolean logic rules.
Code was :
if (a && x == y && !ipv6_only_sock(sk))) {
EXPR1;
} else {
return -1;
}So the 'logical' way of negating the condition is actually to not add double negations and use :
if (!a || x != y || ipv6_only_sock(sk)))
return -1;
EXPR1;
Instead of the less readable form :
if (!(a && x == y && !ipv6_only_sock(sk))))
return -1;
EXPR1;
You do not have to ask David permission for this, really.
Quoted 9 lines · Joe Perches
The
compute_scorefunctions are a bit difficult to read.Neaten them a bit to reduce object sizes and make them a bit more intelligible.
Return early to avoid indentation and avoid unnecessary initializations.
(allyesconfig, but w/ -O2 and no profiling)
hmm... Not sure how you get such large numbers...
Quoted 7 lines
$ size net/ipv[46]/udp.o.* text data bss dec hex filename 28680 1184 25 29889 74c1 net/ipv4/udp.o.new 28756 1184 25 29965 750d net/ipv4/udp.o.old 17600 1010 2 18612 48b4 net/ipv6/udp.o.new 17632 1010 2 18644 48d4 net/ipv6/udp.o.old
Here I have :
# size net/ipv4/udp.o.* text data bss dec hex filename 21989 616 9 22614 5856 net/ipv4/udp.o.old 21957 616 9 22582 5836 net/ipv4/udp.o.new
With CONFIG_CC_OPTIMIZE_FOR_SIZE=y I even have an opposite result (code gets bigger after your patch)
# size net/ipv4/udp.o.* text data bss dec hex filename 17242 600 9 17851 45bb net/ipv4/udp.o.old 17256 600 9 17865 45c9 net/ipv4/udp.o.new
Anyway, your patch looks fine to me, no matter what the code size is.
Quoted 6 lines · Eric Dumazet
Quoted 5 lines · Joe Perches
The
compute_scorefunctions are a bit difficult to read.Neaten them a bit to reduce object sizes and make them a bit more intelligible.
Quoted 6 lines
Quoted 4 lines
Return early to avoid indentation and avoid unnecessary initializations.
(allyesconfig, but w/ -O2 and no profiling)
hmm... Not sure how you get such large numbers...
Nor I particularly, but I do with gcc 4.9.1
Quoted 4 lines
Quoted 4 lines
$ size net/ipv[46]/udp.o.* text data bss dec hex filename 28680 1184 25 29889 74c1 net/ipv4/udp.o.new 28756 1184 25 29965 750d net/ipv4/udp.o.old
[]
Quoted 6 lines
Here I have :
# size net/ipv4/udp.o.* text data bss dec hex filename 21989 616 9 22614 5856 net/ipv4/udp.o.old 21957 616 9 22582 5836 net/ipv4/udp.o.new
Curious. What gcc version?
with that 4.9.1 gcc version and a defconfig (x86-64) I get:
$ size net/ipv[46]/udp.o* text data bss dec hex filename 21328 672 9 22009 55f9 net/ipv4/udp.o.new 21312 672 9 21993 55e9 net/ipv4/udp.o.old 14463 588 2 15053 3acd net/ipv6/udp.o.new 14527 588 2 15117 3b0d net/ipv6/udp.o.old
and defconfig x86-32:
$ size net/ipv[46]/udp.o* text data bss dec hex filename 19626 324 5 19955 4df3 net/ipv4/udp.o.new 19706 324 5 20035 4e43 net/ipv4/udp.o.old 14189 300 2 14491 389b net/ipv6/udp.o.new 14125 300 2 14427 385b net/ipv6/udp.o.old
Quoted 11 lines
With
CONFIG_CC_OPTIMIZE_FOR_SIZE=yI even have an opposite result (code gets bigger after your patch)# size net/ipv4/udp.o.* text data bss dec hex filename 17242 600 9 17851 45bb net/ipv4/udp.o.old 17256 600 9 17865 45c9 net/ipv4/udp.o.newAnyway, your patch looks fine to me, no matter what the code size is.
Acked-by Eric Dumazet <[email protected]>
From: Joe Perches <[email protected]>
Date: Mon, 01 Dec 2014 20:29:06 -0800
Quoted 10 lines
The
compute_scorefunctions are a bit difficult to read.Neaten them a bit to reduce object sizes and make them a bit more intelligible.
Return early to avoid indentation and avoid unnecessary initializations.
(allyesconfig, but w/ -O2 and no profiling)
Quoted 8 lines
$ size net/ipv[46]/udp.o.* text data bss dec hex filename 28680 1184 25 29889 74c1 net/ipv4/udp.o.new 28756 1184 25 29965 750d net/ipv4/udp.o.old 17600 1010 2 18612 48b4 net/ipv6/udp.o.new 17632 1010 2 18644 48d4 net/ipv6/udp.o.oldSigned-off-by Joe Perches <[email protected]>
Applied, thanks Joe.