Skip to content

Conversation

@TomzBench
Copy link
Contributor

This PR attempts to follow the original author of the eventfd
implementations' intuition, that the eventfd feature should be under
subsys/net.

This PR assumes the eventfd feature is more important for usage that is
specific to zephyr, than for usage that is intended for porting existing
posix applications.

Reviewing the original PR that implemented the eventfd feature, the code
reviewers seemed indifferent as to whether this feature was under
subsys/net or lib/posix, and it's reasons for landing in lib/posix
seemed arbitrary.

There was also difficulty coming up with a name that would not conflict
with native_posix host.

So to address the name conflict with posix architectures:

eventfd is rebranded under the zsock_ prefix, and a posix name is
available with CONFIG_NET_SOCKET_POSIX_NAMES, similar to other socket
features.

Samples and unit tests were migrated to network folders instead of posix
folders.

The "pure" posix demo was changed to be no longer be "pure" posix.
However, this sample should probably show how to use eventfd in the
context of a network socket anyway.

For background: #22863

Related issue: #47856

Signed-off-by: thomas chiantia [email protected]

This PR attempts to follow the original author of the eventfd
implementations' intuition, that the eventfd feature should be under
subsys/net.

This PR assumes the eventfd feature is more important for usage that is
specific to zephyr, than for usage that is intended for porting existing
posix applications.

Reviewing the original PR that implemented the eventfd feature, the code
reviewers seemed indifferent as to whether this feature was under
subsys/net or lib/posix, and it's reasons for landing in lib/posix
seemed arbitrary.

There was also difficulty coming up with a name that would not conflict
with native_posix host.

So to address the name conflict with posix architectures:

eventfd is rebranded under the `zsock_` prefix, and a posix name is
available with CONFIG_NET_SOCKET_POSIX_NAMES, similar to other socket
features.

Samples and unit tests were migrated to network folders instead of posix
folders.

The "pure" posix demo was changed to be no longer be "pure" posix.
However, this sample should probably show how to use eventfd in the
context of a network socket anyway.

Signed-off-by: thomas chiantia <[email protected]>
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.

Eventfd is not really part of the network subsystem - on Zephyr or really any OS. While it isn't covered under the POSIX spec per se, I feel it belongs more to lib/posix. There are obvious uses for eventfd when dealing with a collection of file descriptors / sockets, of course.

I do see your concerns with being able to use a number of useful features under ARCH_POSIX, and I have a general issue to resolve that at #45100. There is a fairly simple way to do that in a standards-compliant / portable way, and the obstacles to get there are probably more philosophical / political than technical.

I'm going to NAK for now, but I hope I can address your concerns with some upcoming changes planned.

@TomzBench
Copy link
Contributor Author

@cfriedt

In Zephyr, eventfd is the only (efficient) way to signal to zsock_poll that something happened and you need to update the file descriptor array. Perhaps a zsock_eventfd can be supported until ARCH_POSIX issue is implemented?

There are obvious uses for eventfd when dealing with a collection of file descriptors / sockets, of course.

It is my understanding that k_poll doesn't support file descriptors, and zsock_poll supports sockets. If zsock_poll also supports files opened with fopen ...then doesn't it also make sense to migrate eventfd under the zsock namespace, using the exisiting underlying posix like infrastructure as subsys/net?

I think a long term approach like the one you propose will better address this issue - but this PR I think is useful as only a temporary measure as it serves a common need for zephyr applications in the interim.

@cfriedt
Copy link
Member

cfriedt commented Jul 22, 2022

In Zephyr, eventfd is the only (efficient) way to signal to zsock_poll that something happened and you need to update the file descriptor array. Perhaps a zsock_eventfd can be supported until ARCH_POSIX issue is implemented?

zsock_socketpair() can serve exactly the same purpose in the interim and that is a bit more within the scope of subsys/net - no upstream changes required.

It is my understanding that k_poll doesn't support file descriptors, and zsock_poll supports sockets.

Correct, k_poll() is for signalling on kernel objects.
https://docs.zephyrproject.org/latest/kernel/services/polling.html#concepts

If zsock_poll also supports files opened with fopen ...then doesn't it also make sense to migrate eventfd under the zsock namespace, using the exisiting underlying posix like infrastructure as subsys/net?

Temporarily adjusting a piece of code from one subsystem to another, and then moving it back to the original subsystem does no-one a service. It introduces unnecessary code-churn that has fairly far-reaching and unwanted side-effects not only within Zephyr's project but also with 3rd-party modules. I.e. it would break configurations, external modules, etc. It is not reasonable to expect the rest of the community to repair damages twice so that a minority of the community can have a temporary convenience measure put in place. The alternative requires some patience but is backward compatible with existing configurations and forward compatible with planned changes.

I think a long term approach like the one you propose will better address this issue - but this PR I think is useful as only a temporary measure as it serves a common need for zephyr applications in the interim.

I would suggest using zsock_socketpair() in the mean time. Sorry for any inconvenience.

@TomzBench
Copy link
Contributor Author

Thanks for detailed responses @cfriedt

I would suggest using zsock_socketpair() in the mean time. Sorry for any inconvenience.

For reasons detailed here #47856: I think I'll use this fork for now. but hoping for the solution you propose soon.

Thanks

@TomzBench TomzBench closed this Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Networking area: POSIX POSIX API Library area: Sockets Networking sockets

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants