| Message ID | 1451635091-109673-1-git-send-email-mikedanese@google.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2015-12-31 at 23:58 -0800, Mike Danese wrote: > We can reduce the number of operations performed by icmp_global_allow > and make the routine more readable by refactoring it in two ways: > > First, this patch refactors the meaning of the "delta" variable. Before > this change, it meant min("time since last refill of token bucket", HZ). > After this change, it means "time since last refill". The original > definition is required only once but was being calculated twice. The new > meaning is also more intuitive for a variable named "delta". > > Second, by calculating "delta" (time since last refill of token bucket) > and "cbr" (token bucket can be refilled) at the beginning of the > routine, we reduce the number of repeated calculations of these two > variables. > > There should be no functional difference. > > Signed-off-by: Mike Danese <mikedanese@google.com> > --- > net/ipv4/icmp.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) Hi Mike Sorry, this is a very broken patch. There is a comment you apparently missed completely : /* Check if token bucket is empty and cannot be refilled * without taking the spinlock. */ There is a reason we compute 'delta' two times. One without the spinlock held, and a second time with the spinlock held. This is an opportunistic way to exit early without false sharing in the stress case where many cpus might enter this code. Really I do not think current code needs any 'refactoring', especially around December 31th at midnight ;) -- 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
Yes, I completely missed the purpose of that. As a new year's resolution, I resolve to read the comments. Thanks for the review! On Fri, Jan 1, 2016 at 3:07 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Thu, 2015-12-31 at 23:58 -0800, Mike Danese wrote: >> We can reduce the number of operations performed by icmp_global_allow >> and make the routine more readable by refactoring it in two ways: >> >> First, this patch refactors the meaning of the "delta" variable. Before >> this change, it meant min("time since last refill of token bucket", HZ). >> After this change, it means "time since last refill". The original >> definition is required only once but was being calculated twice. The new >> meaning is also more intuitive for a variable named "delta". >> >> Second, by calculating "delta" (time since last refill of token bucket) >> and "cbr" (token bucket can be refilled) at the beginning of the >> routine, we reduce the number of repeated calculations of these two >> variables. >> >> There should be no functional difference. >> >> Signed-off-by: Mike Danese <mikedanese@google.com> >> --- >> net/ipv4/icmp.c | 17 ++++++++--------- >> 1 file changed, 8 insertions(+), 9 deletions(-) > > Hi Mike > > Sorry, this is a very broken patch. > > There is a comment you apparently missed completely : > > /* Check if token bucket is empty and cannot be refilled > * without taking the spinlock. > */ > > There is a reason we compute 'delta' two times. > > One without the spinlock held, and a second time with the spinlock held. > > This is an opportunistic way to exit early without false sharing in the > stress case where many cpus might enter this code. > > Really I do not think current code needs any 'refactoring', especially > around December 31th at midnight ;) > > > -- 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/icmp.c b/net/ipv4/icmp.c index 36e2697..4e5942d 100644 --- a/net/ipv4/icmp.c +++ b/net/ipv4/icmp.c @@ -252,22 +252,21 @@ static struct { */ bool icmp_global_allow(void) { - u32 credit, delta, incr = 0, now = (u32)jiffies; + u32 credit, incr = 0, now = (u32)jiffies; + u32 delta = now - icmp_global.stamp; bool rc = false; + /* Token bucket can be refilled every (HZ/50) jiffies if necessary. */ + bool cbr = delta >= HZ / 50; /* Check if token bucket is empty and cannot be refilled * without taking the spinlock. */ - if (!icmp_global.credit) { - delta = min_t(u32, now - icmp_global.stamp, HZ); - if (delta < HZ / 50) - return false; - } + if (!icmp_global.credit && !cbr) + return false; spin_lock(&icmp_global.lock); - delta = min_t(u32, now - icmp_global.stamp, HZ); - if (delta >= HZ / 50) { - incr = sysctl_icmp_msgs_per_sec * delta / HZ ; + if (cbr) { + incr = sysctl_icmp_msgs_per_sec * min_t(u32, delta, HZ) / HZ; if (incr) icmp_global.stamp = now; }
We can reduce the number of operations performed by icmp_global_allow and make the routine more readable by refactoring it in two ways: First, this patch refactors the meaning of the "delta" variable. Before this change, it meant min("time since last refill of token bucket", HZ). After this change, it means "time since last refill". The original definition is required only once but was being calculated twice. The new meaning is also more intuitive for a variable named "delta". Second, by calculating "delta" (time since last refill of token bucket) and "cbr" (token bucket can be refilled) at the beginning of the routine, we reduce the number of repeated calculations of these two variables. There should be no functional difference. Signed-off-by: Mike Danese <mikedanese@google.com> --- net/ipv4/icmp.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)