[9fans] Re: _threadmalloc() size>100000000; shouldn't be totalmalloc?
On Sun, 26 Jun 2022, adr wrote: Date: Sun, 26 Jun 2022 09:50:19 + (UTC) From: adr To: 9fans@9fans.net Subject: _threadmalloc() size>1; shouldn't be totalmalloc? /sys/src/libthread/lib.c [...] void* _threadmalloc(long size, int z) { void *m; m = malloc(size); if (m == nil) sysfatal("Malloc of size %ld failed: %r", size); setmalloctag(m, getcallerpc(&size)); totalmalloc += size; if (size > 1) { fprint(2, "Malloc of size %ld, total %ld\n", size, totalmalloc); abort(); } if (z) memset(m, 0, size); return m; } [...] Shouldn't the if statement test the size of totalmalloc before the abort? That size, 100M for internal allocations? It has to be totalmalloc, doesn't it? If not this if statement should be after testing the success of the malloc. Am I missing something? adr. I mean, I think using libthread more like using small channels and thread's stack sizes, but: Why put a limit here to size when totalmalloc could continue to grow until malloc fails? If we want a limit, why don't we take into account the system resources? If we want a constant limit, why don't we put it on threadimpl.h, explain why this value in a comment and document it in thread(2)? Why bother to call malloc before testing if size exeeds that limit??? adr -- 9fans: 9fans Permalink: https://9fans.topicbox.com/groups/9fans/Te1be8dc72738258d-Mf7cdc7010affd696392c0b47 Delivery options: https://9fans.topicbox.com/groups/9fans/subscription
Re: [9fans] Re: _threadmalloc() size>100000000; shouldn't be totalmalloc?
On Tue, Jun 28, 2022 at 9:01 AM adr wrote: > On Sun, 26 Jun 2022, adr wrote: > > [snip] > > /sys/src/libthread/lib.c > > > > [...] > > void* > > _threadmalloc(long size, int z) > > { > > void *m; > > > > m = malloc(size); > > if (m == nil) > > sysfatal("Malloc of size %ld failed: %r", size); > > setmalloctag(m, getcallerpc(&size)); > > totalmalloc += size; > > if (size > 1) { > > fprint(2, "Malloc of size %ld, total %ld\n", size, > > totalmalloc); > > abort(); > > } > > if (z) > > memset(m, 0, size); > > return m; > > } > > [...] > > > > Shouldn't the if statement test the size of totalmalloc before the > > abort? That size, 100M for internal allocations? It has to be > > totalmalloc, doesn't it? If not this if statement should be after > > testing the success of the malloc. Am I missing something? Note that the `if` statement doesn't test the size of *totalmalloc*, but just `size`. `totalsize` is only incidentally printed in the output if `size` exceeds 100 MB: that is, if a single allocation is larger than 100MB. > I mean, I think using libthread more like using small channels and thread's > stack sizes, but: > > Why put a limit here to size when totalmalloc could continue to grow until > malloc fails? > > If we want a limit, why don't we take into account the system resources? > > If we want a constant limit, why don't we put it on threadimpl.h, explain why > this value in a comment > and document it in thread(2)? > > Why bother to call malloc before testing if size exeeds that limit??? Given the name of the function (`_threadmalloc`), I'd guess that this isn't intended for general use, but rather, for the internal consumption of the thread library, where indeed such a large allocation would likely be an error (bear in mind this code was developed on 32-bit machines with RAMs measured in Megabytes, not Gigabytes). As for why the abort is after the allocation? The sad answer, as with so many things in plan 9, is that there probably isn't a particularly good reason. A possible answer may be that this leaves the process in a state where the allocations can be inspected in a debugger after the abort, complete with a proper allocation tag. An equally plausible answer is that it's just the way the original author typed it in. Please understand that Plan 9 spent most of its life as a research system, and the code and its quality reflects that. - Dan C. -- 9fans: 9fans Permalink: https://9fans.topicbox.com/groups/9fans/Te1be8dc72738258d-M6c48d66d6cc15f19fd28066c Delivery options: https://9fans.topicbox.com/groups/9fans/subscription
Re: [9fans] Re: _threadmalloc() size>100000000; shouldn't be totalmalloc?
On Tue, 28 Jun 2022, Dan Cross wrote: [...] void* _threadmalloc(long size, int z) { void *m; m = malloc(size); if (m == nil) sysfatal("Malloc of size %ld failed: %r", size); setmalloctag(m, getcallerpc(&size)); totalmalloc += size; if (size > 1) { fprint(2, "Malloc of size %ld, total %ld\n", size, totalmalloc); abort(); } if (z) memset(m, 0, size); return m; } [...] Shouldn't the if statement test the size of totalmalloc before the abort? That size, 100M for internal allocations? It has to be totalmalloc, doesn't it? If not this if statement should be after testing the success of the malloc. Am I missing something? Note that the `if` statement doesn't test the size of *totalmalloc*, but just `size`. `totalsize` is only incidentally printed in the output if `size` exceeds 100 MB: that is, if a single allocation is larger than 100MB. I know, what called my attention is that size wasn't tested before calling malloc, so the first thought I had was that the code had a typo and it should be testing totalmalloc instead. I mean, I think using libthread more like using small channels and thread's stack sizes, but: Why put a limit here to size when totalmalloc could continue to grow until malloc fails? If we want a limit, why don't we take into account the system resources? If we want a constant limit, why don't we put it on threadimpl.h, explain why this value in a comment and document it in thread(2)? Why bother to call malloc before testing if size exeeds that limit??? Given the name of the function (`_threadmalloc`), I'd guess that this isn't intended for general use, but rather, for the internal consumption of the thread library, where indeed such a large allocation would likely be an error (bear in mind this code was developed on 32-bit machines with RAMs measured in Megabytes, not Gigabytes). No, it's used also when creating a channel and setting a thread's stack size. adr -- 9fans: 9fans Permalink: https://9fans.topicbox.com/groups/9fans/Te1be8dc72738258d-Mcbc484781129f693c26365ef Delivery options: https://9fans.topicbox.com/groups/9fans/subscription
Re: [9fans] Re: _threadmalloc() size>100000000; shouldn't be totalmalloc?
On Tue, Jun 28, 2022 at 10:22 AM adr wrote: > On Tue, 28 Jun 2022, Dan Cross wrote: > > [snip] > > Given the name of the function (`_threadmalloc`), I'd guess that this isn't > > intended for general use, but rather, for the internal consumption of the > > thread library, where indeed such a large allocation would likely be an > > error (bear in mind this code was developed on 32-bit machines with RAMs > > measured in Megabytes, not Gigabytes). > > No, it's used also when creating a channel and setting a thread's > stack size. You mean by `newthread` and `chancreate`? Those are part of the thread library. Notice that there are no callers outside of /sys/src/libthread. - Dan C. -- 9fans: 9fans Permalink: https://9fans.topicbox.com/groups/9fans/Te1be8dc72738258d-M5755b12d8fc8f5fc46f51c97 Delivery options: https://9fans.topicbox.com/groups/9fans/subscription
Re: [9fans] Re: _threadmalloc() size>100000000; shouldn't be totalmalloc?
On Tue, 28 Jun 2022, Dan Cross wrote: You mean by `newthread` and `chancreate`? Those are part of the thread library. Notice that there are no callers outside of /sys/src/libthread. What I mean is that "size" in _threadmalloc() will be set by those functions with values directly given by the programmer, with this limit not documented. I wouldn't call a function wich is part of an api internal. An internal function, for me, is a function inaccesible for the programmer, like _threadmalloc itself. By the way, you mean threadcreate, don't you? Regars, adr. -- 9fans: 9fans Permalink: https://9fans.topicbox.com/groups/9fans/Te1be8dc72738258d-M0c09e2ed1ced2383c89f1fe9 Delivery options: https://9fans.topicbox.com/groups/9fans/subscription
Re: [9fans] Re: _threadmalloc() size>100000000; shouldn't be totalmalloc?
On Tue, Jun 28, 2022 at 11:38 AM adr wrote: > On Tue, 28 Jun 2022, Dan Cross wrote: > > You mean by `newthread` and `chancreate`? Those are part of the > > thread library. Notice that there are no callers outside of > > /sys/src/libthread. > > What I mean is that "size" in _threadmalloc() will be set by those > functions with values directly given by the programmer, with this > limit not documented. Like I said earlier, plan9 had the luxury of being a research system. It has brilliant ideas, but mostly trapped inside of research-quality code. If you look just below the surface, there are arbitrary limits and edge cases all over the system. It's honestly surprising that it works as well as it does. That this particular limit is not documented isn't terribly surprising. Most of these limits are undocumented. I doubt anyone ever thought to create a 100MB stack or channel when that code was written. > I wouldn't call a function wich is part of an api internal. An > internal function, for me, is a function inaccesible for the > programmer, like _threadmalloc itself. > > By the way, you mean threadcreate, don't you? No, I meant the direct calls to `_threadmalloc`. But sure, we can say `theadcreate` since that just expands to a call to `newthead` and `newthread` is static. We may as well throw `proccreate` into the mix too as it also indirectly calls `_threadmalloc` via `_newproc`. For that matter, libthead's `main` also calls `_threadmalloc`. I'm not sure if that changes the point, though. - Dan C. -- 9fans: 9fans Permalink: https://9fans.topicbox.com/groups/9fans/Te1be8dc72738258d-M71941c1e8258c2d786a38352 Delivery options: https://9fans.topicbox.com/groups/9fans/subscription