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.

Sign up for GitHub

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

Jump to bottom

[PATCH v4] linux-gen: shm: make internal ishm block names unique #2174

Merged
merged 3 commits into from
Jan 28, 2025
Merged

[PATCH v4] linux-gen: shm: make internal ishm block names unique #2174

merged 3 commits into from
Jan 28, 2025

Conversation

MatiasElo
Copy link
Collaborator

No description provided.

@odpbuild odpbuild changed the title linux-gen: shm: make internal ishm block names unique [PATCH v1] linux-gen: shm: make internal ishm block names unique Jan 17, 2025
JannePeltonen
JannePeltonen reviewed Jan 20, 2025
View reviewed changes
@odpbuild odpbuild changed the title [PATCH v1] linux-gen: shm: make internal ishm block names unique [PATCH v2] linux-gen: shm: make internal ishm block names unique Jan 22, 2025
JannePeltonen
JannePeltonen reviewed Jan 22, 2025
View reviewed changes
platform/linux-generic/odp_ishm.c
@@ -2045,7 +2065,7 @@ int _odp_ishm_status(const char *title)
_ODP_PRINT("%2i %-*s %s%c %p-%p %-8" PRIu64 " "
"%-8" PRIu64 " %-3" PRIu64 " %-3" PRIu64 " "
"%-3d %s\n",
i, max_name_len, ishm_tbl->block[i].name,
i, max_name_len, ishm_tbl->block[i].param_name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since .name was changed to .param_name. the corresponding field width parameter max_name_len should also be calculated from param_name instead of name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in V3.

platform/linux-generic/odp_ishm.c
@@ -2142,7 +2162,7 @@ void _odp_ishm_print(int block_index)
block = &ishm_tbl->block[block_index];

_ODP_PRINT("\nSHM block info\n--------------\n");
_ODP_PRINT(" name: %s\n", block->name);
_ODP_PRINT(" name: %s\n", block->param_name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the internal unique name could still be printed too since this function appears to be meant for debugging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Print added in V3.

platform/linux-generic/odp_ishm.c
_odp_strcpy(new_block->name, name, ISHM_NAME_MAXLEN);
else
/* Save application given block name as-is and make internal block name used for mmap
unique */
Copy link
Collaborator

Choose a reason for hiding this comment

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

The call to alloc_single_va() later on in this function still uses the application given non-unique name. I did not read carefully but it looks like it would be a problem for exported memory blocks as it would cause overlapping export file names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was not a problem but logic changed in V3.

platform/linux-generic/odp_ishm.c Outdated
else
/* Save application given block name as-is and make internal block name used for mmap
unique */
if (name) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it would be simpler to always make the internal name unique, even if no name was given by the application? Now the situation is a bit weird that the internal name is unique except when it is the empty string in which case it is not unique and then the lower level code has to special case that and create a unique string using create_seq_string(). Making the name always unique would make the code more clear, I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Logic changed in V3 to always use unique internal names.

platform/linux-generic/odp_ishm.c Outdated
_odp_strcpy(block_name, name, sz);

while (find_block_by_name(block_name, false) >= 0)
_odp_snprint(block_name, sz, "%d-%s", i++, name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that I noticed that the create_seq_string() and ishm_tbl->dev_seq are already there, maybe (?) this could just generate the unique name using dev_seq or similar and avoid the search?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed in V3.

@odpbuild odpbuild changed the title [PATCH v2] linux-gen: shm: make internal ishm block names unique [PATCH v3] linux-gen: shm: make internal ishm block names unique Jan 24, 2025
JannePeltonen
JannePeltonen approved these changes Jan 24, 2025
View reviewed changes
Copy link
Collaborator

@JannePeltonen JannePeltonen left a comment

Choose a reason for hiding this comment

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

Export behaves a bit weirdly if the application given SHM name is empty or non-unique, but that seem to have been there already before this commit and I am not sure how export with non-unique name could even be used.

@MatiasElo
abi: shm: increase ODP_SHM_NAME_LEN value
a88184a 
Increase ODP_SHM_NAME_LEN default value to 64. It can be useful that SHM
block names are longer compared to name lengths in other API modules. For
example this enables easier use of SHM blocks to implement other API
modules.

Signed-off-by: Matias Elo <[email protected]>
Reviewed-by: Janne Peltonen <[email protected]>
@MatiasElo
linux-gen: shm: check shm block name length
4577650 
Check that SHM block names given as odp_shm_reserve() or odp_shm_import()
arguments fit into ODP_SHM_NAME_LEN characters. Also, verify that internal
ISHM_NAME_MAXLEN define is large enough to store block names from
application.

Signed-off-by: Matias Elo <[email protected]>
Reviewed-by: Janne Peltonen <[email protected]>
@MatiasElo
linux-gen: shm: make internal ishm block names unique
d8a2f1a 
Previously, if application used same name and ODP_SHM_PROC/ODP_SHM_EXPORT
flag for multiple SHM reservations the memory blocks were mapped to the
same file. Fix this by always using unique implementation internal names
for memory block reservations.

Signed-off-by: Matias Elo <[email protected]>
Reviewed-by: Janne Peltonen <[email protected]>
@odpbuild odpbuild changed the title [PATCH v3] linux-gen: shm: make internal ishm block names unique [PATCH v4] linux-gen: shm: make internal ishm block names unique Jan 24, 2025
@TuomasTaipale TuomasTaipale merged commit 82580dd into OpenDataPlane:master Jan 28, 2025
160 checks passed
@MatiasElo MatiasElo deleted the fix/shm-same-name branch February 3, 2025 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JannePeltonen JannePeltonen JannePeltonen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MatiasElo @JannePeltonen @TuomasTaipale