pkg/tlsf: revert to original api #9006

jcarrano · 2018-04-23T14:57:40Z

Refactor and cleanup of the TLSF package

The TLSF package has a big patch that amongst other things, changes the TLSF api and removes the possibility to have private heaps. It also adds an implementation of the system malloc, that is not being documented because it lives inside a diff.

This PR:

removes the big patch
cleans up the package Makefiles:
- It pulls the external code directly from git.
- The patch is applied by makefile magic
implements the malloc code in a contrib package.

The original TLSF code is left practically unmodified. The only usage of TLSF in core riot is in the CCN example, which I have tested and still works.

miri64 · 2018-04-23T15:01:08Z

pkg/tlsf/patches/0001-Fix-warnining-on-implicit-pointer-conversion.patch

+ {
+ 	(void)user;
+-	printf("\t%p %s size: %x (%p)\n", ptr, used ? "used" : "free", (unsigned int)size, block_from_ptr(ptr));
+	printf("\t%p %s size: %x (%p)\n", ptr, used ? "used" : "free", (unsigned int)size, (void *)block_from_ptr(ptr));


I think this small fix is something we should try to get upstream.

yes, I'll try to do that.

Done: mattconte/tlsf#5

jcarrano · 2018-04-23T15:06:13Z

Travis is failing in the CPP check for a line number I didn't modify.

cgundogan · 2018-04-23T15:07:08Z

examples/ccn-lite-relay/Makefile


+USEPKG += tlsf


You could model tlsf as a dependency for tlsf-malloc. Using tlsf-malloc without tlsf doesn't make much sense, or am I missing something? This way, we could skip the explicit USEPKG += tlsf statement.

--

Just saw that you wrote USEPKG += tlsf in pkg/tlsf/contrib/Makefile.dep, maybe that's eough? Can we drop the explicit USEPKG += tlsf?

That's what I tried to do. However, the +DIR is in tlsf's Makefile.include and I think it doesn't get read if the USEPKG is not there. It's tricky because the contrib is a module that lives inside a package, I'm not sure if it can be done "the right way".

@jcarrano I would rather have this line removed. We can achieve that by setting a dependency in Makefile.dep:

ifneq (,$(filter tlsf-malloc,$(USEMODULE))) USEPKG += tlsf endif

This resolves the dependencies in the correct order.

That does not work. The Makefiles in the tlsf directory only get read if USEPKG += tlsf. In the other packages that have contrib directorties, the contib gets added to USEMODULE always if the parent package is used.
It is a limitation of the current build system: if a package has an optional submodule, then the package and the module must be explicitly selected.

But when I apply my proposed code snipped, then it still builds and executes for me..

You have to put it around line 726, before the call to

-include $(USEPKG:%=$(RIOTPKG)/%/Makefile.dep)

where the USEPKG statements are resolved

In case it wasn't clear enough: I was talking about the main Makefile.dep in the RIOT root

Ok, I didn't know that file existed. I wish I could keep all of TLSF inside pkg/tlsf without having to touch global files, but I'll do it anyways.

I wish I could keep all of TLSF inside pkg/tlsf without having to touch global files, but I'll do it anyways.

I understand your motivation, but it looks nicer and is less error prone if you only have to use USEMODULE += tlsf-malloc in the user application instead of also specifying the package (:
And we already have this global depencendy file .. so let's make use of it.

jia200x · 2018-04-23T15:39:26Z

I like the idea of using TLSF not only as a malloc-like implentation.

What I'm not sure is the implementation in tlsf_g* and memory management functions aliases. Having multiple global memory allocators seems to be a rare scenario IMO, considering the fact these pkgs add their fingerprint in memory consumption (TLSF is around 3 kb).

What could be is a global allocator and another functionality that strictly requires an isolated heap. But a global malloc implementation still allows to use tlsf_* functions directly from the pkg.

That way, USEPKG += tlsf-malloc implements the stdlib functions with TLSF, but there could be cases where only "tlsf" is required.

What do you think? @jcarrano @miri64

jcarrano · 2018-04-23T15:55:01Z

@jia200x I did the global allocator thing because that is what was already done, so I supposed there was a good reason (and maybe there is, the system allocator may be better on average than TLSF, but TLSF has a predictable O(1) performance.

I agree that there's probably no reason to have both the original malloc and tlsf_gmalloc, but the original code allowed the user to rename the global memory allocators by a define that was turned into a prefix:

#ifndef TLSF_MALLOC_PREFIX
#   define TLSF_MALLOC_PREFIX
#endif
#define __TLSF_MALLOC_NAME(A, B) A ## B
#define _TLSF_MALLOC_NAME(A, B) __TLSF_MALLOC_NAME(A, B)
#define TLSF_MALLOC_NAME(NAME) _TLSF_MALLOC_NAME(TLSF_MALLOC_PREFIX, NAME)

void *TLSF_MALLOC_NAME(malloc)(size_t bytes);

jia200x · 2018-04-23T16:06:03Z

@jia200x I did the global allocator thing because that is what was already done, so I supposed there was a good reason (and maybe there is, the system allocator may be better on average than TLSF, but TLSF has a predictable O(1) performance.

Yes, I meant to leave these functions with their stdlib name (malloc, free, etc) rather than these tlsf_g* names.

Basically is "implementing missing stdlib from a pkg" rather than "implementing a global allocator that points by default to stdlib". The first approach is only a turn on/off in Makefiles.

I'm aware this behavior comes from the original implementation.

What do you think on leaving it this way?

jia200x · 2018-04-23T16:13:40Z

e.g this one adds malloc support for msp430. The stdlib functions are implemented here

jcarrano · 2018-04-23T16:35:25Z

@jia200x I think that what you are saying makes more sense. It also simplifies the code. I'm not sure I'd add the weak attribute like oneway malloc does, because generally one would not want to override TLSF.

As an aside comment, after seeing this definition, I was trying to avoid __attribute__ because I thought that it could bring trouble with compilers that do not support it.

jia200x · 2018-04-23T16:37:29Z

@jcarrano yes, I agree with you. I would leave it as "strong" definition, since the TLSF malloc implementation would have the same weight as other global implementations.

jcarrano · 2018-04-24T08:51:26Z

@jia200x The tlsf_g* functions are gone now.

jia200x · 2018-04-24T09:38:54Z

pkg/tlsf/contrib/include/tlsf-malloc.h

+ *
+ * If this module is used as the system memory allocator, then the global memory
+ * control block should be initialized as the first thing before the stdlib is
+ * used. Boards should use tlsf_add_global_pool() at startup to add all the memory


In fact, I think this could be in auto_init modules. For the CCN example, this avoids the need to call tlsf_add_global_pool. If so, this function could be renamed to tlsf_malloc_init. What do you think?

I'm not familiar with auto init. When initializing, tlsf_add_global_pool should be called with an address that depends on the memory map of the chip.

Is it OK to merge this without auto init support? Maybe it can be the subject of a future PR.

yes, sure. Could be done in further PRs

jia200x · 2018-05-17T17:51:48Z

I just tried it. Seems to work OK ;). And code-wise is fine.

cgundogan

ACK once my comment is addressed

jcarrano · 2018-05-18T13:24:42Z

Shall I squash?

cgundogan · 2018-05-18T13:33:12Z

Shall I squash?

yup, go ahead! 👍

- Cleanup package makefile. - Download directly from git. - Remove giant patch. - Implement malloc function as a contrib package. - Update ccn example. - Update ps command.

cgundogan · 2018-05-18T13:54:53Z

sys/ps/ps.c:84: error (cppcheckError): Analysis failed. If the code is valid then please report this failure.

What's wrong with cppcheck nowadays?

cgundogan · 2018-05-18T15:56:59Z

sys/ps/ps.c:84: error (cppcheckError): Analysis failed. If the code is valid then please report this failure.

Let's skip the broken cppcheck output for this PR. Just unrelated false positives. ACK => GO

miri64 reviewed Apr 23, 2018

View reviewed changes

cgundogan reviewed Apr 23, 2018

View reviewed changes

cgundogan requested review from Kijewski and jia200x April 23, 2018 15:10

cgundogan assigned jia200x Apr 23, 2018

cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 24, 2018

jia200x reviewed Apr 24, 2018

View reviewed changes

kaspar030 changed the title ~~Tlsf original api~~ pkg/tlsf: revert to original api Apr 24, 2018

cgundogan requested changes May 17, 2018

View reviewed changes

jcarrano force-pushed the tlsf-original-api branch from ad4977a to 1d63385 Compare May 18, 2018 13:23

pkg/tlsf: Refactor the TLSF package and bring back original TLSF api.

cf686bd

- Cleanup package makefile. - Download directly from git. - Remove giant patch. - Implement malloc function as a contrib package. - Update ccn example. - Update ps command.

jcarrano force-pushed the tlsf-original-api branch from 1d63385 to cf686bd Compare May 18, 2018 13:41

cgundogan approved these changes May 18, 2018

View reviewed changes

jcarrano mentioned this pull request May 18, 2018

pkg/lua: Better Lua-RIOT integration #9153

Merged

cgundogan merged commit f307314 into RIOT-OS:master May 18, 2018

jcarrano deleted the tlsf-original-api branch May 28, 2018 14:41

jcarrano mentioned this pull request May 30, 2018

sys/ps: Use correct define for TLSF information. #9247

Merged

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

jcarrano mentioned this pull request Jul 17, 2018

pkg/tlsf: add documentation. #8994

Closed

kYc0o mentioned this pull request Jul 17, 2018

pkg: TLSF does not build for 16 bit platforms #4612

Closed

jcarrano mentioned this pull request Aug 19, 2019

pkg/tlsf: Fix the way system functions are overriden. #12031

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/tlsf: revert to original api #9006

pkg/tlsf: revert to original api #9006

jcarrano commented Apr 23, 2018

miri64 Apr 23, 2018

jcarrano Apr 23, 2018

jcarrano Apr 23, 2018

jcarrano commented Apr 23, 2018

cgundogan Apr 23, 2018

jcarrano Apr 23, 2018

cgundogan May 17, 2018

jcarrano May 18, 2018

cgundogan May 18, 2018

cgundogan May 18, 2018 •

edited

cgundogan May 18, 2018 •

edited

jcarrano May 18, 2018

cgundogan May 18, 2018

jia200x commented Apr 23, 2018

jcarrano commented Apr 23, 2018

jia200x commented Apr 23, 2018

jia200x commented Apr 23, 2018

jcarrano commented Apr 23, 2018

jia200x commented Apr 23, 2018 •

edited

jcarrano commented Apr 24, 2018 •

edited

jia200x Apr 24, 2018

jcarrano Apr 24, 2018

jcarrano May 7, 2018

jia200x May 8, 2018

jia200x commented May 17, 2018

cgundogan left a comment

jcarrano commented May 18, 2018

cgundogan commented May 18, 2018

cgundogan commented May 18, 2018

cgundogan commented May 18, 2018

pkg/tlsf: revert to original api #9006

pkg/tlsf: revert to original api #9006

Conversation

jcarrano commented Apr 23, 2018

Refactor and cleanup of the TLSF package

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcarrano commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgundogan May 18, 2018 • edited

Choose a reason for hiding this comment

cgundogan May 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jia200x commented Apr 23, 2018

jcarrano commented Apr 23, 2018

jia200x commented Apr 23, 2018

jia200x commented Apr 23, 2018

jcarrano commented Apr 23, 2018

jia200x commented Apr 23, 2018 • edited

jcarrano commented Apr 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jia200x commented May 17, 2018

cgundogan left a comment

Choose a reason for hiding this comment

jcarrano commented May 18, 2018

cgundogan commented May 18, 2018

cgundogan commented May 18, 2018

cgundogan commented May 18, 2018

cgundogan May 18, 2018 •

edited

cgundogan May 18, 2018 •

edited

jia200x commented Apr 23, 2018 •

edited

jcarrano commented Apr 24, 2018 •

edited