sys/netinet/cc: Switch from deprecated random() to prng32() #1162

hhartzer · 2024-04-10T21:55:46Z

Related: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277655

hhartzer · 2024-04-10T21:56:39Z

Hoping that someone with familiarity of these components can review this. I feel like it looks like an innocent enough set of changes, but I don't want to get something wrong here.

gmshake · 2024-04-11T01:25:23Z

CC @tuexen

bsdimp · 2024-04-12T17:05:41Z

Also cc @cemeyer @amotin @markjdb

These seem fine, but it looks like the range is changing for these (the range is accounted for, but I want more eyes on it).

amotin · 2024-04-12T17:27:29Z

sys/netinet/cc/cc_cdg.c

+		p = (UINT_MAX / (1 << EXP_PREC)) * probexp[idx];
+		backoff = (prng32() < p);


Why p is still int, not uint32_t or uint_t?

I do think that's a mistake. Should I do unit32_t for it?

Yes. I'd say p should be unsigned for correct comparison, and it should have the same range for the probability to work. So there should be uint32_t and UINT32_MAX.

Thank you for the feedback! I've adjusted p accordingly.

Thanks. But why did you leave UINT_MAX in its assignment instead of UINT32_MAX I proposed?

My bad! I think I missed that.

amotin · 2024-04-12T17:28:32Z

sys/netinet/cc/cc_chd.c

