[9fans] _threadmalloc() size>100000000; shouldn't be totalmalloc?

[9fans] _threadmalloc() size>100000000; shouldn't be totalmalloc?

[adr]

/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 > 100000000) {
                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.

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/Te1be8dc72738258d-M704d54262c512e9fce0f9fae
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

[9fans] Re: _threadmalloc() size>100000000; shouldn't be totalmalloc?

[adr]

On Sun, 26 Jun 2022, adr wrote:

> Date: Sun, 26 Jun 2022 09:50:19 +0000 (UTC)
> From: adr <adr@SDF.ORG>
> To: 9fans@9fans.net
> Subject: _threadmalloc() size>100000000; 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 > 100000000) {
>               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?

[Dan Cross]

On Tue, Jun 28, 2022 at 9:01 AM adr <adr@sdf.org> 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 > 100000000) {
> >               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?

[adr]

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 > 100000000) {
>>>               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?

[Dan Cross]

On Tue, Jun 28, 2022 at 10:22 AM adr <adr@sdf.org> 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?

[adr]

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?

[Dan Cross]

On Tue, Jun 28, 2022 at 11:38 AM adr <adr@sdf.org> 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