From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: vmstat(8): get rid of KERN_MALLOC_BUCKETS sysctl(2) call
To: tech@openbsd.org
Date: Tue, 14 Jan 2025 15:33:41 +0300

On Tue, Jan 14, 2025 at 01:14:30PM +0100, Claudio Jeker wrote:
> On Tue, Jan 14, 2025 at 02:08:38PM +0300, Vitaliy Makkoveev wrote:
> > claudio@ pointed that KERN_MALLOC_BUCKETS aka kern.malloc.buckets is just
> > the dummy string representation of (1 << i) for MINBUCKET to MINBUCKET +
> > 16 integers, so no reason to keep it in kernel. Also tb@ pointed, that
> > Debian code search has no hits, so it's probably unused.
> > 
> > Let's try to remove it. Our vmstat(8) and systat(a) are the only
> > userland users and they could generate desired bucket names by them self
> > instead of KERN_MALLOC_BUCKETS strtonum(3) parsing.
> > 
> > This diff converts vmstat(8). I'll convert systat(1) separately if this
> > conversion will be accepted.
> > 
> > Note, MINBUCKET and MAXBUCKET are defined in kernel headers, the
> > "MINBUCKET + 16" is already hard-coded to vmstat.c.
> > 
> > Note, sysctl_malloc() has the similar KERN_MALLOC_KMEMNAMES string,
> > which is unused in our userland and also was not found by Debian code
> > search.
> 
> One thing this breaks is -M / -N support for kernels that have a different
> MINBUCKET define. Do we care about that?
>  

It is already "broken" because both vmstat and systat use MINBUCKET
define instead of dynamic calculation. They should be recompiled if
MINBUCKET was changed in <sys/malloc.h>.

> > Index: usr.bin/vmstat/vmstat.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/vmstat/vmstat.c,v
> > retrieving revision 1.158
> > diff -u -p -r1.158 vmstat.c
> > --- usr.bin/vmstat/vmstat.c	1 May 2024 12:54:27 -0000	1.158
> > +++ usr.bin/vmstat/vmstat.c	14 Jan 2025 10:39:58 -0000
> > @@ -735,43 +735,22 @@ domem(void)
> >  	struct kmemstats kmemstats[M_LAST], *ks;
> >  	int i, j, len, size, first, mib[4];
> >  	u_long totuse = 0, totfree = 0;
> > -	char buf[BUFSIZ], *bufp, *ap;
> >  	unsigned long long totreq = 0;
> >  	const char *name;
> >  	size_t siz;
> >  
> > -	if (memf == NULL && nlistf == NULL) {
> > -		mib[0] = CTL_KERN;
> > -		mib[1] = KERN_MALLOCSTATS;
> > -		mib[2] = KERN_MALLOC_BUCKETS;
> > -		siz = sizeof(buf);
> > -		if (sysctl(mib, 3, buf, &siz, NULL, 0) == -1) {
> > -			warnx("could not read kern.malloc.buckets");
> > -			return;
> > -		}
> > +	mib[0] = CTL_KERN;
> > +	mib[1] = KERN_MALLOCSTATS;
> > +	mib[2] = KERN_MALLOC_BUCKET;
> > +	siz = sizeof(struct kmembuckets);
> >  
> > -		bufp = buf;
> > -		mib[2] = KERN_MALLOC_BUCKET;
> > -		siz = sizeof(struct kmembuckets);
> > -		i = 0;
> > -		while ((ap = strsep(&bufp, ",")) != NULL) {
> > -			const char *errstr;
> > +	for (i = MINBUCKET; i < MINBUCKET + 16; i++) {
> > +		mib[3] = (1<<i);
> >  
> > -			mib[3] = strtonum(ap, 0, INT_MAX, &errstr);
> > -			if (errstr) {
> > -				warnx("kernel lied about %d being a number", mib[3]);
> > -				return;
> > -			}
> > -
> > -			if (sysctl(mib, 4, &buckets[MINBUCKET + i], &siz,
> > -			    NULL, 0) == -1) {
> > -				warn("could not read kern.malloc.bucket.%d", mib[3]);
> > -				return;
> > -			}
> > -			i++;
> > +		if (sysctl(mib, 4, &buckets[i], &siz, NULL, 0) == -1) {
> > +			warn("could not read kern.malloc.bucket.%d", mib[3]);
> > +			return;
> >  		}
> > -	} else {
> > -		kread(X_KMEMBUCKETS, buckets, sizeof(buckets));
> >  	}
> >  
> >  	for (first = 1, i = MINBUCKET, kp = &buckets[i]; i < MINBUCKET + 16;
> 
> -- 
> :wq Claudio
>