Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/ccn-lite: remove dependency on tlsf-malloc. #12043

Merged
merged 1 commit into from
Aug 21, 2019

Conversation

jcarrano
Copy link
Contributor

Contribution description

There is no reason why this package would need tlsf. Using tlsf as system malloc is not known to work in all platforms.

With this patch CCN-Lite will use the default malloc provided by the target's C library.

Testing procedure

The test procedure is described in the examples/ccn-lite-relay/README.md.

Issues/PRs references

Works around the need for #12021 .
See #12031 for an explanation of why it is not such a good idea to use TLSF as the system-wide malloc.

There is no reason why this package would need tlsf. Using tlsf as
system malloc is not known to work in all platforms.

With this patch CCN-Lite will use the default malloc provided by the
target's C library.
@jcarrano jcarrano added Area: pkg Area: External package ports Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 20, 2019
@kaspar030
Copy link
Contributor

ACK from my side. @OlegHahm do you remember why tlsf was used at all?

@cgundogan
Copy link
Member

Is it guaranteed that the provided libc implements malloc as well as free? IIRC, allocating memory was working, but free seemed always to be a problem. Did this change nowadays?

@jcarrano
Copy link
Contributor Author

@cgundogan AFAIK it was only a problem in MSP430, which only had a one-way malloc (this got fixed in #10944) and in any case I believe tlsf does not work in 16 bit.

Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this patch and did a couple of tests with ccn-lite, where I allocate/free thousands of name prefixes. I also exchanged a couple of Interest/Data messages (samr21-xpro). So, I guess the included libc makes tlsf obsolete. The only problem would arise for platforms without a proper dynamic allocation support, but since there is now code for MSP430, there doesn't seem to be many exotics left.

@cgundogan cgundogan added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Aug 21, 2019
@jcarrano
Copy link
Contributor Author

@cgundogan

tlsf obsolete.

It is still useful if somebody needs a O(1) allocator, which is to say never. I never really understood why TLSF was introduced when a more traditional malloc would have been easier to integrate and more space efficient. @gschorcht fixed the problem in msp430 by porting the avr-libc's malloc.

@jcarrano
Copy link
Contributor Author

In conclusion: I would not mind if TLSF is removed. I would not rush it though, as there is an intern here at FU working on tuning it for smaller devices and he might come up with interesting results.

@cgundogan
Copy link
Member

It is still useful if somebody needs a O(1) allocator

just as a side remark: in my tests, the dynamic memory de-/allocation of newlib was noticably faster than tlsf ..

@jcarrano
Copy link
Contributor Author

newlib was noticably faster than tlsf ..

The price to pay for lower asymptotic complexity is higher constant factors. If those benchmarks could be published (even in rough state, you could open a demonstrator pr) it may be useful for Antoine (who is working on tlsf).

@cgundogan
Copy link
Member

The price to pay for lower asymptotic complexity is higher constant factors.

Computational complexity depends on the algorithm at hand. If you refer to a proper space-time trade-off (like, e.g., with lookup-tables), then I don't know how this relates to my statement.

If those benchmarks could be published (even in rough state, you could open a demonstrator pr) it may be useful for Antoine (who is working on tlsf).

these weren't sophisticated memory benchmarks. I just tested whether ccn-lite was still working or not with your patch applied. I also re-discovered the malloc test application in the tests folder. You could add necessary probes before/after malloc/free for proper speed tests.

@kaspar030 was requesting feedback from @OlegHahm . Should we wait for an answer, before we merge?

@jcarrano
Copy link
Contributor Author

Computational complexity depends on the algorithm at hand.

Yes, I know. What I mean is that if there was a way to have O(1) performance and at the same time have that time be less (or even similar) than what a regular, double linked list malloc implementation takes on average, then everyone would be using TLSF.

@kaspar030 was requesting feedback from @OlegHahm .

No, just merge it.

@cgundogan cgundogan merged commit 83e092e into RIOT-OS:master Aug 21, 2019
@kaspar030
Copy link
Contributor

So, I guess the included libc makes tlsf obsolete.

Well, I wouldn't be so quick. The original paper shows that tlsf is half as fast as DL malloc on average, which newlib's malloc is based on. But the point of tlsf is that it uses constant time malloc() and free(), whereas DL malloc has unbounded worst cases.
The paper shows tlsf stays almost constant using the used test cases, being ~100 times faster than DL malloc for some.

Not interesting for ccn-lite? Do we know that this cannot be exploited remotely by feeding forged data (patterns) into the cache?

For RIOT in general, tlsf makes it actually possible to use somewhat dynamic allocation in real-time application.

@jcarrano
Copy link
Contributor Author

From the paper:

Therefore, in order to obtain experimentalresults that really show the performance parameters relevant to real-time applications, synthetic workload has been used

mmmh, I see, benchmarksmanship.

Pentium P4 2.8 Ghz machine with 512 KB of cache memoryand 512 MB of main memory.

just like the samr21.

Lets not forget how these paper things work- I won't throw the first stone, of course. The guys have to show some decent results and so it is expected they will tilt the balance to their side as much as they can.

Do we know that this cannot be exploited remotely by feeding forged data (patterns) into the cache?

No, we don't know, but that could be said of TLSF too. If I want to protect myself against a malicious peer I would trust newlib's malloc more, if only because it is more widely deployed and tested.

@cgundogan
Copy link
Member

cgundogan commented Aug 21, 2019

In addition to @jcarrano's comments: a typical ccn-lite setup would end up with newlib's malloc implementation plus tlsf. So, two dynamic allocators, meaning wasted ROM and RAM. For a proper use of tlsf, it must become the system allocator for RIOT, but that didn't work out so far.

EDIT: On second thought, tlsf is actually wrapping around malloc calls. I am not certain, however, how well the linker is able to kick out the unused newlib allocator.

@jcarrano
Copy link
Contributor Author

@cgundogan I would say the opposite: the strength of TLSF is that all the functions take an additional parameter to the heap being used. The "tlsf-malloc" module just calls these functions with a global heap pointer. If the library (CCN-lite in this case) allows one to provide a custom allocator then TLSF can be used only where needed. This has the additional advantage that the subsystem using a custom allocator cannot exhaust the system's memory. I used TLSF in that way in the Lua port.

Of course, this advantage is not inherent to TLSF.

I am not certain, however, how well the linker is able to kick out the unused newlib allocator.

By overwriting the symbols the linker should be able to garbage-collected them but only if the libc has been compiled in a way that allows this (the linker can only discard whole sections).

@cgundogan
Copy link
Member

This has the additional advantage that the subsystem using a custom allocator cannot exhaust the system's memory. I used TLSF in that way in the Lua port.

In which scenario is this an advantage? Since stack memory is pre-allocated on compile-time, what is the problem with allowing the heap to take on the rest of the RAM?

@cgundogan
Copy link
Member

The scenario I can think of is when two or more modules have strict memory needs and thus require local memory pools that are protected. Nevertheless .. the use cases can get quite esoteric and I do not see a certain need for CCN-lite to not use the system heap.

@kaspar030
Copy link
Contributor

Pentium P4 2.8 Ghz machine with 512 KB of cache memoryand 512 MB of main memory.
just like the samr21.

The tests have been done with <=2MB pool sizes. The DL malloc worst case even in only 256kB RAM.

No, we don't know, but that could be said of TLSF too.

Well, TLSF malloc() / free() have no loops. There is no O(n) case. Even I can verify that claim by looking for for and while clauses.

mmmh, I see, benchmarksmanship.

That sounds overly polemic. They showed that there are pathological cases for DL malloc that make it take 200 times longer than usual. They showed that TLSF delivered on being constant.

If an application doesn't need any guarantees on malloc()/free() time, fine (20k cycles is not that much). But if it does, using DL malloc is chosing the wrong algorithm. (reduce the number 20k until you arrive at a value that you admit random or forged input might bring in practice).

@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants