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

core: fix priority ordering in notify-handling #1707

Merged
merged 1 commit into from Oct 28, 2015

Conversation

dvdhrm
Copy link
Contributor

@dvdhrm dvdhrm commented Oct 28, 2015

Currently, we dispatch NOTIFY messages in a tight loop. Regardless how
much data is incoming, we always dispatch everything that is queued.
This, however, completely breaks priority event-handling of sd-event.
When dispatching one NOTIFY event, another completely different event
might fire, or might be queued by the NOTIFY handling. However, this
event will not get dispatched until all other further NOTIFY messages are
handled. Those might even arrive after the other event fired, and as
such completely break priority ordering of sd-event (which several code
paths rely on).

Break this by never dispatching multiple messages. Just return after each
message that was read and let sd-event handle everything else.

(The patch looks scarier that it is. It basically just drops the for(;;)
loop and re-indents the loop-content.)

Maybe related to #1505. Please test! Regardless whether it fixes #1505, this looks like something we should merge.

Currently, we dispatch NOTIFY messages in a tight loop. Regardless how
much data is incoming, we always dispatch everything that is queued.
This, however, completely breaks priority event-handling of sd-event.
When dispatching one NOTIFY event, another completely different event
might fire, or might be queued by the NOTIFY handling. However, this
event will not get dispatched until all other further NOTIFY messages are
handled. Those might even arrive _after_ the other event fired, and as
such completely break priority ordering of sd-event (which several code
paths rely on).

Break this by never dispatching multiple messages. Just return after each
message that was read and let sd-event handle everything else.

(The patch looks scarier that it is. It basically just drops the for(;;)
 loop and re-indents the loop-content.)
@poettering
Copy link
Member

patch lgtm. ready to merge if semaphore agrees

@poettering poettering closed this Oct 28, 2015
@poettering poettering reopened this Oct 28, 2015
@poettering
Copy link
Member

fuck, pressed the wrong button. sorry

poettering added a commit that referenced this pull request Oct 28, 2015
core: fix priority ordering in notify-handling
@poettering poettering merged commit 7f610de into systemd:master Oct 28, 2015
@zonque
Copy link
Member

zonque commented Oct 28, 2015

FTR: it does not fix #1505

@mbiebl
Copy link
Contributor

mbiebl commented Oct 28, 2015

Well, I guess github was just too clever here and interpreted "whether it fixes ..." in the PR incorrectly.

@Shuangistan
Copy link
Contributor

If a similar logic is also be true for manager_dispatch_sigchld. Seems here also be too much. It takes 3.2s and which might be critical for hardware watchdog?

[ 42.564130] systemd[1]: 'manager-signal' here
[ 42.607821] systemd[1]: Received SIGCHLD from PID 1549 (mkdir).
[ 42.625604] systemd[1]: Child 1549 (mkdir) died (code=exited, status=0/SUCCESS)
...
[ 42.903391] systemd[1]: Child 1556 (d_xxx_dump_o) died (code=exited, status=0/SUCCESS)
...
[ 43.265386] systemd[1]: Child 1559 (mkdir) died (code=exited, status=0/SUCCESS)
...
[ 43.727794] systemd[1]: Child 1573 (chown) died (code=exited, status=0/SUCCESS)
...
[ 44.337163] systemd[1]: Child 1597 (xxx_once) died (code=exited, status=0/SUCCESS)
...
[ 45.014195] systemd[1]: Child 1624 (chmod) died (code=exited, status=0/SUCCESS)
...
[ 45.757349] systemd[1]: delta_t=3260.015000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

[227 regression] Boot occasionally fails with "Connection timed out"
5 participants