[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-18 Thread PSUdaemon 
Github user PSUdaemon commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r83922132
  
--- Diff: iocore/eventsystem/EventSystem.cc ---
@@ -51,5 +52,15 @@ ink_event_system_init(ModuleVersion v)
   if (default_large_iobuffer_size > max_iobuffer_size) {
 default_large_iobuffer_size = max_iobuffer_size;
   }
-  init_buffer_allocators();
+
+#ifdef MADV_DONTDUMP // This should only exist on Linux 3.4 and higher.
+  RecBool dont_dump_enabled = true;
+  RecGetRecordBool("proxy.config.allocator.dontdump_iobuffers", (RecBool 
*)_dump_enabled, false);
--- End diff --

Is this case still needed?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-18 Thread jrushford 
Github user jrushford commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r83866632
  
--- Diff: 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;
--- End diff --

@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?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-13 Thread jrushford 
Github user jrushford commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r83292609
  
--- Diff: iocore/eventsystem/EventSystem.cc ---
@@ -51,5 +51,15 @@ ink_event_system_init(ModuleVersion v)
   if (default_large_iobuffer_size > max_iobuffer_size) {
 default_large_iobuffer_size = max_iobuffer_size;
   }
+
+#ifdef MADV_DONTDUMP // This should only exist on Linux 3.4 and higher.
+  bool dont_dump_enabled;
--- End diff --

done


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-13 Thread jrushford 
Github user jrushford commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r83227421
  
--- Diff: iocore/eventsystem/EventSystem.cc ---
@@ -51,5 +51,15 @@ ink_event_system_init(ModuleVersion v)
   if (default_large_iobuffer_size > max_iobuffer_size) {
 default_large_iobuffer_size = max_iobuffer_size;
   }
+
+#ifdef MADV_DONTDUMP // This should only exist on Linux 3.4 and higher.
+  bool dont_dump_enabled;
--- End diff --

done


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-12 Thread jpeach 
Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r83048130
  
--- Diff: iocore/eventsystem/EventSystem.cc ---
@@ -51,5 +51,15 @@ ink_event_system_init(ModuleVersion v)
   if (default_large_iobuffer_size > max_iobuffer_size) {
 default_large_iobuffer_size = max_iobuffer_size;
   }
+
+#ifdef MADV_DONTDUMP // This should only exist on Linux 3.4 and higher.
+  bool dont_dump_enabled;
--- End diff --

Yes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-12 Thread PSUdaemon 
Github user PSUdaemon commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r83040861
  
--- Diff: iocore/eventsystem/EventSystem.cc ---
@@ -51,5 +51,15 @@ ink_event_system_init(ModuleVersion v)
   if (default_large_iobuffer_size > max_iobuffer_size) {
 default_large_iobuffer_size = max_iobuffer_size;
   }
+
+#ifdef MADV_DONTDUMP // This should only exist on Linux 3.4 and higher.
+  bool dont_dump_enabled;
--- End diff --

If the default is true, shouldn't we initialize to true?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-12 Thread PSUdaemon 
Github user PSUdaemon commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r83040763
  
--- Diff: 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;
--- End diff --

I think what we ran into last time was a dependency ordering issue, which 
is why we scrapped the whole idea of a config variable in the first iteration.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-12 Thread jpeach 
Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r83016299
  
--- Diff: iocore/eventsystem/EventSystem.cc ---
@@ -51,5 +51,15 @@ ink_event_system_init(ModuleVersion v)
   if (default_large_iobuffer_size > max_iobuffer_size) {
 default_large_iobuffer_size = max_iobuffer_size;
   }
+
+#ifdef MADV_DONTDUMP // This should only exist on Linux 3.4 and higher.
+  bool dont_dump_enabled;
--- End diff --

Use RecBool for the type so it matches the cast below.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-12 Thread jpeach 
Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r83016016
  
--- Diff: iocore/eventsystem/EventSystem.cc ---
@@ -51,5 +51,15 @@ ink_event_system_init(ModuleVersion v)
   if (default_large_iobuffer_size > max_iobuffer_size) {
 default_large_iobuffer_size = max_iobuffer_size;
   }
+
+#ifdef MADV_DONTDUMP // This should only exist on Linux 3.4 and higher.
+  bool dont_dump_enabled;
--- End diff --

Initialize to false.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-12 Thread jpeach 
Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r83016651
  
--- Diff: 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;
--- End diff --

We added another global variable? Can't we do this locally in 
``init_buffer_allocators()``?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-11 Thread PSUdaemon 
Github user PSUdaemon commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r82918082
  
--- Diff: mgmt/RecordsConfig.cc ---
@@ -1449,6 +1449,8 @@ static const RecordElement RecordsConfig[] =
   ,
   {RECT_CONFIG, "proxy.config.allocator.hugepages", RECD_INT, "0", 
RECU_RESTART_TS, RR_NULL, RECC_NULL, "[0-1]", RECA_NULL}
   ,
+  {RECT_CONFIG, "proxy.config.allocator.dontdump_iobuffers", RECD_INT, 
"0", RECU_RESTART_TS, RR_NULL, RECC_NULL, "[0-1]", RECA_NULL}
--- End diff --

I'm fine with it being backported if @bryancall is. Though, I don't think 
we absolutely have to. In 7.0.x it's on by default now, you just can't disable 
it.

I'm considering backporting this to 6.2.x as well due to the trouble it 
causes on older kernels.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-11 Thread zwoop 
Github user zwoop commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r82917530
  
--- Diff: mgmt/RecordsConfig.cc ---
@@ -1449,6 +1449,8 @@ static const RecordElement RecordsConfig[] =
   ,
   {RECT_CONFIG, "proxy.config.allocator.hugepages", RECD_INT, "0", 
RECU_RESTART_TS, RR_NULL, RECC_NULL, "[0-1]", RECA_NULL}
   ,
+  {RECT_CONFIG, "proxy.config.allocator.dontdump_iobuffers", RECD_INT, 
"0", RECU_RESTART_TS, RR_NULL, RECC_NULL, "[0-1]", RECA_NULL}
--- End diff --

I'm ok with that, but that means this also should be back ported to 7.0.0.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-11 Thread jrushford 
Github user jrushford commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r82902604
  
--- Diff: iocore/eventsystem/EventSystem.cc ---
@@ -51,5 +51,15 @@ ink_event_system_init(ModuleVersion v)
   if (default_large_iobuffer_size > max_iobuffer_size) {
 default_large_iobuffer_size = max_iobuffer_size;
   }
+
+#ifdef MADV_DONTDUMP // This should only exist on Linux 3.4 and higher.
+  bool dont_dump_enabled;
+  RecGetRecordBool("proxy.config.allocator.dontdump_iobuffers", (RecBool 
*)_dump_enabled, false);
+
+  if (dont_dump_enabled) {
+iobuffer_advice = MADV_DONTDUMP;
--- End diff --

Changes were made and squashed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-11 Thread jrushford 
Github user jrushford commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r82898723
  
--- Diff: mgmt/RecordsConfig.cc ---
@@ -1449,6 +1449,8 @@ static const RecordElement RecordsConfig[] =
   ,
   {RECT_CONFIG, "proxy.config.allocator.hugepages", RECD_INT, "0", 
RECU_RESTART_TS, RR_NULL, RECC_NULL, "[0-1]", RECA_NULL}
   ,
+  {RECT_CONFIG, "proxy.config.allocator.dontdump_iobuffers", RECD_INT, 
"0", RECU_RESTART_TS, RR_NULL, RECC_NULL, "[0-1]", RECA_NULL}
--- End diff --

Okay, I'll make the change and update the doc.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-11 Thread jrushford 
Github user jrushford commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r82898472
  
--- Diff: iocore/eventsystem/EventSystem.cc ---
@@ -51,5 +51,15 @@ ink_event_system_init(ModuleVersion v)
   if (default_large_iobuffer_size > max_iobuffer_size) {
 default_large_iobuffer_size = max_iobuffer_size;
   }
+
+#ifdef MADV_DONTDUMP // This should only exist on Linux 3.4 and higher.
+  bool dont_dump_enabled;
+  RecGetRecordBool("proxy.config.allocator.dontdump_iobuffers", (RecBool 
*)_dump_enabled, false);
+
+  if (dont_dump_enabled) {
+iobuffer_advice = MADV_DONTDUMP;
--- End diff --

Good idea, I'll make the change.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-11 Thread PSUdaemon 
Github user PSUdaemon commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r82896873
  
--- Diff: iocore/eventsystem/EventSystem.cc ---
@@ -51,5 +51,15 @@ ink_event_system_init(ModuleVersion v)
   if (default_large_iobuffer_size > max_iobuffer_size) {
 default_large_iobuffer_size = max_iobuffer_size;
   }
+
+#ifdef MADV_DONTDUMP // This should only exist on Linux 3.4 and higher.
+  bool dont_dump_enabled;
+  RecGetRecordBool("proxy.config.allocator.dontdump_iobuffers", (RecBool 
*)_dump_enabled, false);
+
+  if (dont_dump_enabled) {
+iobuffer_advice = MADV_DONTDUMP;
--- End diff --

Should we do `|=` incase we do more in the future?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-11 Thread PSUdaemon 
Github user PSUdaemon commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1097#discussion_r82896517
  
--- Diff: mgmt/RecordsConfig.cc ---
@@ -1449,6 +1449,8 @@ static const RecordElement RecordsConfig[] =
   ,
   {RECT_CONFIG, "proxy.config.allocator.hugepages", RECD_INT, "0", 
RECU_RESTART_TS, RR_NULL, RECC_NULL, "[0-1]", RECA_NULL}
   ,
+  {RECT_CONFIG, "proxy.config.allocator.dontdump_iobuffers", RECD_INT, 
"0", RECU_RESTART_TS, RR_NULL, RECC_NULL, "[0-1]", RECA_NULL}
--- End diff --

I think we should default this to 1


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1097: Make the use of madvise() with MADV_DONTDU...

2016-10-11 Thread jrushford 
GitHub user jrushford opened a pull request:

https://github.com/apache/trafficserver/pull/1097

Make the use of madvise() with MADV_DONTDUMP configurable.

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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/jrushford/trafficserver TS-4957

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/trafficserver/pull/1097.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1097


commit f1c37818622fc0fc4e05614c7f1f83a10fe8c063
Author: John J. Rushford 
Date:   2016-10-11T21:37:01Z

Make the use of madvise() with MADV_DONTDUMP configurable.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---