From: David Gwynne <david@gwynne.id.au>
Subject: Re: dhcpd(8): don't avoid free()
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Sat, 31 May 2025 11:48:24 +1000

On Fri, May 30, 2025 at 07:43:37AM +0200, Theo Buehler wrote:
> On Fri, May 30, 2025 at 03:28:54PM +1000, David Gwynne wrote:
> > dhcpd seems to keep it's own free lists for a couple of data
> > structures.  my best guess for why it would do this is to avoid
> > malloc overhead.  however, responding to dhcp packets is not what
> > i would consider a webscale problem^W^W^Wperformance critical, so
> > this optimisation is unecessary.
> > 
> > ok?
> 
> ok
> 
> Are you going to remove the free_timeouts list as well?

like this?

Index: dispatch.c
===================================================================
RCS file: /cvs/src/usr.sbin/dhcpd/dispatch.c,v
diff -u -p -r1.45 dispatch.c
--- dispatch.c	2 Sep 2023 10:18:45 -0000	1.45
+++ dispatch.c	31 May 2025 01:44:48 -0000
@@ -72,7 +72,6 @@ extern int syncfd;
 struct interface_info *interfaces;
 struct protocol *protocols;
 struct dhcpd_timeout *timeouts;
-static struct dhcpd_timeout *free_timeouts;
 static int interfaces_invalidated;
 
 static int interface_status(struct interface_info *ifinfo);
@@ -331,8 +330,7 @@ another:
 				struct dhcpd_timeout *t = timeouts;
 				timeouts = timeouts->next;
 				(*(t->func))(t->what);
-				t->next = free_timeouts;
-				free_timeouts = t;
+				free(t);
 				goto another;
 			}
 
@@ -543,18 +541,11 @@ add_timeout(time_t when, void (*where)(v
 	/* If we didn't supersede a timeout, allocate a timeout
 	   structure now. */
 	if (!q) {
-		if (free_timeouts) {
-			q = free_timeouts;
-			free_timeouts = q->next;
-			q->func = where;
-			q->what = what;
-		} else {
-			q = malloc(sizeof (struct dhcpd_timeout));
-			if (!q)
-				fatalx("Can't allocate timeout structure!");
-			q->func = where;
-			q->what = what;
-		}
+		q = malloc(sizeof (struct dhcpd_timeout));
+		if (!q)
+			fatalx("Can't allocate timeout structure!");
+		q->func = where;
+		q->what = what;
 	}
 
 	q->when = when;
@@ -595,15 +586,10 @@ cancel_timeout(void (*where)(void *), vo
 				t->next = q->next;
 			else
 				timeouts = q->next;
+			free(q);
 			break;
 		}
 		t = q;
-	}
-
-	/* If we found the timeout, put it on the free list. */
-	if (q) {
-		q->next = free_timeouts;
-		free_timeouts = q;
 	}
 }