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

Fix get_affinity for linux #527

Merged
merged 10 commits into from
Sep 15, 2014
Merged

Fix get_affinity for linux #527

merged 10 commits into from
Sep 15, 2014

Conversation

ddaeschler
Copy link
Contributor

This pull request fixes get_affinity under linux to properly deal with the scheduler CPU bitmask.

@giampaolo
Copy link
Owner

Why, what was the problem? Are you able to have a test case which fails before the patch and passes afterwards?

@giampaolo
Copy link
Owner

Note: the travis build appears to break after your patch so it seems something is wrong:
https://travis-ci.org/giampaolo/psutil/jobs/32151822

@giampaolo
Copy link
Owner

Ah sorry, I realized just now this is a PR for #522.

@ddaeschler
Copy link
Contributor Author

This patch is related to #522.

On Ubuntu 12.04.5 LTS (precise) sizeof(cpu_set_t) == 128 bytes. _psutil_linux.c was instead passing a long (8 bytes). In some configurations appears to cause EINVAL to be returned. I imagine this is the case with any system that reports more than 64 CPU cores.

I think I've discovered what was wrong with my initial patch that is causing the test failure.. I just need to figure out how to fix it and update this pull request.

…s return value to base the CPU counting loop on
@ddaeschler
Copy link
Contributor Author

Integration failed, but now it's stating "unknown failure on travis". Not sure what to make of that.

@ddaeschler
Copy link
Contributor Author

I caught a compilation error for one of the builds that was expecting C90 compliant code. I moved declarations around so that all the variables are declared at the top of the function before any other calls. Hoping this maybe fixes the CI issue.

@ddaeschler
Copy link
Contributor Author

All set.

@giampaolo
Copy link
Owner

Do you think this might help you?
http://hg.python.org/cpython/file/18a311479e8b/Modules/posixmodule.c#l7891

@ddaeschler
Copy link
Contributor Author

That source is using dynamic allocation so it would support more than 1024 CPU cores, whereas what I committed would support only 1024. I can do it that way if you'd prefer. This PR is working now though on a few setups I've tested.

@giampaolo
Copy link
Owner

I guess it would be better, so yes, if you still like it, please do it.

@ddaeschler
Copy link
Contributor Author

That code returns a set instead of a list. Would you like the psutil implementation to do the same?

@giampaolo
Copy link
Owner

I'd still like to see a list as I don't want to introduce backward
incompatibilities in a minor version.
We can change that to a set() in the next major version (actually there's a
ticket for this).

On Mon, Aug 11, 2014 at 2:40 PM, ddaeschler [email protected]
wrote:

That code returns a set instead of a list. Would you like the psutil
implementation to do the same?


Reply to this email directly or view it on GitHub
#527 (comment).

Giampaolo - http://grodola.blogspot.com

@ddaeschler
Copy link
Contributor Author

Ok. I'll implement this PR with a list. I can also change it to a set when the time comes

@giampaolo
Copy link
Owner

Thanks a lot for your effort.

On Mon, Aug 11, 2014 at 2:52 PM, ddaeschler [email protected]
wrote:

Ok. I'll implement this PR with a list. I can also change it to a set when
the time comes


Reply to this email directly or view it on GitHub
#527 (comment).

Giampaolo - http://grodola.blogspot.com

… based on the os_sched_getaffinity_impl in newer versions of python. However, this one returns a list of integers instead of a set of longs, and has an optimization in the CPU list building loop
@ddaeschler
Copy link
Contributor Author

This PR is ready with the new code as far as I can tell

giampaolo added a commit that referenced this pull request Sep 15, 2014
Fix get_affinity for linux
@giampaolo giampaolo merged commit 1e5dc37 into giampaolo:master Sep 15, 2014
@giampaolo
Copy link
Owner

Merged (finally). Sorry for waiting this long.

@mlowicki
Copy link

@giampaolo Great! When is next release planned?

@giampaolo giampaolo added the bug label Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants