udp: break from the lookup when hitting the maximum score value

Submitted by Lucian Adrian Grijincu on Oct. 23, 2009, 4:36 p.m.

Details

Message ID 200910231936.16022.lgrijincu@ixiacom.com
State Rejected
Delegated to: David Miller
Headers show

Commit Message

Lucian Adrian Grijincu Oct. 23, 2009, 4:36 p.m.
Before udp hashes were converted to rcu in
	udp: introduce struct udp_table and multiple spinlocks
	645ca708f936b2fbeb79e52d7823e3eb2c0905f8
we stopped searching in list upon hitting the maximum score value (which is 
9).

This got removed in the conversion to rcu.
I'm not sure whether this was intentional or it just slipped by.

As far as I understand it this does not interfere with the lockless rcu: there 
is another score check the result will have to pass and if it doesn't have a 
score of 9 (which will be the value of badness) we'll just restart the lookup.

Even if the node was deleted from the chain and reclaimed at a later time, if 
at the second score test we have value 9 again, we can still return with this 
result.

Am I missing something?

Comments

Eric Dumazet Oct. 23, 2009, 4:57 p.m.
Lucian Adrian Grijincu a écrit :
> Before udp hashes were converted to rcu in
> 	udp: introduce struct udp_table and multiple spinlocks
> 	645ca708f936b2fbeb79e52d7823e3eb2c0905f8
> we stopped searching in list upon hitting the maximum score value (which is 
> 9).
> 
> This got removed in the conversion to rcu.
> I'm not sure whether this was intentional or it just slipped by.
> 
> As far as I understand it this does not interfere with the lockless rcu: there 
> is another score check the result will have to pass and if it doesn't have a 
> score of 9 (which will be the value of badness) we'll just restart the lookup.
> 
> Even if the node was deleted from the chain and reclaimed at a later time, if 
> at the second score test we have value 9 again, we can still return with this 
> result.
> 
> Am I missing something?
> 
> 

This was intentional.

This never happens in practice and slowdown lookups.

(To reach score 9, your UDP socket must be connected, and bound to a device)
Most developpers dont even know UDP socket can be connected...

We added large hashtables in commit f86dcc5aa8c7908f2c287e7a211228df599e3e71
(udp: dynamically size hash tables at boot time), so average chain length should
be small anyway...



--
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

Patch hide | download patch | download mbox

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index d0d436d..69464b9 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -294,6 +294,11 @@  begin:
 	sk_nulls_for_each_rcu(sk, node, &hslot->head) {
 		score = compute_score(sk, net, saddr, hnum, sport,
 				      daddr, dport, dif);
+		if (score == 9) {
+			result = sk;
+			badness = score;
+			goto skip_end_of_nulls_check;
+		}
 		if (score > badness) {
 			result = sk;
 			badness = score;
@@ -307,6 +312,7 @@  begin:
 	if (get_nulls_value(node) != hash)
 		goto begin;
 
+skip_end_of_nulls_check:
 	if (result) {
 		if (unlikely(!atomic_inc_not_zero(&result->sk_refcnt)))
 			result = NULL;