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

lib/posix-poll: Fix epoll missing events #1297

Merged

Conversation

andreittr
Copy link
Contributor

@andreittr andreittr commented Jan 31, 2024

Description of changes

Previously epoll would miss events when configured for edge-polling and unable to return all captured events in a single call to epoll_wait, by erroneously marking the epoll fd as not ready, even though it may still have pending events.
This change fixes this oversight, restoring expected behavior to epoll_wait.

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): N/A
  • Platform(s): N/A
  • Application(s): N/A

Additional configuration

Trigger/test snippet:

int main(void)
{
	struct epoll_event ev;
	int p[2], q[2];
	int r;

	r = pipe(p);
	assert(!r);
	r = pipe(q);
	assert(!r);

	int e = epoll_create(2);
	assert(e >= 0);

	ev.events = EPOLLIN|EPOLLET;
	ev.data.fd = p[0];
	r = epoll_ctl(e, EPOLL_CTL_ADD, p[0], &ev);
	assert(!r);

	ev.events = EPOLLIN|EPOLLET;
	ev.data.fd = q[0];
	r = epoll_ctl(e, EPOLL_CTL_ADD, q[0], &ev);
	assert(!r);

	write(p[1], "a", 1);
	write(q[1], "b", 1);

	do {
		char buf[2];
		r = epoll_wait(e, &ev, 1, 0);
		printf("%d\n", r);
		if (r) {
			read(ev.data.fd, buf, 1);
			buf[1] = 0;
			puts(buf);
		}
	} while (r);
	return 0;
}

Output should be

1
a
1
b
0

On both Linux and Unikraft; previously unikraft would miss one of the messages (most likely b).

Previously epoll would miss events when configured for edge-polling and
unable to return all captured events in a single call to epoll_wait, by
erroneously marking the epoll fd as not ready, even though it may still
have pending events.
This change fixes this oversight, restoring expected behavior to
epoll_wait.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@andreittr andreittr requested a review from a team as a code owner January 31, 2024 08:56
@github-actions github-actions bot added area/lib Internal Unikraft Microlibrary lang/c Issues or PRs to do with C/C++ labels Jan 31, 2024
@razvand razvand requested review from mogasergiu and StefanJum and removed request for a team February 1, 2024 05:20
@razvand razvand added this to the v0.16.2 milestone Feb 1, 2024
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

All good, thanks.
Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

/* If lvlev, update pollin back in */
if (lvlev)
/* If lvlev or limited by maxevents, update pollin back in */
if (lvlev || nout == maxevents)
uk_file_event_set(epf, UKFD_POLLIN);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this function return something? Is there a reasion we are not checking for return values?

Copy link
Member

Choose a reason for hiding this comment

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

In fact I don't see its return values being checked at any of its callsite 🤔

Copy link
Contributor Author

@andreittr andreittr Feb 1, 2024

Choose a reason for hiding this comment

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

uk_file_event_* functions return the previous events, as a convenience due to the fact that event setting is done atomically so there's no other way to get that previous value. This is not needed often, but is needed sometimes, thus the need to return it.

In this case we don't care what the previous events were. I used to cast such invocations to void, but this turned out to be everywhere, visually polluting the code, not to mention increasing the pressure on keeping everything within 80 cols with size 8 tabs 😛 ; so I stopped that.

Copy link
Member

Choose a reason for hiding this comment

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

Oh well I guess we also call printf while ignoring return value...

Copy link
Member

@mogasergiu mogasergiu left a comment

Choose a reason for hiding this comment

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

Reviewed-by: Sergiu Moga sergiu@unikraft.io

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

@razvand razvand added the merge Label to trigger merge action label Feb 6, 2024
@unikraft-bot unikraft-bot changed the base branch from staging to staging-1297 February 6, 2024 17:11
@unikraft-bot unikraft-bot merged commit 33cde0d into unikraft:staging-1297 Feb 6, 2024
13 checks passed
unikraft-bot pushed a commit that referenced this pull request Feb 6, 2024
Previously epoll would miss events when configured for edge-polling and
unable to return all captured events in a single call to epoll_wait, by
erroneously marking the epoll fd as not ready, even though it may still
have pending events.
This change fixes this oversight, restoring expected behavior to
epoll_wait.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Reviewed-by: Sergiu Moga <sergiu@unikraft.io>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
GitHub-Closes: #1297
@unikraft-bot unikraft-bot added ci/merged Merged by CI and removed merge Label to trigger merge action labels Feb 6, 2024
@andreittr andreittr deleted the ttr/fix-epoll-missing branch February 7, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary ci/merged Merged by CI lang/c Issues or PRs to do with C/C++
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants