net: fix bogus cast in skb_pagelen() and use unsigned variables
Subsystems (3)
PATCH 1/1 net: fix bogus cast in skb_pagelen() and use unsigned variables
1) cast to "int" is unnecessary:
u8 will be promoted to int before decrementing,
small positive numbers fit into "int", so their values won't be changed during promotion.
Once everything is int including loop counters, signedness doesn't
matter: 32-bit operations will stay 32-bit operations.
But! Someone tried to make this loop smart by making everything of the same type apparently in an attempt to optimise it. Do the optimization, just differently.
Do the cast where it matters. :^)
2) frag size is unsigned entity and sum of fragments sizes is also unsigned.
Make everything unsigned, leave no MOVSX instruction behind.
add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-4 (-4)
function old new delta
skb_cow_data 835 834 -1
ip_do_fragment 2549 2548 -1
ip6_fragment 3130 3128 -2
Total: Before=154865032, After=154865028, chg -0.00%
---
include/linux/skbuff.h | 6 +++--- net/ipv4/ip_output.c | 2 +- net/ipv6/ip6_output.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-)
include/linux/skbuff.h +3 -3
@@ -1799,11 +1799,11 @@ static inline unsigned int skb_headlen(const struct sk_buff *skb)return skb->len - skb->data_len; } -static inline int skb_pagelen (const struct sk_buff *skb) +static inline unsigned int skb_pagelen (const struct sk_buff *skb) { -int i, len =0 ; +unsigned int i, len =0 ; -for (i = (int )skb_shinfo (skb)->nr_frags -1 ; i >=0 ; i--) +for (i =skb_shinfo (skb)->nr_frags -1 ; (int )i >=0 ; i--) len +=skb_frag_size (&skb_shinfo (skb)->frags[i]);return len +skb_headlen (skb); }
net/ipv4/ip_output.c +1 -1
@@ -581,7 +581,7 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,*/ if (skb_has_frag_list (skb)) {struct sk_buff *frag, *frag2; -int first_len =skb_pagelen (skb); +unsigned int first_len =skb_pagelen (skb); if (first_len - hlen > mtu || ((first_len - hlen) & 7) ||
net/ipv6/ip6_output.c +1 -1
@@ -625,7 +625,7 @@ int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, hroom =LL_RESERVED_SPACE (rt->dst.dev);if (skb_has_frag_list (skb)) { -int first_len =skb_pagelen (skb); +unsigned int first_len =skb_pagelen (skb);struct sk_buff *frag2; if (first_len - hlen > mtu ||
From: Alexey Dobriyan <[email protected]>
Date: Sat, 19 Nov 2016 04:08:08 +0300
Quoted 17 lines
1) cast to "int" is unnecessary:
u8 will be promoted to int before decrementing,
small positive numbers fit into "int", so their values won't be changed during promotion.Once everything is int including loop counters, signedness doesn't
matter: 32-bit operations will stay 32-bit operations.But! Someone tried to make this loop smart by making everything of the same type apparently in an attempt to optimise it. Do the optimization, just differently.
Do the cast where it matters. :^)2) frag size is unsigned entity and sum of fragments sizes is also unsigned.
Make everything unsigned, leave no MOVSX instruction behind.
...
Quoted 1 line
Signed-off-by Alexey Dobriyan <[email protected]>
Applied to net-next.
From: Alexey Dobriyan
Quoted 1 line
Sent: 19 November 2016 01:08
...
include/linux/skbuff.h static inline unsigned int skb_headlen(const struct sk_buff *skb) · line 1807
Quoted 3 lines
include/linux/skbuff.h +1
-1-for (i = (int )skb_shinfo (skb)->nr_frags -1 ; i >=0 ; i--) +for (i =skb_shinfo (skb)->nr_frags -1 ; (int )i >=0 ; i--) len +=skb_frag_size (&skb_shinfo (skb)->frags[i]);
Think I'd use:
for (i = skb_shinfo(skb)->nr_frags; i-- != 0; ) David
Quoted 9 lines · David Laight
From: Alexey Dobriyan
Quoted 1 line
Sent: 19 November 2016 01:08
...
include/linux/skbuff.h static inline unsigned int skb_headlen(const struct sk_buff *skb) · line 1807
Quoted 3 lines
include/linux/skbuff.h +1
-1-for (i = (int )skb_shinfo (skb)->nr_frags -1 ; i >=0 ; i--) +for (i =skb_shinfo (skb)->nr_frags -1 ; (int )i >=0 ; i--) len +=skb_frag_size (&skb_shinfo (skb)->frags[i]);Think I'd use:
for (i = skb_shinfo(skb)->nr_frags; i-- != 0; )
This kind of diverges from canonical loop form:
for (init; temination condition: iterator)
by shifting everything into termination part.
A