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

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

[Lucian Adrian Grijincu]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

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?

-- 
Lucian

[-- Attachment #2: score-9.patch --]
[-- Type: text/plain, Size: 643 bytes --]

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;

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

[Eric Dumazet]

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