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 memory overflow when allocating CPUs when numa is enabled #2745

Merged
merged 1 commit into from
Jun 5, 2018

Conversation

SeanTAllen
Copy link
Member

Specifically when numa is enabled and the count of available cpus isn't
greater than the number of numa cores.

Previously, when

    if(avail_cpu_count > numa_count)
          avail_cpu_size = numa_count;

was executed, then at that point avail_cpu_size was 0. this means if
avail_cpu_count is not greater than numa_count then avail_cpu_size
will remain 0 and:

ponyint_pool_alloc_size(avail_cpu_size * sizeof(uint32_t));

will allocate no memory and bad segfaulting things will happen.

This commit patches the bug to set avail_cpu_size to the CPU_COUNT
when we enter ponyint_cpu_init.

avail_cpu_size = avail_cpu_count = CPU_COUNT(&all_cpus);

Fixes #2559
Fixes #2689

Specifically when numa is enabled and the count of available cpus isn't
greater than the number of numa cores.

Previously, when

```
    if(avail_cpu_count > numa_count)
          avail_cpu_size = numa_count;
```

was executed, then at that point avail_cpu_size was 0. this means if
`avail_cpu_count` is not greater than `numa_count` then `avail_cpu_size`
will remain 0 and:

```
ponyint_pool_alloc_size(avail_cpu_size * sizeof(uint32_t));
```

will allocate no memory and bad segfaulting things will happen.

This commit patches the bug to set `avail_cpu_size` to the CPU_COUNT
when we enter `ponyint_cpu_init`.

`  avail_cpu_size = avail_cpu_count = CPU_COUNT(&all_cpus);`

Fixes #2559
Fixes #2689
@SeanTAllen SeanTAllen added triggers release Major issue that when fixed, results in an "emergency" release changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge labels Jun 5, 2018
Copy link
Contributor

@slfritchie slfritchie left a comment

Choose a reason for hiding this comment

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

+1, this is at least 106% less terrible

@slfritchie
Copy link
Contributor

slfritchie commented Jun 5, 2018

And also this patch doesn't crash when using the reproducer recipe in #2559 on an AWS c4.8xlarge VM.

@SeanTAllen SeanTAllen merged commit 752deb7 into master Jun 5, 2018
@SeanTAllen SeanTAllen deleted the e837d09 branch June 5, 2018 03:35
ponylang-main added a commit that referenced this pull request Jun 5, 2018
@SeanTAllen SeanTAllen mentioned this pull request Jun 5, 2018
dipinhora pushed a commit to dipinhora/ponyc that referenced this pull request Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge triggers release Major issue that when fixed, results in an "emergency" release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants