You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/07/07 14:56:39 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #6978: Add option for hybrid global and thread session pools
shinrich opened a new pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978
Came up with this option while debugging the issue fixed by PR #6796
Even with that fixed, the hybrid approach looks like it is a useful mid-ground between minimizing number of connections to origin and minimizing lock contention. We plan on experimenting with this option some more. I wanted to share this approach to get feedback from the community.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#issuecomment-666406521
In my experiment, proxy.config.http.server_session_sharing.pool_hybrid_limit was set to -1. I've only played a little bit with different hybrid limits. Most of my efforts have been looking at all on or all off. May not be worth the complexity of being able to adjust this limit.
We are running with autoscale == 1.0, which would be 48 ET_NET threads.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#issuecomment-709656223
Pushed two additional commits. One to address @masaori335's method naming concerns. And another to simplify the config and associated documentation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#discussion_r460591651
##########
File path: proxy/http/HttpSessionManager.cc
##########
@@ -369,6 +370,25 @@ HttpSessionManager::acquire_session(Continuation * /* cont ATS_UNUSED */, sockad
to_return = nullptr;
}
+ // Otherwise, check the thread pool first
+ if (this->get_pool_type() == TS_SERVER_SESSION_SHARING_POOL_THREAD || this->get_hybrid_limit() != 0) {
+ retval = acquire_session(ip, hostname_hash, sm, match_style, TS_SERVER_SESSION_SHARING_POOL_THREAD);
+ }
+
+ // If you didn't get a match, and the global pool is an option go there.
+ if (retval != HSM_DONE && (TS_SERVER_SESSION_SHARING_POOL_GLOBAL == this->get_pool_type())) {
+ retval = acquire_session(ip, hostname_hash, sm, match_style, TS_SERVER_SESSION_SHARING_POOL_GLOBAL);
+ }
+ return retval;
+}
+
+HSMresult_t
+HttpSessionManager::acquire_session(sockaddr const *ip, CryptoHash const &hostname_hash, HttpSM *sm,
Review comment:
Could you rename like `_acquire_session()` or something else? Having public and private functions in the same name looks confusing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#discussion_r518739913
##########
File path: proxy/http/HttpSessionManager.cc
##########
@@ -446,6 +466,10 @@ HttpSessionManager::release_session(Http1ServerSession *to_release)
MUTEX_TRY_LOCK(lock, pool->mutex, ethread);
if (lock.is_locked()) {
Review comment:
Hmmm, so the try-lock fails, and we simply just close the session (without this new code)? Why can't we just re-schedule the release event to try again 10ms (or whatever, existing config) later? This wouldn't affect the client I assume (since the txn is done).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#issuecomment-664054893
Interesting. Would you share the setting of `proxy.config.http.server_session_sharing.pool_hybrid_limit` with your experiment?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 edited a comment on pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
masaori335 edited a comment on pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#issuecomment-664054893
Interesting. Would you share the setting of `proxy.config.http.server_session_sharing.pool_hybrid_limit` and the number of ET_NET thread with your experiment?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#discussion_r502048357
##########
File path: doc/admin-guide/files/records.config.en.rst
##########
@@ -990,6 +990,22 @@ mptcp
``thread`` Re-use sessions from a per-thread pool.
========== =================================================================
+.. ts:cv:: CONFIG proxy.config.http.server_session_sharing.pool_hybrid_limit INT 0
+
+ Setting :ts:cv:`proxy.config.http.server_session_sharing.pool` to global can reduce
+ the number of connections to origin for some traffic loads. However, if many
+ execute threads are active, the thread contention on the global pool can reduce the
+ lifetime of connections to origin and reduce effective origin connection reuse. The
+ :ts:cv:`proxy.config.http.server_session_sharing.pool_hybrid_limit` setting lets you
+ enable a hybrid pool mode using both global and per thread pools. If hybrid mode
+ is enabled, sessons are returned to the local thread pool if the global pool lock is
+ not acquired rather than just closing the origin connection as is the case in standard
+ global mode.
+
+ Setting it to 0, disables hybrid mode. Setting it to -1, enables hybrid
Review comment:
Agreed. Adding a setting which there is no way to find a better value for than "on or off" doesn't make sense, and does more harm. I'd say nuke this setting for now (until it's shown to have a need), and add this hybrid model to the existing setting with another numeric value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#discussion_r502048357
##########
File path: doc/admin-guide/files/records.config.en.rst
##########
@@ -990,6 +990,22 @@ mptcp
``thread`` Re-use sessions from a per-thread pool.
========== =================================================================
+.. ts:cv:: CONFIG proxy.config.http.server_session_sharing.pool_hybrid_limit INT 0
+
+ Setting :ts:cv:`proxy.config.http.server_session_sharing.pool` to global can reduce
+ the number of connections to origin for some traffic loads. However, if many
+ execute threads are active, the thread contention on the global pool can reduce the
+ lifetime of connections to origin and reduce effective origin connection reuse. The
+ :ts:cv:`proxy.config.http.server_session_sharing.pool_hybrid_limit` setting lets you
+ enable a hybrid pool mode using both global and per thread pools. If hybrid mode
+ is enabled, sessons are returned to the local thread pool if the global pool lock is
+ not acquired rather than just closing the origin connection as is the case in standard
+ global mode.
+
+ Setting it to 0, disables hybrid mode. Setting it to -1, enables hybrid
Review comment:
Agreed. Adding a setting which there is no way to find a better value for than "on or off" doesn't make sense, and does more harm. I'd say nuke this setting for now (until it's shown to have a need), and add this hybrid model to the existing setting with another numeric value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#issuecomment-723379504
Thinking about this some more , something feels fishy. How can there be so much pressure on this mutex? I mean, Linux can resolve this 100s of thousands per second without problem. So I’m wondering, is the critical section in this lock too long and/or to slow? If not, we should just be able to change this from a try lock to a regular blocking lock
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich edited a comment on pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
shinrich edited a comment on pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#issuecomment-663228884
I did some side-by-side measurements in with three machines in one of our colo's between global pool, hybrid pool, and thread pool. I ran a python script over the 15 minute squid.log files to calculate the latencies for transactions that go to origin. The script also computed what percent of those transactions reused an existing origin connection. I combined that with the connection count information from ysar (which pulls traffic_server metrics once a minute).
Reuse Model|Median Latency|90% Latency|99% Latency|% Trans reusing origin connection|concurrent open origin connections
-----------|-------------|-----------|-----------|----------------------------------|----------------------------------
Global|26ms|293|964|71.48|1972
Hybrid|21ms|280|928|78.91|3668
Thread|21ms|284|949|78.36|7998
The global pool has the smallest number of origin connections, but the latency numbers are better for hybrid and thread. Those two models behave similarly, but the hybrid has far fewer open connections.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#discussion_r460591289
##########
File path: doc/admin-guide/files/records.config.en.rst
##########
@@ -990,6 +990,22 @@ mptcp
``thread`` Re-use sessions from a per-thread pool.
========== =================================================================
+.. ts:cv:: CONFIG proxy.config.http.server_session_sharing.pool_hybrid_limit INT 0
+
+ Setting :ts:cv:`proxy.config.http.server_session_sharing.pool` to global can reduce
+ the number of connections to origin for some traffic loads. However, if many
+ execute threads are active, the thread contention on the global pool can reduce the
+ lifetime of connections to origin and reduce effective origin connection reuse. The
+ :ts:cv:`proxy.config.http.server_session_sharing.pool_hybrid_limit` setting lets you
+ enable a hybrid pool mode using both global and per thread pools. If hybrid mode
+ is enabled, sessons are returned to the local thread pool if the global pool lock is
+ not acquired rather than just closing the origin connection as is the case in standard
+ global mode.
+
+ Setting it to 0, disables hybrid mode. Setting it to -1, enables hybrid
Review comment:
This looks a bit vague. The hybrid mode is enabled or disabled regardless of the setting of `proxy.config.http.server_session_sharing.pool`?
I'd recommend below to make it clear.
1. add `hybrid` mode to `proxy.config.http.server_session_sharing.pool`
2. `proxy.config.http.server_session_sharing.pool_hybrid_limit` works with `hybrid` mode only
3. `0` for no limit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a change in pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#discussion_r463046907
##########
File path: doc/admin-guide/files/records.config.en.rst
##########
@@ -990,6 +990,22 @@ mptcp
``thread`` Re-use sessions from a per-thread pool.
========== =================================================================
+.. ts:cv:: CONFIG proxy.config.http.server_session_sharing.pool_hybrid_limit INT 0
+
+ Setting :ts:cv:`proxy.config.http.server_session_sharing.pool` to global can reduce
+ the number of connections to origin for some traffic loads. However, if many
+ execute threads are active, the thread contention on the global pool can reduce the
+ lifetime of connections to origin and reduce effective origin connection reuse. The
+ :ts:cv:`proxy.config.http.server_session_sharing.pool_hybrid_limit` setting lets you
+ enable a hybrid pool mode using both global and per thread pools. If hybrid mode
+ is enabled, sessons are returned to the local thread pool if the global pool lock is
+ not acquired rather than just closing the origin connection as is the case in standard
+ global mode.
+
+ Setting it to 0, disables hybrid mode. Setting it to -1, enables hybrid
Review comment:
Yes, as it stands, it is using hybrid limit as a modifier to the pool setting. As I noted below, it isn't clear to me from my testing so far that you gain anything by messing with the limit beyond turning it all the way on (-1) or off (0). In that case, I agree that just making a hybrid pool option would be a clearer config.
I guess I could keep around the hybrid_limit has a lesser used tuning option that is set to -1 by default but only used if pool == hybrid.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich merged pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
shinrich merged pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a change in pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#discussion_r518913271
##########
File path: proxy/http/HttpSessionManager.cc
##########
@@ -446,6 +466,10 @@ HttpSessionManager::release_session(Http1ServerSession *to_release)
MUTEX_TRY_LOCK(lock, pool->mutex, ethread);
if (lock.is_locked()) {
Review comment:
I did try the reschedule a few years back. It didn't help things because our global pool is already heavily contended. Adding retries just adds to the contention. Even for this PR, I tried a version that attempted to put the sessions back into the global pool on the next use even if we had temporarily put them in the thread pool. Just that change increased number of connections to origin and reduced connection reuse. I can give the reschedule another attempt, but probably not until January. I'm heads down in other issues for the rest of the quarter.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich edited a comment on pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
shinrich edited a comment on pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#issuecomment-663228884
I did some side-by-side measurements in with three machines in one of our colo's between global pool, hybrid pool, and thread pool. I ran a python script over the 15 minute squid.log files to calculate the latencies for transactions that go to origin. The script also computed what percent of those transactions reused an existing origin connection. I combined that with the connection count information from ysar (which pulls traffic_server metrics once a minute).
Reuse Model|Median Latency|90% Latency|99% Latency|% Trans reusing origin connection|concurrent open origin connections
Global|26ms|293|964|71.48|1972
Hybrid|21ms|280|928|78.91|3668
Thread|21ms|284|949|78.36|7998
The global pool has the smallest number of origin connections, but the latency numbers are better for hybrid and thread. Those two models behave similarly, but the hybrid has far fewer open connections.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#issuecomment-677892640
[approve ci docs]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #6978: Add option for hybrid global and thread session pools
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #6978:
URL: https://github.com/apache/trafficserver/pull/6978#issuecomment-663228884
I did some side-by-side measurements in with three machines in one of our colo's between global pool, hybrid pool, and thread pool. I ran a python script over the 15 minute squid.log files to calculate the latencies for transactions that go to origin. The script also computed what percent of those transactions reused an existing origin connection. I combined that with the connection count information from ysar (which pulls traffic_server metrics once a minute).
||Reuse Model||Median Latency||90% Latency||99% Latency||% Trans reusing origin connection||concurrent open origin connections||
|Global|26ms|293|964|71.48|1972|
|Hybrid|21ms|280|928|78.91|3668|
|Thread|21ms|284|949|78.36|7998|
The global pool has the smallest number of origin connections, but the latency numbers are better for hybrid and thread. Those two models behave similarly, but the hybrid has far fewer open connections.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org