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/tlsf: add documentation. #8994

Closed
wants to merge 1 commit into from
Closed

Conversation

jcarrano
Copy link
Contributor

The doc.txt for TLSF was empty, so it wouldn't show up in Doxygen. I wrote a brief description and also cross linked it from the "Oneway Malloc" docs, so that people looking for memory allocation can find it.

@jcarrano
Copy link
Contributor Author

jcarrano commented Apr 20, 2018

Also, there's a big patch being applied, and it's not very clear what it does. It changes the TLSF api, which is a problem since now there is no way for the user to find the correct docs for the new api.

Also, the patch removes one of the features of TLSF, that is the possibility of not having any global state, that is, functions of the form
void* tlsf_malloc(tlsf_t tlsf, size_t bytes);
now become
void* tlsf_malloc(size_t bytes);
To be able to do this patch also adds a global variable to tlsf.c
@@ -165,0 +153 @@ typedef struct control_t
+static control_t *control;

As far a I can tell, no one is using TLSF directly, other code in RIOT uses vanilla malloc/free. This means that malloc/free could be implemented in terms of the unmodified API, and the global heap could be defined in the unit that defines malloc/free/etc.

I want to be able to use the original TLSF functions to be able to isolate the memory for the Lua interpreter.

@waehlisch waehlisch requested review from jia200x and miri64 April 20, 2018 11:17
@jia200x
Copy link
Member

jia200x commented Apr 20, 2018

hi @jcarrano

The patch allows the usage of TLSF as the global malloc/free implementation. Concerning the isolated TLSF environments, I'm pretty sure @OlegHahm or @Kijewski has more information about why it was implemented this way.

and also cross linked it from the "Oneway Malloc" docs

Not sure if it's the best to put the tlsf link there, since Oneway Malloc is a specific implementation for platforms that don't implement malloc and free in the toolchain. Maybe a Memory Allocator entry which points to Oneway Malloc and TLSF? Opinions?

* @defgroup pkg_tlsf Two-Level Segregated Fit memory allocator
* @ingroup pkg
* @brief TLSF is a general purpose dynamic memory allocator specifically
* designed to meet real-time requirements:
Copy link
Member

Choose a reason for hiding this comment

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

Colon in the end. Should be dot.

* - Works on a user supplied block of memory instead of a global heap.
* - Efficient both in terms of memory overhead and processor time.
* - Low fragmentation.
*
Copy link
Member

@jia200x jia200x Apr 20, 2018

Choose a reason for hiding this comment

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

As far as I know, this implementation/adoption doesn't support 16-bit platforms and maybe should be pointed out here. @OlegHahm can you confirm this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not building for 16 bit, see #4612

@@ -20,6 +20,8 @@
*
* @note You should prefer statically allocated memory whenever possible.
*
* @see pkg_tlsf
Copy link
Member

Choose a reason for hiding this comment

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

@jcarrano
Copy link
Contributor Author

jcarrano commented Apr 23, 2018

@jia200x

Oneway Malloc is a specific implementation for platforms that don't implement malloc and free in the toolchain

If that's the case, then it's a good reason for linking the docs. If the platform does not implement malloc/free then the user will try to look at an external implementation.

About the patch, that's another issue, I asked about it in the devel mailing list. For this PR I only care about documentation.

@jcarrano
Copy link
Contributor Author

jcarrano commented May 18, 2018

I think we can drop this one now the the real tlsf patch (#9006 ) is being merged.

@jcarrano jcarrano closed this May 18, 2018
@kYc0o
Copy link
Contributor

kYc0o commented Jul 17, 2018

@jcarrano Can you reference your real patch? and if it's already merged?

@jcarrano
Copy link
Contributor Author

@kYc0o #9006 Included a doc.txt as part of the changes.

@kYc0o
Copy link
Contributor

kYc0o commented Jul 17, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants