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

Small notifier fix for pullswitch #41

Closed
wants to merge 1 commit into from
Closed

Conversation

jdizzle
Copy link

@jdizzle jdizzle commented Feb 2, 2012

If the switch's input is set to -1, it is disabled and should notify
registered signal handlers that they should sleep.

If the switch's input is set to -1, it is disabled and should notify
registered signal handlers that they should sleep.
@clifffrey
Copy link
Collaborator

I worked with Justin on this fix, I thought that the following case would demonstrate the problem, but it doesn't for me. Specifically, I believed that the issue was that PullSwitch's notifier could be enabled if any upstream notifiers became active (because of SEARCH_CONTINUE_WAKE), and then nothing would ever shut it off.... so maybe I'm confused as to the actual problem. However, we were seeing 100% cpu utilization without this change, and normal cpu utilization after this change... so it does something.

click --simtime <<EOF
is::InfiniteSource(LIMIT 1, ACTIVE false) -> Queue -> PullSwitch(-1) -> Discard;
Script(wait 1, write is.active true, wait 1, stop);
EOF

@kohler
Copy link
Owner

kohler commented Feb 16, 2012

An interesting problem! I can tell you what happened, I think. There was already code to set the notifier to false when input was -1. But, there is also code that attaches the notifier to the upstream signals. So if an upstream source TRANSITIONS from inactive to active, that will turn on the notifier, even if input < 0. Your infinitesource is always active, so it never transitions.

This is a bit of an odd problem. I am tempted to think about how to fix it in a different way. In particular, it would be nice for the "upstream_empty_signal" notification support to handle this "correctly.",........

@kohler
Copy link
Owner

kohler commented Mar 29, 2012

Dudes, have you had a chance to look at the different way I sent mail about a bit ago?

@clifffrey
Copy link
Collaborator

I assume you mean the solution in d72071e and 417215b ? I think that the solution in those commits is good. It has been merged into our branch here already. I guess I don't feel strongly either way as to whether this patch should be merged now or not.

@kohler
Copy link
Owner

kohler commented Mar 29, 2012

OK, cool. I think that solution dominates this patch. I did just check in b445a79 because in some implausible contexts a notifier that got turned on at the wrong time would stay on; now it won't.

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

Successfully merging this pull request may close these issues.

None yet

3 participants