@@ -161,7 +162,7 @@ should_backoff(int qdly, int maxqdly, struct chd *chd_data)
 {
 	unsigned long p, rand;

-	rand = random();
+	rand = prng32();

 	if (qdly < V_chd_qthresh) {


I just wonder whether should_backoff() arguments here and below should really be signed, whether they makes sense and if so, whether it is handled right.

I also am not sure why these were signed. I don't know hardly a thing about the code in question. Was just trying to take a stab at removing the deprecated random() call where possible.

I don't know this code either, so it was just a thought. What you should to though is use UINT32_MAX for RANDOM_MAX (or just inline it) and uint32_t for p. rand variable I would just remove, inlining the prng32() call.

Thanks! I did most of what you suggested, but I found that the rand variable was referenced twice, so it seemed necessary to keep it assigned. Is that alright?

Oh, looks like RANDOM_MAX is still defined in the file. Should I remove it or reference it instead?

Is that alright?

Sure. I've just missed the other use. Sorry.

Either use RANDOM_MAX everywhere, or remove everywhere (in the two files for consistency). It is used only in the same small function as prng32(), so it is a bit obvious, but a bit confusing being declared many screens away.

hhartzer · 2024-04-25T22:38:27Z

I can update this but would prefer a little bit more guidance about how exactly it should be done, if that's possible.

bsdimp · 2024-05-16T10:03:20Z

@amotin I think @hhartzer needs some help with your suggestion

amotin · 2024-05-16T19:23:29Z

sys/netinet/cc/cc_cdg.c

@@ -308,7 +309,7 @@ cdg_cb_init(struct cc_var *ccv, void *ptr)
 	cdg_data->queue_state = CDG_Q_UNKNOWN;
 	cdg_data->maxrtt_in_rtt = 0;
 	cdg_data->maxrtt_in_prevrtt = 0;
-	cdg_data->minrtt_in_rtt = INT_MAX;
+	cdg_data->minrtt_in_rtt = UINT_MAX;


minrtt_in_rtt is just int, this change makes no sense to me, same as one below.

Oops! I am not sure why I did that. Good catch.

amotin · 2024-05-16T19:24:23Z

sys/netinet/cc/cc_cdg.c

+		p = (UINT_MAX / (1 << EXP_PREC)) * probexp[idx];
+		backoff = (prng32() < p);


Thanks. But why did you leave UINT_MAX in its assignment instead of UINT32_MAX I proposed?

amotin · 2024-05-16T19:30:20Z

sys/netinet/cc/cc_chd.c

@@ -161,7 +162,7 @@ should_backoff(int qdly, int maxqdly, struct chd *chd_data)
 {
 	unsigned long p, rand;

-	rand = random();
+	rand = prng32();

 	if (qdly < V_chd_qthresh) {


Is that alright?

Sure. I've just missed the other use. Sorry.

Either use RANDOM_MAX everywhere, or remove everywhere (in the two files for consistency). It is used only in the same small function as prng32(), so it is a bit obvious, but a bit confusing being declared many screens away.

amotin · 2024-05-16T20:30:59Z

sys/netinet/cc/cc_cdg.c

@@ -637,7 +639,7 @@ cdg_ack_received(struct cc_var *ccv, ccsignal_t ack_type)
 		}

 		cdg_data->minrtt_in_prevrtt = cdg_data->minrtt_in_rtt;
-		cdg_data->minrtt_in_rtt = INT_MAX;
+		cdg_data->minrtt_in_rtt = UINT_MAX;


"The one below"

amotin · 2024-05-16T20:31:30Z

sys/netinet/cc/cc_chd.c

-/* Largest possible number returned by random(). */
-#define	RANDOM_MAX	INT_MAX
+/* Largest possible number returned by prng32(). */
+#define	RANDOM_MAX	RANDOM_MAX


Cool define! ;)

Today is not my day! Thank you for being patient with me.

Fix stdint.h file not found. Fixes: 4b75afe

Related: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277655 Signed-off-by: henrichhartzer@tuta.io Reviewed by: imp, mav Pull Request: freebsd#1162

Related: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277655 Signed-off-by: henrichhartzer@tuta.io Reviewed by: imp, mav Pull Request: freebsd/freebsd-src#1162

amotin reviewed Apr 12, 2024

View reviewed changes

bsdimp added the changes-required label Apr 19, 2024

bsdimp added the needs-review label May 16, 2024

hhartzer force-pushed the sys-inet-cc-switch-from-random-to-prng32 branch from a3cc47d to bb3f867 Compare May 16, 2024 18:55

amotin reviewed May 16, 2024

View reviewed changes

hhartzer force-pushed the sys-inet-cc-switch-from-random-to-prng32 branch from bb3f867 to ba1e56d Compare May 16, 2024 20:27

amotin reviewed May 16, 2024

View reviewed changes

hhartzer force-pushed the sys-inet-cc-switch-from-random-to-prng32 branch from ba1e56d to f521b30 Compare May 16, 2024 20:51

amotin approved these changes May 16, 2024

View reviewed changes

amotin removed the changes-required label May 16, 2024

sys/netinet/icmp6.h: Fix build

380ee9b

Fix stdint.h file not found. Fixes: 4b75afe

bsdimp force-pushed the sys-inet-cc-switch-from-random-to-prng32 branch from f521b30 to 98b7bbf Compare May 23, 2024 21:09

sys/netinet/cc: Switch from deprecated random() to prng32()

674956e

Related: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277655 Signed-off-by: henrichhartzer@tuta.io Reviewed by: imp, mav Pull Request: freebsd#1162

bsdimp force-pushed the sys-inet-cc-switch-from-random-to-prng32 branch from 98b7bbf to 674956e Compare May 23, 2024 21:10

freebsd-git merged commit 674956e into freebsd:main May 23, 2024
7 of 9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/netinet/cc: Switch from deprecated random() to prng32() #1162

sys/netinet/cc: Switch from deprecated random() to prng32() #1162

hhartzer commented Apr 10, 2024

hhartzer commented Apr 10, 2024

gmshake commented Apr 11, 2024

bsdimp commented Apr 12, 2024

amotin Apr 12, 2024

hhartzer Apr 12, 2024

amotin May 16, 2024

hhartzer May 16, 2024

amotin May 16, 2024

hhartzer May 16, 2024

amotin Apr 12, 2024

hhartzer Apr 12, 2024

amotin May 16, 2024

hhartzer May 16, 2024

hhartzer May 16, 2024

amotin May 16, 2024

hhartzer commented Apr 25, 2024

bsdimp commented May 16, 2024

amotin May 16, 2024

hhartzer May 16, 2024

amotin May 16, 2024

amotin May 16, 2024

amotin May 16, 2024

hhartzer May 16, 2024

amotin May 16, 2024

hhartzer May 16, 2024

		p = (UINT_MAX / (1 << EXP_PREC)) * probexp[idx];
		backoff = (prng32() < p);

sys/netinet/cc: Switch from deprecated random() to prng32() #1162

sys/netinet/cc: Switch from deprecated random() to prng32() #1162

Conversation

hhartzer commented Apr 10, 2024

hhartzer commented Apr 10, 2024

gmshake commented Apr 11, 2024

bsdimp commented Apr 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhartzer commented Apr 25, 2024

bsdimp commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment