Skip to content

Conversation

@jlaitine
Copy link
Contributor

@jlaitine jlaitine commented Jan 3, 2023

Summary

This PR adds a simple linked list to store mmap mappings, to support unmap.

The drivers which want to store mappings can use mm_map_add and mm_map_rm to register/unregister mappings to the task_group.

This PR also takes the mm_map into use in mm/shm driver and anonymous mappings. The sys-v shm interface still maintains it's own granule allocator in task_group

This PR also takes the mm_map into use in "rammap"

Impact

Adds interface to make it possible to do munmap
Adds munmap for anonymous mappings
Fixes mm/shm for process exit; previously it doesn't free the reserved physical pages if they are mapped at process exit

Testing

Tested in stm32 and mpfs riscv builds, together with inhouse posix shmfs driver

@jlaitine
Copy link
Contributor Author

jlaitine commented Jan 3, 2023

There were some issues left after re-base. I commented them, and will fix those tomorrow.

@xiaoxiang781216
Copy link
Contributor

BTW, it's better to migrate the ram map and anon map to the new interface, which is clearer and also demo that new mmap interface is flexible enough to support the complex case.

@jlaitine
Copy link
Contributor Author

jlaitine commented Jan 4, 2023

BTW, it's better to migrate the ram map and anon map to the new interface, which is clearer and also demo that new mmap interface is flexible enough to support the complex case.

Agree. I can move ram mapping (MAP_PRIVATE, MAP_ANONYMOUS) to an own file at least. It should also have an own CONFIG flag (can make it default to y to keep current functionality). I find this interface quite useless as it is now, it doesn't even support mapping from page pool.

rammaps deserve an own PR, but I'll see if I can make use of the mm_map in it already as you suggested above. That sould be extended to also support MAP_SHARED. I find this interface quite useless as it is now, and this is why I don't like touching it too much in this PR.

I think that after we agree on the interface for storing the mappings, I will present the posix SHMFS; this is an interface that I know is interesting to many. For example we have implemented an own version of PX4 uORB publish subscribe interface with this, and it is really nice, since it works inter process both in linux and nuttx (in all flat, protected and kernel). There are also other pubsub interfaces which could utilize this.

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Jan 4, 2023

BTW, it's better to migrate the ram map and anon map to the new interface, which is clearer and also demo that new mmap interface is flexible enough to support the complex case.

Agree. I can move ram mapping (MAP_PRIVATE, MAP_ANONYMOUS) to an own file at least. It should also have an own CONFIG flag (can make it default to y to keep current functionality). I find this interface quite useless as it is now, it doesn't even support mapping from page pool.

Yes, but it's useful for no MMU case. it's a first step to incorporate rammap into the new mmap interface. Once it's done, we can support page pool for MMU.

rammaps deserve an own PR, but I'll see if I can make use of the mm_map in it already as you suggested above. That sould be extended to also support MAP_SHARED. I find this interface quite useless as it is now, and this is why I don't like touching it too much in this PR.

supporting MAP_SHARED require we save some mapping info to inode, but I think it's very important feature to make rammap usefull.

I think that after we agree on the interface for storing the mappings,

Yes, I think so.

I will present the posix SHMFS; this is an interface that I know is interesting to many. For example we have implemented an own version of PX4 uORB publish subscribe interface with this, and it is really nice, since it works inter process both in linux and nuttx (in all flat, protected and kernel). There are also other pubsub interfaces which could utilize this.

Nice feature. Where can I find the document or source code? is it bas on PX4 uORB or this new one: #6653

@jlaitine
Copy link
Contributor Author

jlaitine commented Jan 4, 2023

I will present the posix SHMFS; this is an interface that I know is interesting to many. For example we have implemented an own version of PX4 uORB publish subscribe interface with this, and it is really nice, since it works inter process both in linux and nuttx (in all flat, protected and kernel). There are also other pubsub interfaces which could utilize this.

Nice feature. Where can I find the document or source code? is it bas on PX4 uORB or this new one: #6653

It is based on PX4 uORB; the project that I am working with atm. is drone project and the uORB needs to be a drop-in replacement for PX4

@jlaitine jlaitine force-pushed the add_mm_map branch 2 times, most recently from 3dd971b to 79586c7 Compare January 4, 2023 11:27
@jlaitine jlaitine force-pushed the add_mm_map branch 4 times, most recently from 4684489 to df4cd3f Compare January 4, 2023 13:54
@jlaitine
Copy link
Contributor Author

jlaitine commented Jan 4, 2023

BTW, it's better to migrate the ram map and anon map to the new interface, which is clearer and also demo that new mmap interface is flexible enough to support the complex case.

Added patches for this

@jlaitine
Copy link
Contributor Author

jlaitine commented Jan 5, 2023

The rammaps went broken, I will need to fix it still. It was a bit too quick...

@jlaitine
Copy link
Contributor Author

jlaitine commented Jan 5, 2023

now done. Rammaps doesn't need the private mapping structure for anything, all info fits in map_entry_s. So removed that too.

@jlaitine
Copy link
Contributor Author

jlaitine commented Jan 5, 2023

re-based to latest master

@jlaitine
Copy link
Contributor Author

jlaitine commented Jan 6, 2023

One way to remove group argument from mm_map_remove is just to put a restriction that "driver mustn't call mm_map_remove in the munmap function if the group is unknown (NULL)". Then you would get rid of the iffy goto in mm_map_remove function as well. All entries can be deleted directly in mm_map_destroy() regardless of the current context.

@jlaitine
Copy link
Contributor Author

jlaitine commented Jan 7, 2023

@xiaoxiang781216 : Here is a commit which shows how the IF would look like if you added the mm_map_s as an argument to every function (the option 1. you mentioned avove):

tiiuae@1385bca

In my opinion it just complicates things for every driver using the interface, since they need to fetch & dig into the task_group structure, even if they don't need it for anything else.

But if you think that this would look better, I am fine with it.

Please let me know which way you insist this to be, I will then clean up the git history accordingly.

@xiaoxiang781216
Copy link
Contributor

One way to remove group argument from mm_map_remove is just to put a restriction that "driver mustn't call mm_map_remove in the munmap function if the group is unknown (NULL)". Then you would get rid of the iffy goto in mm_map_remove function as well. All entries can be deleted directly in mm_map_destroy() regardless of the current context.

My comment isn't to remove group from mm_map_remove, but make the usage consistent.

@xiaoxiang781216 : Here is a commit which shows how the IF would look like if you added the mm_map_s as an argument to every function (the option 1. you mentioned avove):

tiiuae@1385bca

In my opinion it just complicates things for every driver using the interface, since they need to fetch & dig into the task_group structure, even if they don't need it for anything else.

But if you think that this would look better, I am fine with it.

Please let me know which way you insist this to be, I will then clean up the git history accordingly.

Yes, I think it isn't good to let's driver dig into task_group struct. Let's come back to my first suggestion but reject by you:
All mm_map_s related function use tasK_group_s as the first argument.

@jlaitine jlaitine force-pushed the add_mm_map branch 2 times, most recently from 04fc90d to 4ed2485 Compare January 9, 2023 10:02
@jlaitine
Copy link
Contributor Author

jlaitine commented Jan 9, 2023

Yes, I think it isn't good to let's driver dig into task_group struct. Let's come back to my first suggestion but reject by you: All mm_map_s related function use tasK_group_s as the first argument.

Any other part of the task_group_s doesn't belong to mm_map code, so it is really better to just pass mm_map_s where it is needed. I decoupled this from task_struct internals by adding helper macros into sched.h

@jlaitine jlaitine force-pushed the add_mm_map branch 2 times, most recently from f9c2645 to a6dc0cb Compare January 10, 2023 07:13
To be able to directly store also other than pointer types

Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
@jlaitine
Copy link
Contributor Author

re-based to current master

Jukka Laitinen added 5 commits January 10, 2023 12:22
The task_group specific list can be used to store information about
mmappings.

For a driver or filesystem performing mmap can also enable munmap by
adding an item to this list using mm_map_add(). The item is then
returned in the corresponding munmap call.

Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
Replace static gs_vaddr with a new dynamic mapping list. Collecting all
this kind of virtual memory mappings into a single structure makes
things more consistent.

This still leaves the task group specific granule alloocator, gs_handle,
in the task group

Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 left a comment

Choose a reason for hiding this comment

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

LGTM.

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.

4 participants