Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Enable overriding default stack size on Unix #13517

Merged
merged 1 commit into from
Aug 23, 2017

Conversation

janvorli
Copy link
Member

Alpine Linux has a very small default stack size limit, about 80kB.
This is not enough for running coreclr apps.

This change enables overriding the default stack size using the
COMPlus_DefaultStackSize env variable. For Alpine, it also sets
the default stack size to the same value we use for Windows, which
is 1.5MB.

I have also added documentation for this env variable and also the other pre-existing env variables used by the crashdump to the Documentation/project-docs/clr-configuration-knobs.md. Since that file is generated by the Documentation/project-docs/clr-complus-conf-docgen.sh, I have modified that script and added the new resulting clr-configuration-knobs.md. The script was ran more than a year ago and so there were some stale and some new configuration knobs.

@janvorli janvorli requested a review from jkotas August 22, 2017 12:54
@janvorli janvorli self-assigned this Aug 22, 2017
@janvorli
Copy link
Member Author

@swgillespie I have noticed that after I've updated the config knobs doc using the script, some of the GC related knobs were removed. Namely the UNSUPPORTED_GCtraceStart, UNSUPPORTED_GCtraceEnd, UNSUPPORTED_GCprnLvl and INTERNAL_GCtraceFacility knobs that are referred to in the eeconfig.cpp under the TRACE_GC ifdef, but they are not defined in the src/inc/clrconfigvalues.h. Are these really dead?

}

uint8_t *s = (uint8_t *)_alloca(g_defaultStackSize);
s[0] = 0;
Copy link
Member

@jkotas jkotas Aug 22, 2017

Choose a reason for hiding this comment

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

Decent compiler will remove this with optimizations on. You may want to do this with volatile.

Copy link
Member Author

Choose a reason for hiding this comment

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

This function is marked with attribute optnone, so no optimizations are allowed.

Copy link
Member

Choose a reason for hiding this comment

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

It maybe nice to move the small part that does the alloca moved into separate method and mark just that - more readable, and probably smaller too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I actually had the function like that until I've added the ability to set the size by the env variable.

g_defaultStackSize = 1536 * 1024;
}

uint8_t *s = (uint8_t *)_alloca(g_defaultStackSize);
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to commit the memory (touch every page out of the 1.5MB) or is this going to touch the bottom one only? It is pretty unfortunate if this is committing 1.5MB - most of it won't be used by the process.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't commit the whole range. Or at least doesn't grow the resident set for the whole range. I've added dumping the "VmRSS" field from the /proc/self/status and the value doesn't change. I've also tried to touch the whole range page by page and the resident set was growing in steps of 264kB every 66 pages I've touched.
Alpine doesn't keep any fixed stack limit for the main thread. The pthread_getattr_np on Alpine gets the stack size by scanning pages from the bottom of the stack until it finds the stack guard.

Alpine Linux has a very small default stack size limit, about 80kB.
This is not enough for running coreclr apps.

This change enables overriding the default stack size using the
COMPlus_DefaultStackSize env variable. For Alpine, it also sets
the default stack size to the same value we use for Windows, which
is 1.5MB.
@janvorli janvorli force-pushed the ensure-enough-stack-on-alpine branch from 1447449 to eee9694 Compare August 22, 2017 16:15
@janvorli
Copy link
Member Author

@jkotas I've reflected your feedback and amended my commit.

@swgillespie
Copy link

@janvorli Yep, they should be really dead (#11379). I didn't realize there was a script for this, apologies for letting these remain!

@janvorli
Copy link
Member Author

@dotnet-bot test Tizen armel Cross Release Build please

@janvorli janvorli merged commit 5f2d1ec into dotnet:master Aug 23, 2017
@karelz karelz modified the milestone: 2.1.0 Aug 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants