Re: request for testing: malloc and large allocations
On Fri, Jan 28, 2022 at 05:17:48PM +0100, Otto Moerbeek wrote:
> On Fri, Jan 28, 2022 at 04:33:28PM +0100, Alexander Bluhm wrote:
>
> > On Sun, Jan 09, 2022 at 02:54:43PM +0100, Otto Moerbeek wrote:
> > > currently malloc does cache a number of free'ed regions up to 128k in
> > > size. This cache is indexed by size (in # of pages), so it is very
> > > quick to check.
> > >
> > > Some programs allocate and deallocate larger allocations in a frantic
> > > way. Accodomate those programs by also keeping a cache of regions
> > > betwen 128k and 2M, in a cache of variable sized regions.
> > >
> > > My test case speeds up about twice. A make build gets a small speedup.
> > >
> > > This has been tested by myself on amd64 quite intensively. I am asking
> > > for more tests, especialy on more "exotic" platforms. I wil do arm64
> > > myself soon. Test can be running your favorite programs, doing make
> > > builds or running tests in regress/lib/libc/malloc.
> >
> > I see openssl and tmux crash with this diff.
> > /usr/src/regress/usr.sbin/openssl reproduces it on arm64, amd64,
> > i386.
>
> Are you running with any malloc flags?
This bug report enabled me to find a bug that would pop up if G mode
is enabled.
New diff below. New tests appreciated.
-Otto
Index: stdlib/malloc.c
===
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.272
diff -u -p -r1.272 malloc.c
--- stdlib/malloc.c 19 Sep 2021 09:15:22 - 1.272
+++ stdlib/malloc.c 31 Jan 2022 16:27:31 -
@@ -113,13 +113,27 @@ struct region_info {
LIST_HEAD(chunk_head, chunk_info);
-#define MAX_CACHEABLE_SIZE 32
-struct cache {
- void *pages[MALLOC_MAXCACHE];
+/*
+ * Two caches, one for "small" regions, one for "big".
+ * Small cache is an array per size, big cache is one array with different
+ * sized regions
+ */
+#define MAX_SMALLCACHEABLE_SIZE32
+#define MAX_BIGCACHEABLE_SIZE 512
+/* If the total # of pages is larger than this, evict before inserting */
+#define BIGCACHE_FILL(sz) (MAX_BIGCACHEABLE_SIZE * (sz) / 4)
+
+struct smallcache {
+ void **pages;
ushort length;
ushort max;
};
+struct bigcache {
+ void *page;
+ size_t psize;
+};
+
struct dir_info {
u_int32_t canary1;
int active; /* status of malloc */
@@ -139,7 +153,10 @@ struct dir_info {
void *delayed_chunks[MALLOC_DELAYED_CHUNK_MASK + 1];
u_char rbytes[32]; /* random bytes */
/* free pages cache */
- struct cache cache[MAX_CACHEABLE_SIZE];
+ struct smallcache smallcache[MAX_SMALLCACHEABLE_SIZE];
+ size_t bigcache_used;
+ size_t bigcache_size;
+ struct bigcache *bigcache;
#ifdef MALLOC_STATS
size_t inserts;
size_t insert_collisions;
@@ -207,7 +224,7 @@ struct malloc_readonly {
#ifdef MALLOC_STATS
int malloc_stats; /* dump statistics at end */
#endif
- u_int32_t malloc_canary;/* Matched against ones in malloc_pool
*/
+ u_int32_t malloc_canary;/* Matched against ones in pool */
};
/* This object is mapped PROT_READ after initialisation to prevent tampering */
@@ -714,18 +731,61 @@ unmap(struct dir_info *d, void *p, size_
size_t psz = sz >> MALLOC_PAGESHIFT;
void *r;
u_short i;
- struct cache *cache;
+ struct smallcache *cache;
if (sz != PAGEROUND(sz) || psz == 0)
wrterror(d, "munmap round");
- if (psz > MAX_CACHEABLE_SIZE || d->cache[psz - 1].max == 0) {
+ if (d->bigcache_size > 0 && psz > MAX_SMALLCACHEABLE_SIZE &&
+ psz <= MAX_BIGCACHEABLE_SIZE) {
+ u_short base = getrbyte(d);
+ u_short j;
+
+ /* don't look through all slots */
+ for (j = 0; j < d->bigcache_size / 4; j++) {
+ i = (base + j) % d->bigcache_size;
+ if (d->bigcache_used <
+ BIGCACHE_FILL(d->bigcache_size)) {
+ if (d->bigcache[i].psize == 0)
+ break;
+ } else {
+ if (d->bigcache[i].psize != 0)
+ break;
+ }
+ }
+ /* if we didn't find a preferred slot, use random one */
+ if (d->bigcache[i].psize != 0) {
+ size_t tmp;
+
+ r = d->bigcache[i].page;
+ d->bigcache_used -= d->bigcache[i].psize;
+ tmp = d->bigcache[i].psize << MALLOC_PAGESHIFT;
+ if (!mopts.malloc_freeunmap)
+ validate_junk(d, r, tmp);
+ if (munmap(r, tmp))
+wrterror(d, "munmap %p", r)
Re: Missing UBSan libs
Patrick Wildt writes: > regarding the missing userpace support: Since a few clang updates ago > we import more than just the builtins of compiler-rt. This means we > should have at least some related code in our tree, even if it is not > built/complete. From the recent static analyzer mail thread it looks > like people prefer to have such stuff in ports-clang, so, whatever. This may or may not be analogous. How hard is it to build a base/ports program with clang from ports? If it's a simple matter of make CC=/usr/local/bin/cc CFLAGS=-fsanitize=undefined then it makes little difference whether the base compiler includes the libraries for UBSan reporting. jca@ WDYT, should I first target devel/llvm to have UBSan working or go straight to /usr/src? Thanks Greg
Re: in4_cksum changes, step 1
> The register is set to -1, all bits set, and then its upper half
> is cleared. Now that the "and" has been removed, the value seems
> unused. The result of the removed "and" seemed unused too.
Oh, indeed, you're right, that makes things even simpler.
New diff:
Index: sys/arch/alpha/alpha/in_cksum.c
===
RCS file: /OpenBSD/src/sys/arch/alpha/alpha/in_cksum.c,v
retrieving revision 1.9
diff -u -p -r1.9 in_cksum.c
--- sys/arch/alpha/alpha/in_cksum.c 21 Aug 2014 14:24:08 - 1.9
+++ sys/arch/alpha/alpha/in_cksum.c 31 Jan 2022 19:39:56 -
@@ -200,7 +200,7 @@ in4_cksum(struct mbuf *m, u_int8_t nxt,
int clen = 0;
caddr_t addr;
union q_util q_util;
- union l_util l_util;
+ union l_util l_util;
struct ipovly ipov;
if (nxt != 0) {
@@ -212,14 +212,14 @@ in4_cksum(struct mbuf *m, u_int8_t nxt,
panic("in4_cksum: bad mbuf chain");
#endif
- memset(&ipov, 0, sizeof(ipov));
-
- ipov.ih_len = htons(len);
+ ipov.ih_x1[8] = 0;
ipov.ih_pr = nxt;
+ ipov.ih_len = htons(len);
ipov.ih_src = mtod(m, struct ip *)->ip_src;
ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
- sum += in_cksumdata((caddr_t) &ipov, sizeof(ipov));
+ /* first 8 bytes are zeroes */
+ sum += in_cksumdata((caddr_t) &ipov + 8, sizeof(ipov) - 8);
}
/* skip over unnecessary part */
@@ -241,7 +241,7 @@ in4_cksum(struct mbuf *m, u_int8_t nxt,
sum += in_cksumdata(addr, mlen) << 8;
else
sum += in_cksumdata(addr, mlen);
-
+
clen += mlen;
len -= mlen;
}
Index: sys/arch/m88k/m88k/in_cksum.c
===
RCS file: /OpenBSD/src/sys/arch/m88k/m88k/in_cksum.c,v
retrieving revision 1.4
diff -u -p -r1.4 in_cksum.c
--- sys/arch/m88k/m88k/in_cksum.c 21 Aug 2014 14:24:08 - 1.4
+++ sys/arch/m88k/m88k/in_cksum.c 31 Jan 2022 19:39:56 -
@@ -95,19 +95,22 @@ in4_cksum(struct mbuf *m, uint8_t nxt, i
{
u_int16_t *w;
u_int sum = 0;
- struct ipovly ipov;
+ union {
+ struct ipovly ipov;
+ u_int16_t w[10];
+ } u;
if (nxt != 0) {
/* pseudo header */
- bzero(&ipov, sizeof(ipov));
- ipov.ih_len = htons(len);
- ipov.ih_pr = nxt;
- ipov.ih_src = mtod(m, struct ip *)->ip_src;
- ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
- w = (u_int16_t *)&ipov;
- /* assumes sizeof(ipov) == 20 */
- sum += w[0]; sum += w[1]; sum += w[2]; sum += w[3]; sum += w[4];
- sum += w[5]; sum += w[6]; sum += w[7]; sum += w[8]; sum += w[9];
+ u.ipov.ih_x1[8] = 0;
+ u.ipov.ih_pr = nxt;
+ u.ipov.ih_len = htons(len);
+ u.ipov.ih_src = mtod(m, struct ip *)->ip_src;
+ u.ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
+ w = u.w;
+ /* assumes sizeof(ipov) == 20 and first 8 bytes are zeroes */
+ sum += w[4]; sum += w[5]; sum += w[6];
+ sum += w[7]; sum += w[8]; sum += w[9];
}
/* skip unnecessary part */
Index: sys/arch/powerpc/powerpc/in_cksum.c
===
RCS file: /OpenBSD/src/sys/arch/powerpc/powerpc/in_cksum.c,v
retrieving revision 1.10
diff -u -p -r1.10 in_cksum.c
--- sys/arch/powerpc/powerpc/in_cksum.c 22 Jul 2014 10:35:35 - 1.10
+++ sys/arch/powerpc/powerpc/in_cksum.c 31 Jan 2022 19:39:56 -
@@ -87,7 +87,7 @@ in_cksum_internal(struct mbuf *m, int of
* of a word spanning between this mbuf and the
* last mbuf.
*
-* s_util.c[0] is already saved when scanning previous
+* s_util.c[0] is already saved when scanning previous
* mbuf.
*/
s_util.c[1] = *w++;
@@ -254,15 +254,15 @@ in4_cksum(struct mbuf *m, uint8_t nxt, i
if (nxt != 0) {
/* pseudo header */
- memset(&u.ipov, 0, sizeof(u.ipov));
+ u.ipov.ih_x1[8] = 0;
+ u.ipov.ih_pr = nxt;
u.ipov.ih_len = htons(len);
- u.ipov.ih_pr = nxt;
- u.ipov.ih_src = mtod(m, struct ip *)->ip_src;
+ u.ipov.ih_src = mtod(m, struct ip *)->ip_src;
u.ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
w = u.w;
- /* assumes sizeof(ipov) == 20 */
- sum += w[0]; sum += w[1]; sum += w[2]; sum += w[3]; sum += w[4];
fdisk: Preserve BIOS boot partition with -A
This patch makes fdisk -A preserve BIOS boot partition.
When PolarFire SoC boots, the Hart Software Services (HSS) firmware
loads HSS payload that contains U-Boot (and possibly OpenSBI) from eMMC
or SD card.
If there is a valid GPT header, the payload is loaded from GPT partition
with type 21686148-6449-6e6f-744e-656564454649 (BIOS boot). If the
partition is not found, HSS tries loading from sector 0.
When installing OpenBSD, the HSS payload should be preserved.
Overwriting it would essentially brick the boot medium.
The GUID type 21686148-6449-6e6f-744e-656564454649 was originally (?)
used by GRUB. The HSS' reuse of the GUID feels unfortunate.
If this change is acceptable, the next step is to modify the installer.
Index: sbin/fdisk/fdisk.8
===
RCS file: src/sbin/fdisk/fdisk.8,v
retrieving revision 1.108
diff -u -p -r1.108 fdisk.8
--- sbin/fdisk/fdisk.8 26 Nov 2021 03:31:38 - 1.108
+++ sbin/fdisk/fdisk.8 31 Jan 2022 14:50:08 -
@@ -65,6 +65,7 @@ all existing partitions except the boot
.Sq APFS ISC ,
.Sq APFS ,
.Sq APFS Recovry ,
+.Sq BIOS Boot ,
.Sq HiFive FSBL
and
.Sq HiFive BBL .
Index: sbin/fdisk/part.c
===
RCS file: src/sbin/fdisk/part.c,v
retrieving revision 1.113
diff -u -p -r1.113 part.c
--- sbin/fdisk/part.c 27 Jan 2022 16:26:32 - 1.113
+++ sbin/fdisk/part.c 31 Jan 2022 14:50:08 -
@@ -152,7 +152,7 @@ const struct gpt_type gpt_types[] = {
{ 0x07, 0, "NTFS", "ebd0a0a2-b9e5-4433-87c0-68b6b72699c7" },
{ 0x0B, 0, "FAT32 ", "ebd0a0a2-b9e5-4433-87c0-68b6b72699c7" },
{ 0x0C, 0, "FAT32L ", "ebd0a0a2-b9e5-4433-87c0-68b6b72699c7" },
- { 0x0D, 0, "BIOS Boot ", "21686148-6449-6e6f-744e-656564454649" },
+ { 0x0D, 1, "BIOS Boot ", "21686148-6449-6e6f-744e-656564454649" },
{ 0x0E, 0, "FAT16L ", "ebd0a0a2-b9e5-4433-87c0-68b6b72699c7" },
{ 0x11, 0, "OS/2 hidden ", "ebd0a0a2-b9e5-4433-87c0-68b6b72699c7" },
{ 0x14, 0, "OS/2 hidden ", "ebd0a0a2-b9e5-4433-87c0-68b6b72699c7" },
Re: in4_cksum changes, step 1
On Sun, Jan 30, 2022 at 06:37:39PM +, Miod Vallat wrote:
> > > - sum += in_cksumdata((caddr_t) &ipov, sizeof(ipov));
> > > + sum += in_cksumdata((caddr_t) &ipov + 8, sizeof(ipov) - 8);
> >
> > I think this would be clearer with a comment.
>
> Sure, added one.
>
> > Please remove the trailing space that some of the changed lines have.
>
> Ok. Updated patch below.
>
> > > Index: sys/arch/sparc64/sparc64/in4_cksum.c
>
> > > + __asm volatile(
> > > + " lduw [%5 + 12], %1; "
> > > + " lduw [%5 + 16], %2; "
> > > " mov -1, %3; add %0, %1, %0; "
> > > " srl %3, 0, %3; add %0, %2, %0; "
> > > - " srlx %0, 32, %2; and %0, %3, %1; "
> > > + " srlx %0, 32, %2; "
> > > " add %0, %2, %0; "
> > > : "=r" (sum), "=&r" (tmp1), "=&r" (tmp2), "=&r" (tmp3)
> > > - : "0" (sum), "r" (w));
> > > + : "0" (sum), "r" (&ipov));
> >
> > I might be missing something, but is the temporary register %3 needed
> > at all?
>
> Yes, it is set to -1 and used for shifts, at the moment.
The register is set to -1, all bits set, and then its upper half
is cleared. Now that the "and" has been removed, the value seems
unused. The result of the removed "and" seemed unused too.
However, the revised patch looks good.
OK visa@
> Index: sys/arch/alpha/alpha/in_cksum.c
> ===
> RCS file: /OpenBSD/src/sys/arch/alpha/alpha/in_cksum.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 in_cksum.c
> --- sys/arch/alpha/alpha/in_cksum.c 21 Aug 2014 14:24:08 - 1.9
> +++ sys/arch/alpha/alpha/in_cksum.c 30 Jan 2022 18:35:18 -
> @@ -200,7 +200,7 @@ in4_cksum(struct mbuf *m, u_int8_t nxt,
> int clen = 0;
> caddr_t addr;
> union q_util q_util;
> - union l_util l_util;
> + union l_util l_util;
> struct ipovly ipov;
>
> if (nxt != 0) {
> @@ -212,14 +212,14 @@ in4_cksum(struct mbuf *m, u_int8_t nxt,
> panic("in4_cksum: bad mbuf chain");
> #endif
>
> - memset(&ipov, 0, sizeof(ipov));
> -
> - ipov.ih_len = htons(len);
> + ipov.ih_x1[8] = 0;
> ipov.ih_pr = nxt;
> + ipov.ih_len = htons(len);
> ipov.ih_src = mtod(m, struct ip *)->ip_src;
> ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
>
> - sum += in_cksumdata((caddr_t) &ipov, sizeof(ipov));
> + /* first 8 bytes are zeroes */
> + sum += in_cksumdata((caddr_t) &ipov + 8, sizeof(ipov) - 8);
> }
>
> /* skip over unnecessary part */
> @@ -241,7 +241,7 @@ in4_cksum(struct mbuf *m, u_int8_t nxt,
> sum += in_cksumdata(addr, mlen) << 8;
> else
> sum += in_cksumdata(addr, mlen);
> -
> +
> clen += mlen;
> len -= mlen;
> }
> Index: sys/arch/m88k/m88k/in_cksum.c
> ===
> RCS file: /OpenBSD/src/sys/arch/m88k/m88k/in_cksum.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 in_cksum.c
> --- sys/arch/m88k/m88k/in_cksum.c 21 Aug 2014 14:24:08 - 1.4
> +++ sys/arch/m88k/m88k/in_cksum.c 30 Jan 2022 18:35:18 -
> @@ -95,19 +95,22 @@ in4_cksum(struct mbuf *m, uint8_t nxt, i
> {
> u_int16_t *w;
> u_int sum = 0;
> - struct ipovly ipov;
> + union {
> + struct ipovly ipov;
> + u_int16_t w[10];
> + } u;
>
> if (nxt != 0) {
> /* pseudo header */
> - bzero(&ipov, sizeof(ipov));
> - ipov.ih_len = htons(len);
> - ipov.ih_pr = nxt;
> - ipov.ih_src = mtod(m, struct ip *)->ip_src;
> - ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
> - w = (u_int16_t *)&ipov;
> - /* assumes sizeof(ipov) == 20 */
> - sum += w[0]; sum += w[1]; sum += w[2]; sum += w[3]; sum += w[4];
> - sum += w[5]; sum += w[6]; sum += w[7]; sum += w[8]; sum += w[9];
> + u.ipov.ih_x1[8] = 0;
> + u.ipov.ih_pr = nxt;
> + u.ipov.ih_len = htons(len);
> + u.ipov.ih_src = mtod(m, struct ip *)->ip_src;
> + u.ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
> + w = u.w;
> + /* assumes sizeof(ipov) == 20 and first 8 bytes are zeroes */
> + sum += w[4]; sum += w[5]; sum += w[6];
> + sum += w[7]; sum += w[8]; sum += w[9];
> }
>
> /* skip unnecessary part */
> Index: sys/arch/powerpc/powerpc/in_cksum.c
> ===
> RCS file: /OpenBSD/src/sys/arch/powerpc/powerpc/in_cksum.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 in_cksum.c
> --- sys/arch/powerpc/powerpc/in_cksum.c 22 Jul 2014 10:35:35 -
Re: uvm_unmap_kill_entry(): unwire with map lock held
On 31/01/22(Mon) 10:24, Klemens Nanni wrote:
> Running with my uvm assert diff showed that uvm_fault_unwire_locked()
> was called without any locks held.
>
> This happened when I rebooted my machine:
>
> uvm_fault_unwire_locked()
> uvm_unmap_kill_entry_withlock()
> uvm_unmap_kill_entry()
> uvm_map_teardown()
> uvmspace_free()
>
> This code does not corrupt anything because
> uvm_unmap_kill_entry_withlock() is grabbing the kernel lock aorund its
> uvm_fault_unwire_locked() call.
>
> But regardless of the kernel lock dances in this code path, the uvm map
> ought to be protected by its own lock. uvm_fault_unwire() does that.
>
> uvm_fault_unwire_locked()'s comment says the map must at least be read
> locked, which is what all other code paths to that function do.
>
> This makes my latest assert diff happy in the reboot case (it did not
> always hit that assert).
I'm happy your asserts found a first bug.
I"m afraid calling this function below could result in a grabbing the
lock twice. Can we be sure this doesn't happen?
uvm_unmap_kill_entry() is called in many different contexts and this is
currently a mess. I don't know what NetBSD did in this area but it is
worth looking at and see if there isn't a good idea to untangle this.
> Index: uvm_map.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> retrieving revision 1.282
> diff -u -p -r1.282 uvm_map.c
> --- uvm_map.c 21 Dec 2021 22:21:32 - 1.282
> +++ uvm_map.c 31 Jan 2022 09:28:04 -
> @@ -2132,7 +2143,7 @@ uvm_unmap_kill_entry_withlock(struct vm_
> if (VM_MAPENT_ISWIRED(entry)) {
> KERNEL_LOCK();
> entry->wired_count = 0;
> - uvm_fault_unwire_locked(map, entry->start, entry->end);
> + uvm_fault_unwire(map, entry->start, entry->end);
> KERNEL_UNLOCK();
> }
>
>
uvm_unmap_kill_entry(): unwire with map lock held
Running with my uvm assert diff showed that uvm_fault_unwire_locked()
was called without any locks held.
This happened when I rebooted my machine:
uvm_fault_unwire_locked()
uvm_unmap_kill_entry_withlock()
uvm_unmap_kill_entry()
uvm_map_teardown()
uvmspace_free()
This code does not corrupt anything because
uvm_unmap_kill_entry_withlock() is grabbing the kernel lock aorund its
uvm_fault_unwire_locked() call.
But regardless of the kernel lock dances in this code path, the uvm map
ought to be protected by its own lock. uvm_fault_unwire() does that.
uvm_fault_unwire_locked()'s comment says the map must at least be read
locked, which is what all other code paths to that function do.
This makes my latest assert diff happy in the reboot case (it did not
always hit that assert).
Index: uvm_map.c
===
RCS file: /cvs/src/sys/uvm/uvm_map.c,v
retrieving revision 1.282
diff -u -p -r1.282 uvm_map.c
--- uvm_map.c 21 Dec 2021 22:21:32 - 1.282
+++ uvm_map.c 31 Jan 2022 09:28:04 -
@@ -2132,7 +2143,7 @@ uvm_unmap_kill_entry_withlock(struct vm_
if (VM_MAPENT_ISWIRED(entry)) {
KERNEL_LOCK();
entry->wired_count = 0;
- uvm_fault_unwire_locked(map, entry->start, entry->end);
+ uvm_fault_unwire(map, entry->start, entry->end);
KERNEL_UNLOCK();
}
