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

TS-4957: Make the use of madvise() with MADV_DONTDUMP configurable. #1097

Merged
merged 1 commit into from Oct 19, 2016

Conversation

Projects
None yet
5 participants
@jrushford
Contributor

jrushford commented Oct 11, 2016

We have seen high cpu loads and high time to serve problems in our production platform when using madvise() with the MADV_DONTDUMP option. This appears to be a kernel issue with madvise(). in order to avoid having to rebuild ats, this PR uses a patch from TS-3417 to make the use of madvise() with the MADV_DONTDUMP flag configurable.

Show outdated Hide outdated mgmt/RecordsConfig.cc Outdated
Show outdated Hide outdated iocore/eventsystem/EventSystem.cc Outdated
@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 11, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/971/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 11, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/863/ for details.

@jrushford

This comment has been minimized.

Show comment
Hide comment
@jrushford

jrushford Oct 11, 2016

Contributor

@PSUdaemon - requested changes have been made and squashed.

Contributor

jrushford commented Oct 11, 2016

@PSUdaemon - requested changes have been made and squashed.

@jrushford

This comment has been minimized.

Show comment
Hide comment
@jrushford

jrushford Oct 11, 2016

Contributor

[approve ci]

Contributor

jrushford commented Oct 11, 2016

[approve ci]

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 11, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/974/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 11, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/866/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 11, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/867/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 11, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/975/ for details.

@zwoop zwoop added the Core label Oct 12, 2016

@zwoop zwoop added this to the 7.1.0 milestone Oct 12, 2016

Show outdated Hide outdated iocore/eventsystem/EventSystem.cc Outdated
Show outdated Hide outdated iocore/eventsystem/EventSystem.cc Outdated
Show outdated Hide outdated iocore/eventsystem/I_IOBuffer.h Outdated
@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 13, 2016

Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/885/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 13, 2016

FreeBSD build failed! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/993/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 13, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/994/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 13, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/886/ for details.

@jpeach

I still think we can do this without extra globals.

@jpeach

This comment has been minimized.

Show comment
Hide comment
@jpeach

jpeach Oct 18, 2016

Contributor

No I think it is trivial to remove the global and we should just do it

On Oct 18, 2016, at 7:29 AM, John J. Rushford notifications@github.com wrote:

@jrushford commented on this pull request.

In iocore/eventsystem/I_IOBuffer.h:

@@ -58,6 +58,7 @@ class VIO;
inkcoreapi extern int64_t max_iobuffer_size;
extern int64_t default_small_iobuffer_size;
extern int64_t default_large_iobuffer_size; // matched to size of OS buffers
+extern int iobuffer_advice;
@jpeach I'd like to land this as is and write a JIRA to come back to the global variable once we've had time to evaluate this. Are you okay with this?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Contributor

jpeach commented Oct 18, 2016

No I think it is trivial to remove the global and we should just do it

On Oct 18, 2016, at 7:29 AM, John J. Rushford notifications@github.com wrote:

@jrushford commented on this pull request.

In iocore/eventsystem/I_IOBuffer.h:

@@ -58,6 +58,7 @@ class VIO;
inkcoreapi extern int64_t max_iobuffer_size;
extern int64_t default_small_iobuffer_size;
extern int64_t default_large_iobuffer_size; // matched to size of OS buffers
+extern int iobuffer_advice;
@jpeach I'd like to land this as is and write a JIRA to come back to the global variable once we've had time to evaluate this. Are you okay with this?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 18, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1040/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 18, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/933/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 18, 2016

Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/936/ for details.

@jrushford

This comment has been minimized.

Show comment
Hide comment
@jrushford

jrushford Oct 18, 2016

Contributor

@jpeach and @PSUdaemon - I've made iobuffer_advice a local variable. Tested and it is working okay.

Contributor

jrushford commented Oct 18, 2016

@jpeach and @PSUdaemon - I've made iobuffer_advice a local variable. Tested and it is working okay.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 18, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1044/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 18, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/937/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 18, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1045/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 18, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/938/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 18, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1046/ for details.

@jrushford jrushford changed the title from Make the use of madvise() with MADV_DONTDUMP configurable. to TS-4957: Make the use of madvise() with MADV_DONTDUMP configurable. Oct 18, 2016

@jpeach

jpeach approved these changes Oct 19, 2016

@jpeach

jpeach approved these changes Oct 19, 2016

Please squash before merging.

I'm OK with these changes, but if you agree, I think the configuration name can be improved. First, it is better to avoid double-negatives. Second we already have proxy.config.core_limit. Consider proxy.config.core_include_iobuffers?

@jrushford

This comment has been minimized.

Show comment
Hide comment
@jrushford

jrushford Oct 19, 2016

Contributor

Thanks @jpeach. FWIW, this patch is originally from TS-3417 and @PSUdaemon and I dug it up after we discovered we had a problem with madvise() and to fix, required deploying a new rpm to 1300+ proxies. I'm okay with the name change and will make it if @zwoop and @PSUdaemon agree.

Contributor

jrushford commented Oct 19, 2016

Thanks @jpeach. FWIW, this patch is originally from TS-3417 and @PSUdaemon and I dug it up after we discovered we had a problem with madvise() and to fix, required deploying a new rpm to 1300+ proxies. I'm okay with the name change and will make it if @zwoop and @PSUdaemon agree.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 19, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1056/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 19, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/948/ for details.

@jrushford jrushford merged commit 514d3fd into apache:master Oct 19, 2016

@PSUdaemon

This comment has been minimized.

Show comment
Hide comment
@PSUdaemon

PSUdaemon Oct 19, 2016

Contributor

I don't care about the name as long as it's not uselessly confusing.

Contributor

PSUdaemon commented Oct 19, 2016

I don't care about the name as long as it's not uselessly confusing.

@jrushford jrushford deleted the jrushford:TS-4957 branch Nov 2, 2016

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