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

net: sockets: socketpair: Allow statically allocated socketpairs #60914

Merged
merged 1 commit into from Jul 31, 2023

Conversation

SeppoTakalo
Copy link
Collaborator

When the target board does not have heap by default, allows statically reserving the space for required socketpairs.

Maximum number of statically allocated pairs is 16, so we can fit the usage flags into 32-bit atomic variable.

@SeppoTakalo
Copy link
Collaborator Author

If this is acceptable, I would use the socketpairs on LwM2M engine #60887

Not all boards define heap and I don't want to push that requirement to all LwM2M users, so I would propose this instead.

@@ -56,6 +56,11 @@ __net_socket struct spair {
uint8_t buf[CONFIG_NET_SOCKETPAIR_BUFFER_SIZE];
};

#ifdef CONFIG_NET_SOCKETPAIR_STATIC
static struct spair storage[CONFIG_NET_SOCKETPAIR_MAX * 2];
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to use k_mem_slab to hide the allocation details? It could simplify the allocation/deallocation of static objects a bit.

Copy link
Member

@cfriedt cfriedt Jul 28, 2023

Choose a reason for hiding this comment

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

sys_bitarray is also useful for this sort of thing and it works with an arbitrary number of entries

config NET_SOCKETPAIR_MAX
int "How many socketpairs to pre-allocate"
default 1
range 1 16
Copy link
Member

Choose a reason for hiding this comment

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

If we map this to 32bit var, then minimum is to check that if this value is changed, there will be a build error, so please add BUILD_ASSERT somewhere to enforce this.

Also we could allow arbitrary long bitmasks with ATOMIC_DEFINE and not rely on int size. Something like this could be implemented later.

Copy link
Member

@cfriedt cfriedt Jul 28, 2023

Choose a reason for hiding this comment

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

I agree that an int can be used without a range, and then just use a build assert

Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

int res;

#ifdef CONFIG_USERSPACE
#ifdef CONFIG_NET_SOCKETPAIR_STATIC
Copy link
Member

Choose a reason for hiding this comment

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

This search is O(n) but search with sys_bitarray is O(n/64).. which is also O(n)..

I've never done a performance comparison though 🤷🏼‍♂️

@cfriedt
Copy link
Member

cfriedt commented Jul 28, 2023

If this is acceptable, I would use the socketpairs on LwM2M engine #60887

It's close.

@SeppoTakalo
Copy link
Collaborator Author

Thank you for both of the suggestions.
I have now refactored the PR to use k_mem_slab as allocator, as suggested by Robert.
I think that between these two approaches, it is the cleanest implementation.

When the target board does not have heap by default, allows
statically reserving the space for required socketpairs.

Signed-off-by: Seppo Takalo <seppo.takalo@nordicsemi.no>
@carlescufi carlescufi merged commit c8ac307 into zephyrproject-rtos:main Jul 31, 2023
18 checks passed
@SeppoTakalo SeppoTakalo deleted the socketpair branch August 1, 2023 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants