You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by am...@apache.org on 2014/07/25 04:20:33 UTC

git commit: TS-2863: Enable FQDN selection for shared sessions.

Repository: trafficserver
Updated Branches:
  refs/heads/master be428d6f4 -> 0c358a4b1


TS-2863: Enable FQDN selection for shared sessions.


Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/0c358a4b
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/0c358a4b
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/0c358a4b

Branch: refs/heads/master
Commit: 0c358a4b166b3dbeb847634cc575e8dec06eff7f
Parents: be428d6
Author: Alan M. Carroll <am...@network-geographics.com>
Authored: Sat May 31 22:41:27 2014 -0500
Committer: Alan M. Carroll <am...@network-geographics.com>
Committed: Thu Jul 24 20:22:06 2014 -0500

----------------------------------------------------------------------
 doc/arch/hacking/config-var-impl.en.rst |   2 +-
 doc/arch/hacking/index.en.rst           |   2 +-
 iocore/eventsystem/I_EThread.h          |   4 +-
 iocore/eventsystem/UnixEThread.cc       |   2 +-
 lib/ts/INK_MD5.h                        |  26 ++-
 lib/ts/Map.h                            |   2 +-
 lib/ts/ink_code.cc                      |   4 +-
 lib/ts/ink_code.h                       |   2 +-
 lib/ts/ink_inet.h                       |  15 ++
 proxy/Plugin.h                          |   3 +
 proxy/http/HttpSM.cc                    |   8 +-
 proxy/http/HttpServerSession.h          |  10 +-
 proxy/http/HttpSessionManager.cc        | 290 ++++++++++++---------------
 proxy/http/HttpSessionManager.h         | 100 ++++++---
 14 files changed, 253 insertions(+), 217 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/doc/arch/hacking/config-var-impl.en.rst
----------------------------------------------------------------------
diff --git a/doc/arch/hacking/config-var-impl.en.rst b/doc/arch/hacking/config-var-impl.en.rst
index 75a80e1..2f3a584 100644
--- a/doc/arch/hacking/config-var-impl.en.rst
+++ b/doc/arch/hacking/config-var-impl.en.rst
@@ -206,7 +206,7 @@ Handling Updates
 
 The simplest mechanism for handling updates is the ``REC_EstablishStaticConfigXXX`` family of functions. This mechanism
 will cause the value in the indicated instance to be updated in place when an update to :file:`records.config` occurs.
-This is done asynchronously using atomic operations. Use of these variables must keep that in mind. Adding ``volatile`` to the declaration is likely to be a good idea.
+This is done asynchronously using atomic operations. Use of these variables must keep that in mind.
 
 If a variable requires additional handling when updated a callback can be registered which is called when the variable
 is updated. This is what the ``REC_EstablishStaticConfigXXX`` calls do internally with a callback that simply reads the

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/doc/arch/hacking/index.en.rst
----------------------------------------------------------------------
diff --git a/doc/arch/hacking/index.en.rst b/doc/arch/hacking/index.en.rst
index 84e997a..68fd419 100644
--- a/doc/arch/hacking/index.en.rst
+++ b/doc/arch/hacking/index.en.rst
@@ -25,6 +25,6 @@ This is a documentation stub on how to hack Apache Traffic Server. Here we try t
 and run unit or regression tests or how to inspect the state of the core with a debugger.
 
 .. toctree::
-   :maxdepth: 1
+   :maxdepth: 2
 
    config-var-impl.en

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/iocore/eventsystem/I_EThread.h
----------------------------------------------------------------------
diff --git a/iocore/eventsystem/I_EThread.h b/iocore/eventsystem/I_EThread.h
index f557471..1aca68d 100644
--- a/iocore/eventsystem/I_EThread.h
+++ b/iocore/eventsystem/I_EThread.h
@@ -44,7 +44,7 @@
 struct DiskHandler;
 struct EventIO;
 
-class SessionBucket;
+class ServerSessionPool;
 class Event;
 class Continuation;
 
@@ -322,7 +322,7 @@ public:
   ThreadType tt;
   Event *oneevent;              // For dedicated event thread
 
-  SessionBucket* l1_hash;
+  ServerSessionPool* server_session_pool;
 };
 
 /**

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/iocore/eventsystem/UnixEThread.cc
----------------------------------------------------------------------
diff --git a/iocore/eventsystem/UnixEThread.cc b/iocore/eventsystem/UnixEThread.cc
index bb21748..4c9fed9 100644
--- a/iocore/eventsystem/UnixEThread.cc
+++ b/iocore/eventsystem/UnixEThread.cc
@@ -60,7 +60,7 @@ EThread::EThread(ThreadType att, int anid)
     event_types(0),
     signal_hook(0),
     tt(att),
-    l1_hash(NULL)
+    server_session_pool(NULL)
 {
   ethreads_to_be_signalled = (EThread **)ats_malloc(MAX_EVENT_THREADS * sizeof(EThread *));
   memset((char *) ethreads_to_be_signalled, 0, MAX_EVENT_THREADS * sizeof(EThread *));

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/lib/ts/INK_MD5.h
----------------------------------------------------------------------
diff --git a/lib/ts/INK_MD5.h b/lib/ts/INK_MD5.h
index b046fa6..37585eb 100644
--- a/lib/ts/INK_MD5.h
+++ b/lib/ts/INK_MD5.h
@@ -1,6 +1,6 @@
 /** @file
 
-  A brief file description
+  MD5 support class.
 
   @section license License
 
@@ -48,7 +48,7 @@ struct INK_MD5
   {
     u64[0] = md5.u64[0];
     u64[1] = md5.u64[1];
-    return md5;
+    return *this;
   }
   uint32_t word(int i)
   {
@@ -58,18 +58,18 @@ struct INK_MD5
   {
     return u8[i];
   }
-  INK_MD5 & loadFromBuffer(char *md5_buf) {
+  INK_MD5 & loadFromBuffer(char const* md5_buf) {
     memcpy((void *) u8, (void *) md5_buf, 16);
     return (*this);
   }
-  INK_MD5 & storeToBuffer(char *md5_buf) {
+  INK_MD5 & storeToBuffer(char const* md5_buf) {
     memcpy((void *) md5_buf, (void *) u8, 16);
     return (*this);
   }
-  INK_MD5 & operator =(char *md5) {
+  INK_MD5 & operator =(char const* md5) {
     return (loadFromBuffer(md5));
   }
-  INK_MD5 & operator =(unsigned char *md5) {
+  INK_MD5 & operator =(unsigned char const* md5) {
     return (loadFromBuffer((char *) md5));
   }
 
@@ -77,13 +77,13 @@ struct INK_MD5
   {
     return (char *) memcpy((void *) md5_str, (void *) u8, 16);
   }
-  void encodeBuffer(unsigned char *buffer, int len)
+  void encodeBuffer(unsigned char const* buffer, int len)
   {
     ink_code_md5(buffer, len, u8);
   }
   void encodeBuffer(const char *buffer, int len)
   {
-    encodeBuffer((unsigned char *) buffer, len);
+    encodeBuffer(reinterpret_cast<unsigned char const*>(buffer), len);
   }
   char *str()
   {
@@ -132,10 +132,15 @@ struct INK_MD5
   {
     return u64[i];
   }
-  bool operator==(INK_MD5 const& md5)
+  bool operator==(INK_MD5 const& md5) const
   {
     return u64[0] == md5.u64[0] && u64[1] == md5.u64[1];
   }
+  bool operator != (INK_MD5 const& that) const
+  {
+    return !(*this == that);
+  }
+
   INK_MD5() {
     u64[0] = 0;
     u64[1] = 0;
@@ -144,6 +149,9 @@ struct INK_MD5
     u64[0] = a1;
     u64[1] = a2;
   }
+
+  /// Static default constructed instance.
+  static INK_MD5 const ZERO;
 };
 
 #endif

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/lib/ts/Map.h
----------------------------------------------------------------------
diff --git a/lib/ts/Map.h b/lib/ts/Map.h
index 27ea183..11d27a0 100644
--- a/lib/ts/Map.h
+++ b/lib/ts/Map.h
@@ -1385,7 +1385,7 @@ template < typename H > void
 TSHashTable<H>::clear() {
   Bucket null_bucket;
   // Remove the values but not the actual buckets.
-  for ( int i = 0 ; i < m_array.n ; ++i ) {
+  for ( size_t i = 0 ; i < m_array.n ; ++i ) {
     m_array[i] = null_bucket;
   }
   // Clear container data.

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/lib/ts/ink_code.cc
----------------------------------------------------------------------
diff --git a/lib/ts/ink_code.cc b/lib/ts/ink_code.cc
index 8cc80eb..f3a403f 100644
--- a/lib/ts/ink_code.cc
+++ b/lib/ts/ink_code.cc
@@ -24,8 +24,10 @@
 #include <string.h>
 #include <stdio.h>
 #include "ink_code.h"
+#include "INK_MD5.h"
 #include "ink_assert.h"
 
+INK_MD5 const INK_MD5::ZERO; // default constructed is correct.
 
 /**
   @brief Wrapper around MD5_Init
@@ -57,7 +59,7 @@ ink_code_incr_md5_final(char *sixteen_byte_hash_pointer, INK_DIGEST_CTX * contex
   @return always returns 0, maybe some error checking should be done
 */
 int
-ink_code_md5(unsigned char *input, int input_length, unsigned char *sixteen_byte_hash_pointer)
+ink_code_md5(unsigned char const* input, int input_length, unsigned char *sixteen_byte_hash_pointer)
 {
   MD5_CTX context;
 

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/lib/ts/ink_code.h
----------------------------------------------------------------------
diff --git a/lib/ts/ink_code.h b/lib/ts/ink_code.h
index 8e9eb38..70bc830 100644
--- a/lib/ts/ink_code.h
+++ b/lib/ts/ink_code.h
@@ -34,7 +34,7 @@ typedef MD5_CTX INK_DIGEST_CTX;
   Wrappers around the MD5 functions, all of this should be depericated and just use the functions directly
 */
 
-inkcoreapi int ink_code_md5(unsigned char *input, int input_length, unsigned char *sixteen_byte_hash_pointer);
+inkcoreapi int ink_code_md5(unsigned char const* input, int input_length, unsigned char *sixteen_byte_hash_pointer);
 inkcoreapi char *ink_code_md5_stringify(char *dest33, const size_t destSize, const char *md5);
 inkcoreapi char *ink_code_md5_stringify_fast(char *dest33, const char *md5);
 

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/lib/ts/ink_inet.h
----------------------------------------------------------------------
diff --git a/lib/ts/ink_inet.h b/lib/ts/ink_inet.h
index 30fc036..fcaca0d 100644
--- a/lib/ts/ink_inet.h
+++ b/lib/ts/ink_inet.h
@@ -105,6 +105,9 @@ union IpEndpoint {
   in_port_t& port();
   /// Port in network order.
   in_port_t port() const;
+
+  operator sockaddr* () { return &sa; }
+  operator sockaddr const* () const { return &sa; }
 };
 
 struct ink_gethostbyname_r_data
@@ -762,6 +765,18 @@ inline bool operator != (IpEndpoint const& lhs, IpEndpoint const& rhs) {
   return 0 != ats_ip_addr_cmp(&lhs.sa, &rhs.sa);
 }
 
+/// Compare address and port for equality.
+inline bool ats_ip_addr_port_eq(sockaddr const* lhs, sockaddr const* rhs) {
+  bool zret = false;
+  if (lhs->sa_family == rhs->sa_family && ats_ip_port_cast(lhs) == ats_ip_port_cast(rhs)) {
+    if (AF_INET == lhs->sa_family)
+      zret = ats_ip4_cast(lhs)->sin_addr.s_addr == ats_ip4_cast(rhs)->sin_addr.s_addr;
+    else if (AF_INET6 == lhs->sa_family)
+      zret = 0 == memcmp(&ats_ip6_cast(lhs)->sin6_addr, &ats_ip6_cast(rhs)->sin6_addr, sizeof(in6_addr));
+  }
+  return zret;
+}
+
 //@}
 
 /// Get IP TCP/UDP port.

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/proxy/Plugin.h
----------------------------------------------------------------------
diff --git a/proxy/Plugin.h b/proxy/Plugin.h
index 8f70f1a..4a23557 100644
--- a/proxy/Plugin.h
+++ b/proxy/Plugin.h
@@ -75,6 +75,9 @@ void plugin_init(void);
 class PluginIdentity
 {
  public:
+  /// Make sure destructor is virtual.
+  virtual ~PluginIdentity() {}
+
   /** Get the plugin tag.
       The returned string must have a lifetime at least as long as the plugin.
       @return A string identifying the plugin or @c NULL.

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/proxy/http/HttpSM.cc
----------------------------------------------------------------------
diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
index 1fdfd26..5d7239c 100644
--- a/proxy/http/HttpSM.cc
+++ b/proxy/http/HttpSM.cc
@@ -4527,11 +4527,10 @@ HttpSM::do_http_server_open(bool raw)
 
     if (existing_ss) {
       // [amc] Not sure if this is the best option, but we don't get here unless session sharing is disabled
-      // so there's point in further checking on the match or pool values. But why check anything? The
+      // so there's no point in further checking on the match or pool values. But why check anything? The
       // client has already exchanged a request with this specific origin server and has sent another one
       // shouldn't we just automatically keep the association?
-      if (ats_ip_addr_eq(&existing_ss->server_ip.sa, &t_state.current.server->addr.sa) &&
-          ats_ip_port_cast(&existing_ss->server_ip) == ats_ip_port_cast(&t_state.current.server->addr)) {
+      if (ats_ip_addr_port_eq(&existing_ss->server_ip.sa, &t_state.current.server->addr.sa)) {
         ua_session->attach_server_session(NULL);
         existing_ss->state = HSS_ACTIVE;
         this->attach_server_session(existing_ss);
@@ -4911,7 +4910,8 @@ HttpSM::release_server_session(bool serve_from_cache)
     return;
   }
 
-  if (t_state.current.server->keep_alive == HTTP_KEEPALIVE &&
+  if (TS_SERVER_SESSION_SHARING_MATCH_NONE != t_state.txn_conf->server_session_sharing_match &&
+      t_state.current.server->keep_alive == HTTP_KEEPALIVE &&
       t_state.hdr_info.server_response.valid() &&
       (t_state.hdr_info.server_response.status_get() == HTTP_STATUS_NOT_MODIFIED ||
        (t_state.hdr_info.server_request.method_get_wksidx() == HTTP_WKSIDX_HEAD

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/proxy/http/HttpServerSession.h
----------------------------------------------------------------------
diff --git a/proxy/http/HttpServerSession.h b/proxy/http/HttpServerSession.h
index c4fd75c..a788a35 100644
--- a/proxy/http/HttpServerSession.h
+++ b/proxy/http/HttpServerSession.h
@@ -71,7 +71,7 @@ public:
   HttpServerSession()
     : VConnection(NULL),
       hostname_hash(),
-      host_hash_computed(false), con_id(0), transact_count(0),
+      con_id(0), transact_count(0),
       state(HSS_INIT), to_parent_proxy(false), server_trans_stat(0),
       private_session(false),
       sharing_match(TS_SERVER_SESSION_SHARING_MATCH_BOTH),
@@ -119,7 +119,6 @@ public:
   // Keys for matching hostnames
   IpEndpoint server_ip;
   INK_MD5 hostname_hash;
-  bool host_hash_computed;
 
   int64_t con_id;
   int transact_count;
@@ -145,8 +144,8 @@ public:
   TSServerSessionSharingPoolType sharing_pool;
   //  int share_session;
 
-  LINK(HttpServerSession, lru_link);
-  LINK(HttpServerSession, hash_link);
+  LINK(HttpServerSession, ip_hash_link);
+  LINK(HttpServerSession, host_hash_link);
 
   // Keep track of connection limiting and a pointer to the
   // singleton that keeps track of the connection counts.
@@ -176,9 +175,8 @@ extern ClassAllocator<HttpServerSession> httpServerSessionAllocator;
 inline void
 HttpServerSession::attach_hostname(const char *hostname)
 {
-  if (!host_hash_computed) {
+  if (INK_MD5::ZERO == hostname_hash) {
     ink_code_md5((unsigned char *) hostname, strlen(hostname), (unsigned char *) &hostname_hash);
-    host_hash_computed = true;
   }
 }
 #endif

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/proxy/http/HttpSessionManager.cc
----------------------------------------------------------------------
diff --git a/proxy/http/HttpSessionManager.cc b/proxy/http/HttpSessionManager.cc
index 8f68f79..e42dd2c 100644
--- a/proxy/http/HttpSessionManager.cc
+++ b/proxy/http/HttpSessionManager.cc
@@ -36,35 +36,103 @@
 #include "HttpSM.h"
 #include "HttpDebugNames.h"
 
-#define FIRST_LEVEL_HASH(x)   ats_ip_hash(x) % HSM_LEVEL1_BUCKETS
-#define SECOND_LEVEL_HASH(x)  ats_ip_hash(x) % HSM_LEVEL2_BUCKETS
-
 // Initialize a thread to handle HTTP session management
 void
 initialize_thread_for_http_sessions(EThread *thread, int /* thread_index ATS_UNUSED */)
 {
-  thread->l1_hash = new SessionBucket[HSM_LEVEL1_BUCKETS];
-  for (int i = 0; i < HSM_LEVEL1_BUCKETS; ++i)
-    thread->l1_hash[i].mutex = new_ProxyMutex();
-  //thread->l1_hash[i].mutex = thread->mutex;
+  thread->server_session_pool = new ServerSessionPool;
 }
 
-
 HttpSessionManager httpSessionManager;
 
-SessionBucket::SessionBucket()
-  : Continuation(NULL)
+ServerSessionPool::ServerSessionPool()
+  : Continuation(new_ProxyMutex()), m_ip_pool(1023), m_host_pool(1023)
 {
-  SET_HANDLER(&SessionBucket::session_handler);
+  SET_HANDLER(&ServerSessionPool::eventHandler);
+  m_ip_pool.setExpansionPolicy(IPHashTable::MANUAL);
+  m_host_pool.setExpansionPolicy(HostHashTable::MANUAL);
+}
+
+void
+ServerSessionPool::purge()
+{ 
+  for ( IPHashTable::iterator last = m_ip_pool.end(), spot = m_ip_pool.begin() ; spot != last ; ++spot ) {
+    spot->do_io_close();
+  }
+  m_ip_pool.clear();
+  m_host_pool.clear();
+}
+
+bool
+ServerSessionPool::match(HttpServerSession* ss, sockaddr const* addr, INK_MD5 const& hostname_hash, TSServerSessionSharingMatchType match_style)
+{
+  return TS_SERVER_SESSION_SHARING_MATCH_NONE != match_style && // if no matching allowed, fail immediately.
+    // The hostname matches if we're not checking it or it (and the port!) is a match.
+    (TS_SERVER_SESSION_SHARING_MATCH_IP == match_style ||  (ats_ip_port_cast(addr) == ats_ip_port_cast(ss->server_ip) && ss->hostname_hash == hostname_hash)) &&
+    // The IP address matches if we're not checking it or it is a match.
+    (TS_SERVER_SESSION_SHARING_MATCH_HOST == match_style || ats_ip_addr_port_eq(ss->server_ip, addr))
+    ;
+}
+
+HttpServerSession*
+ServerSessionPool::acquireSession(sockaddr const* addr, INK_MD5 const& hostname_hash, TSServerSessionSharingMatchType match_style)
+{
+  HttpServerSession* zret = NULL;
+
+  if (TS_SERVER_SESSION_SHARING_MATCH_HOST == match_style) {
+    // This is broken out because only in this case do we check the host hash first.
+    HostHashTable::Location loc = m_host_pool.find(hostname_hash);
+    if (loc) {
+      zret = loc;
+      m_host_pool.remove(loc);
+      m_ip_pool.remove(m_ip_pool.find(zret));
+    }
+  } else if (TS_SERVER_SESSION_SHARING_MATCH_NONE != match_style) { // matching is not disabled.
+    IPHashTable::Location loc = m_ip_pool.find(addr);
+    // If we're matching on the IP address we're done, this one is good enough.
+    // Otherwise we need to scan further matches to match the host name as well.
+    // Note we don't have to check the port because it's checked as part of the IP address key.
+    if (TS_SERVER_SESSION_SHARING_MATCH_IP != match_style) {
+      while (loc && loc->hostname_hash != hostname_hash)
+        ++loc;
+    }
+    if (loc) {
+      zret = loc;
+      m_ip_pool.remove(loc);
+      m_host_pool.remove(m_host_pool.find(zret));
+    }
+  }
+  return zret;
+}
+
+void
+ServerSessionPool::releaseSession(HttpServerSession* ss)
+{
+  ss->state = HSS_KA_SHARED;
+  // Now we need to issue a read on the connection to detect
+  //  if it closes on us.  We will get called back in the
+  //  continuation for this bucket, ensuring we have the lock
+  //  to remove the connection from our lists
+  ss->do_io_read(this, INT64_MAX, ss->read_buffer);
+
+  // Transfer control of the write side as well
+  ss->do_io_write(this, 0, NULL);
+
+  // we probably don't need the active timeout set, but will leave it for now
+  ss->get_netvc()->set_inactivity_timeout(ss->get_netvc()->get_inactivity_timeout());
+  ss->get_netvc()->set_active_timeout(ss->get_netvc()->get_active_timeout());
+  // put it in the pools.
+  m_ip_pool.insert(ss);
+  m_host_pool.insert(ss);
+
+  Debug("http_ss", "[%" PRId64 "] [release session] " "session placed into shared pool", ss->con_id);
 }
 
-// int SessionBucket::session_handler(int event, void* data)
-//
 //   Called from the NetProcessor to left us know that a
 //    connection has closed down
 //
 int
-SessionBucket::session_handler(int event, void *data)
+ServerSessionPool::eventHandler(int event, void *data)
 {
   NetVConnection *net_vc = NULL;
   HttpServerSession *s = NULL;
@@ -78,7 +146,7 @@ SessionBucket::session_handler(int event, void *data)
   case VC_EVENT_ERROR:
   case VC_EVENT_INACTIVITY_TIMEOUT:
   case VC_EVENT_ACTIVE_TIMEOUT:
-    net_vc = (NetVConnection *) ((VIO *) data)->vc_server;
+    net_vc = static_cast<NetVConnection *>((static_cast<VIO *>(data))->vc_server);
     break;
 
   default:
@@ -86,16 +154,12 @@ SessionBucket::session_handler(int event, void *data)
     return 0;
   }
 
-  // Search the 2nd level bucket for appropriate netvc
-  int l2_index = SECOND_LEVEL_HASH(net_vc->get_remote_addr());
+  sockaddr const* addr = net_vc->get_remote_addr();
   HttpConfigParams *http_config_params = HttpConfig::acquire();
   bool found = false;
 
-  ink_assert(l2_index < HSM_LEVEL2_BUCKETS);
-  s = l2_hash[l2_index].head;
-
-  while (s != NULL) {
-    if (s->get_netvc() == net_vc) {
+  for ( ServerSessionPool::IPHashTable::Location lh = m_ip_pool.find(addr) ; lh.isValid() ; ++lh ) {
+    if ((s = lh)->get_netvc() == net_vc) {
       // if there was a timeout of some kind on a keep alive connection, and
       // keeping the connection alive will not keep us above the # of max connections
       // to the origin and we are below the min number of keep alive connections to this
@@ -118,27 +182,26 @@ SessionBucket::session_handler(int event, void *data)
 
       // We've found our server session. Remove it from
       //   our lists and close it down
-      Debug("http_ss", "[%" PRId64 "] [session_bucket] session received io notice [%s]",
-            s->con_id, HttpDebugNames::get_event_name(event));
+      Debug("http_ss", "[%" PRId64 "] [session_pool] session %p received io notice [%s]",
+            s->con_id, s, HttpDebugNames::get_event_name(event));
       ink_assert(s->state == HSS_KA_SHARED);
-      lru_list.remove(s);
-      l2_hash[l2_index].remove(s);
+      // Out of the pool! Now!
+      m_ip_pool.remove(lh);
+      m_host_pool.remove(m_host_pool.find(s));
+      // Drop connection on this end.
       s->do_io_close();
       found = true;
       break;
-    } else {
-      s = s->hash_link.next;
     }
   }
 
   HttpConfig::release(http_config_params);
-  if (found)
-    return 0;
-
-  // We failed to find our session.  This can only be the result
-  //  of a programming flaw
-  Warning("Connection leak from http keep-alive system");
-  ink_assert(0);
+  if (!found) {
+    // We failed to find our session.  This can only be the result
+    //  of a programming flaw
+    Warning("Connection leak from http keep-alive system");
+    ink_assert(0);
+  }
   return 0;
 }
 
@@ -146,81 +209,20 @@ SessionBucket::session_handler(int event, void *data)
 void
 HttpSessionManager::init()
 {
-  // Initialize our internal (global) hash table
-  for (int i = 0; i < HSM_LEVEL1_BUCKETS; i++) {
-    g_l1_hash[i].mutex = new_ProxyMutex();
-  }
+  m_g_pool = new ServerSessionPool;
 }
 
 // TODO: Should this really purge all keep-alive sessions?
+// Does this make any sense, since we always do the global pool and not the per thread?
 void
 HttpSessionManager::purge_keepalives()
 {
   EThread *ethread = this_ethread();
 
-  for (int i = 0; i < HSM_LEVEL1_BUCKETS; i++) {
-    SessionBucket *b = &g_l1_hash[i];
-    MUTEX_TRY_LOCK(lock, b->mutex, ethread);
-    if (lock) {
-      while (b->lru_list.head) {
-        HttpServerSession *sess = b->lru_list.head;
-        b->lru_list.remove(sess);
-        int l2_index = SECOND_LEVEL_HASH(&sess->server_ip.sa);
-        b->l2_hash[l2_index].remove(sess);
-        sess->do_io_close();
-      }
-    } else {
-      // Fix me, should retry
-    }
-  }
-}
-
-bool
-HttpSessionManager::match(HttpServerSession* s, sockaddr const* addr, INK_MD5 const& hostname_hash, HttpSM* sm)
-{
-  return
-    (TS_SERVER_SESSION_SHARING_MATCH_HOST == sm->t_state.txn_conf->server_session_sharing_match || 
-          (ats_ip_addr_eq(&s->server_ip.sa, addr) && ats_ip_port_cast(&s->server_ip.sa) == ats_ip_port_cast(addr)))
-    && (TS_SERVER_SESSION_SHARING_MATCH_IP == sm->t_state.txn_conf->server_session_sharing_match ||
-        s->hostname_hash == hostname_hash)
-    ;
-}
-
-bool
-HttpSessionManager::match(HttpServerSession* s, sockaddr const* addr, char const* hostname, HttpSM* sm)
-{
-  INK_MD5 hostname_hash;
-  ink_code_md5((unsigned char *) hostname, strlen(hostname), reinterpret_cast<unsigned char *>(&hostname_hash));
-  return match(s, addr, hostname_hash, sm);
-}
-
-HSMresult_t
-_acquire_session(SessionBucket *bucket, sockaddr const* ip, INK_MD5 &hostname_hash, HttpSM *sm)
-{
-  HttpServerSession *b;
-  HttpServerSession *to_return = NULL;
-  int l2_index = SECOND_LEVEL_HASH(ip);
-
-  ink_assert(l2_index < HSM_LEVEL2_BUCKETS);
-
-  // Check to see if an appropriate connection is in
-  //  the 2nd level bucket
-  b = bucket->l2_hash[l2_index].head;
-  while (b != NULL) {
-    if (HttpSessionManager::match(b, ip, hostname_hash, sm)) {
-      bucket->lru_list.remove(b);
-      bucket->l2_hash[l2_index].remove(b);
-      b->state = HSS_ACTIVE;
-      to_return = b;
-      Debug("http_ss", "[%" PRId64 "] [acquire session] " "return session from shared pool", to_return->con_id);
-      sm->attach_server_session(to_return);
-      return HSM_DONE;
-    }
-
-    b = b->hash_link.next;
-  }
-
-  return HSM_NOT_FOUND;
+  MUTEX_TRY_LOCK(lock, m_g_pool->mutex, ethread);
+  if (lock) {
+    m_g_pool->purge();
+  } // should we do something clever if we don't get the lock?
 }
 
 HSMresult_t
@@ -228,17 +230,7 @@ HttpSessionManager::acquire_session(Continuation * /* cont ATS_UNUSED */, sockad
                                     const char *hostname, HttpClientSession *ua_session, HttpSM *sm)
 {
   HttpServerSession *to_return = NULL;
-
-  //  We compute the hash for matching the hostname as the last
-  //  check for a match between the session the HttpSM is looking
-  //  for and the sessions we have. We have to use the hostname
-  //  as part of the match because some stupid servers can't
-  //  handle getting request for different virtual hosts over
-  //  the same keep-alive session (INKqa05429).
-  // 
-  //  Also, note the ip is required as well to maintain client
-  //  to server affinity so that we don't break certain types
-  //  of authentication.
+  TSServerSessionSharingMatchType match_style = static_cast<TSServerSessionSharingMatchType>(sm->t_state.txn_conf->server_session_sharing_match);
   INK_MD5 hostname_hash;
 
   ink_code_md5((unsigned char *) hostname, strlen(hostname), (unsigned char *) &hostname_hash);
@@ -249,7 +241,7 @@ HttpSessionManager::acquire_session(Continuation * /* cont ATS_UNUSED */, sockad
   if (to_return != NULL) {
     ua_session->attach_server_session(NULL);
 
-    if (match(to_return, ip, hostname_hash, sm)) {
+    if (ServerSessionPool::match(to_return, ip, hostname_hash, match_style)) {
       Debug("http_ss", "[%" PRId64 "] [acquire session] returning attached session ", to_return->con_id);
       to_return->state = HSS_ACTIVE;
       sm->attach_server_session(to_return);
@@ -262,74 +254,46 @@ HttpSessionManager::acquire_session(Continuation * /* cont ATS_UNUSED */, sockad
     to_return = NULL;
   }
 
-  // Now check to see if we have a connection is our
-  //  shared connection pool
-  int l1_index = FIRST_LEVEL_HASH(ip);
+  // Now check to see if we have a connection in our shared connection pool
   EThread *ethread = this_ethread();
 
-  ink_assert(l1_index < HSM_LEVEL1_BUCKETS);
-
   if (TS_SERVER_SESSION_SHARING_POOL_THREAD == sm->t_state.txn_conf->server_session_sharing_pool) {
-    ink_assert(ethread->l1_hash);
-    return _acquire_session(ethread->l1_hash + l1_index, ip, hostname_hash, sm);
+    to_return = ethread->server_session_pool->acquireSession(ip, hostname_hash, match_style);
   } else {
-    SessionBucket *bucket = g_l1_hash + l1_index;
-
-    MUTEX_TRY_LOCK(lock, bucket->mutex, ethread);
+    MUTEX_TRY_LOCK(lock, m_g_pool->mutex, ethread);
     if (lock) {
-      return _acquire_session(bucket, ip, hostname_hash, sm);
+      to_return = m_g_pool->acquireSession(ip, hostname_hash, match_style);
+      Debug("http_ss", "[acquire session] pool search %s", to_return ? "successful" : "failed");
     } else {
       Debug("http_ss", "[acquire session] could not acquire session due to lock contention");
+      return HSM_RETRY;
     }
   }
 
-  return HSM_RETRY;
+  if (to_return) {
+    Debug("http_ss", "[%" PRId64 "] [acquire session] " "return session from shared pool", to_return->con_id);
+    to_return->state = HSS_ACTIVE;
+    sm->attach_server_session(to_return);
+    return HSM_DONE;
+  }
+  return HSM_NOT_FOUND;
 }
 
 HSMresult_t
 HttpSessionManager::release_session(HttpServerSession *to_release)
 {
   EThread *ethread = this_ethread();
-  int l1_index = FIRST_LEVEL_HASH(&to_release->server_ip.sa);
-  SessionBucket *bucket;
-
-  ink_assert(l1_index < HSM_LEVEL1_BUCKETS);
-
-  if (TS_SERVER_SESSION_SHARING_POOL_THREAD == to_release->sharing_pool) {
-    bucket = ethread->l1_hash + l1_index;
-  } else {
-    bucket = g_l1_hash + l1_index;
-  }
-
-  MUTEX_TRY_LOCK(lock, bucket->mutex, ethread);
+  ServerSessionPool* pool = TS_SERVER_SESSION_SHARING_POOL_THREAD == to_release->sharing_pool ? ethread->server_session_pool : m_g_pool;
+  bool released_p = true;
+  
+  // The per thread lock looks like it should not be needed but if it's not locked the close checking I/O op will crash.
+  MUTEX_TRY_LOCK(lock, pool->mutex, ethread);
   if (lock) {
-    int l2_index = SECOND_LEVEL_HASH(&to_release->server_ip.sa);
-
-    ink_assert(l2_index < HSM_LEVEL2_BUCKETS);
-
-    // First insert the session on to our lists
-    bucket->lru_list.enqueue(to_release);
-    bucket->l2_hash[l2_index].push(to_release);
-    to_release->state = HSS_KA_SHARED;
-
-    // Now we need to issue a read on the connection to detect
-    //  if it closes on us.  We will get called back in the
-    //  continuation for this bucket, ensuring we have the lock
-    //  to remove the connection from our lists
-    to_release->do_io_read(bucket, INT64_MAX, to_release->read_buffer);
-
-    // Transfer control of the write side as well
-    to_release->do_io_write(bucket, 0, NULL);
-
-    // we probably don't need the active timeout set, but will leave it for now
-    to_release->get_netvc()->set_inactivity_timeout(to_release->get_netvc()->get_inactivity_timeout());
-    to_release->get_netvc()->set_active_timeout(to_release->get_netvc()->get_active_timeout());
-    Debug("http_ss", "[%" PRId64 "] [release session] " "session placed into shared pool", to_release->con_id);
-
-    return HSM_DONE;
+    pool->releaseSession(to_release);
   } else {
     Debug("http_ss", "[%" PRId64 "] [release session] could not release session due to lock contention", to_release->con_id);
+    released_p = false;
   }
 
-  return HSM_RETRY;
+  return released_p ? HSM_DONE : HSM_RETRY;
 }

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0c358a4b/proxy/http/HttpSessionManager.h
----------------------------------------------------------------------
diff --git a/proxy/http/HttpSessionManager.h b/proxy/http/HttpSessionManager.h
index f6086eb..442a5c1 100644
--- a/proxy/http/HttpSessionManager.h
+++ b/proxy/http/HttpSessionManager.h
@@ -36,6 +36,7 @@
 
 #include "P_EventSystem.h"
 #include "HttpServerSession.h"
+#include <ts/Map.h>
 
 class HttpClientSession;
 class HttpSM;
@@ -43,16 +44,77 @@ class HttpSM;
 void
 initialize_thread_for_http_sessions(EThread *thread, int thread_index);
 
-#define  HSM_LEVEL1_BUCKETS   127
-#define  HSM_LEVEL2_BUCKETS   63
+/** A pool of server sessions.
 
-class SessionBucket: public Continuation
+    This is a continuation so that it can get callbacks from the server sessions.
+    This is used to track remote closes on the sessions so they can be cleaned up.
+
+    @internal Cleanup is the real reason we will always need an IP address mapping for the
+    sessions. The I/O callback will have only the NetVC and thence the remote IP address for the
+    closed session and we need to be able find it based on that.
+*/
+class ServerSessionPool: public Continuation
 {
 public:
-  SessionBucket();
-  int session_handler(int event, void *data);
-  Que(HttpServerSession, lru_link) lru_list;
-  DList(HttpServerSession, hash_link) l2_hash[HSM_LEVEL2_BUCKETS];
+  /// Default constructor.
+  /// Constructs an empty pool.
+  ServerSessionPool();
+  /// Handle events from server sessions.
+  int eventHandler(int event, void *data);
+ protected:
+  /// Interface class for IP map.
+  struct IPHashing
+  {
+    typedef uint32_t ID;
+    typedef sockaddr const* Key;
+    typedef HttpServerSession Value;
+    typedef DList(HttpServerSession, ip_hash_link) ListHead;
+
+    static ID hash(Key key) { return ats_ip_hash(key); }
+    static Key key(Value const* value) { return &value->server_ip.sa; }
+    static bool equal(Key lhs, Key rhs) { return ats_ip_addr_port_eq(lhs, rhs); }
+  };
+
+  /// Interface class for FQDN map.
+  struct HostHashing
+  {
+    typedef uint64_t ID;
+    typedef INK_MD5 const& Key;
+    typedef HttpServerSession Value;
+    typedef DList(HttpServerSession, host_hash_link) ListHead;
+
+    static ID hash(Key key) { return key.fold(); }
+    static Key key(Value const* value) { return value->hostname_hash; } 
+    static bool equal(Key lhs, Key rhs) { return lhs == rhs; }
+  };
+
+  typedef TSHashTable<IPHashing> IPHashTable; ///< Sessions by IP address.
+  typedef TSHashTable<HostHashing> HostHashTable; ///< Sessions by host name.
+
+public:
+  /** Check if a session matches address and host name.
+   */
+  static bool match(HttpServerSession* ss, sockaddr const* addr, INK_MD5 const& host_hash, TSServerSessionSharingMatchType match_style);
+
+  /** Get a session from the pool.
+
+      The session is selected based on @a match_style equivalently to @a match. If found the session
+      is removed from the pool.
+
+      @return A pointer to the session or @c NULL if not matching session was found.
+  */
+  HttpServerSession* acquireSession(sockaddr const* addr, INK_MD5 const& host_hash, TSServerSessionSharingMatchType match_style);
+  /** Release a session to to pool.
+   */
+  void releaseSession(HttpServerSession* ss);
+
+  /// Close all sessions and then clear the table.
+  void purge();
+
+  // Pools of server sessions.
+  // Note that each server session is stored in both pools.
+  IPHashTable m_ip_pool;
+  HostHashTable m_host_pool;
 };
 
 enum HSMresult_t
@@ -65,7 +127,7 @@ enum HSMresult_t
 class HttpSessionManager
 {
 public:
-  HttpSessionManager()
+  HttpSessionManager() : m_g_pool(NULL)
   { }
 
   ~HttpSessionManager()
@@ -78,26 +140,10 @@ public:
   void init();
   int main_handler(int event, void *data);
 
-  /// Check if a session is a valid match.
-  static bool match(
-		    HttpServerSession* s, ///< Session to check for match.
-		    sockaddr const* addr, ///< IP address.
-		    INK_MD5 const& hostname_hash, ///< Hash of hostname of origin server.
-		    HttpSM* sm ///< State machine (for configuration data).
-		    );
-
-  /// Check if a session is a valid match.
-  static bool match(
-		    HttpServerSession* s, ///< Session to check for match.
-		    sockaddr const* addr, ///< IP address.
-		    char const* hostname, ///< Hostname of origin server.
-		    HttpSM* sm ///< State machine (for configuration data).
-		    );
-
-
 private:
-  //    Global l1 hash, used when there is no per-thread buckets
-  SessionBucket g_l1_hash[HSM_LEVEL1_BUCKETS];
+  /// Global pool, used if not per thread pools.
+  /// @internal We delay creating this because the session manager is created during global statics init.
+  ServerSessionPool* m_g_pool;
 };
 
 extern HttpSessionManager httpSessionManager;


Re: git commit: TS-2863: Enable FQDN selection for shared sessions.

Posted by "Alan M. Carroll" <am...@network-geographics.com>.
I would like to note that another purpose of the FQDN change is to provide better encapsulation for server session management with an eye to future work in making that more accessible to plugins and other core code.


Re: git commit: TS-2863: Enable FQDN selection for shared sessions.

Posted by "Alan M. Carroll" <am...@network-geographics.com>.
James,

Friday, July 25, 2014, 3:59:18 PM, you wrote:

> Still reviewing, but I wish this had been a number of smaller patches. For example, const'ing the MD5 stuff and adding ~PluginIdentity() could have been independent patches ...

Yes, the ~PluginIdentity() is a leak from some other work I missed before I committed. I needed the const on the MD5 stuff, though, to make thing compile.

But, just for fun, I'm about to add another big change that messes around with the MD5 setup anyway.


Re: git commit: TS-2863: Enable FQDN selection for shared sessions.

Posted by James Peach <jp...@apache.org>.
Still reviewing, but I wish this had been a number of smaller patches. For example, const'ing the MD5 stuff and adding ~PluginIdentity() could have been independent patches ...

On Jul 24, 2014, at 7:20 PM, amc@apache.org wrote:

> Repository: trafficserver
> Updated Branches:
>  refs/heads/master be428d6f4 -> 0c358a4b1
> 
> 
> TS-2863: Enable FQDN selection for shared sessions.

[snip]

> +bool
> +ServerSessionPool::match(HttpServerSession* ss, sockaddr const* addr, INK_MD5 const& hostname_hash, TSServerSessionSharingMatchType match_style)
> +{
> +  return TS_SERVER_SESSION_SHARING_MATCH_NONE != match_style && // if no matching allowed, fail immediately.
> +    // The hostname matches if we're not checking it or it (and the port!) is a match.
> +    (TS_SERVER_SESSION_SHARING_MATCH_IP == match_style ||  (ats_ip_port_cast(addr) == ats_ip_port_cast(ss->server_ip) && ss->hostname_hash == hostname_hash)) &&
> +    // The IP address matches if we're not checking it or it is a match.
> +    (TS_SERVER_SESSION_SHARING_MATCH_HOST == match_style || ats_ip_addr_port_eq(ss->server_ip, addr))
> +    ;
> +}

The comments help, but IMHO this would still be easier to read in a more declarative style:

switch (match_style) {
case TS_SERVER_SESSION_SHARING_MATCH_IP:
	return ats_ip_port_cast(addr) == ats_ip_port_cast(ss->server_ip) && ss->hostname_hash == hostname_hash;
case TS_SERVER_SESSION_SHARING_MATCH_HOST:
	return ats_ip_addr_port_eq(ss->server_ip, addr);
default:
	ink_assert(match_style == TS_SERVER_SESSION_SHARING_MATCH_NONE);
	return false;
}



Re: git commit: TS-2863: Enable FQDN selection for shared sessions.

Posted by James Peach <jp...@apache.org>.
Still reviewing, but I wish this had been a number of smaller patches. For example, const'ing the MD5 stuff and adding ~PluginIdentity() could have been independent patches ...

On Jul 24, 2014, at 7:20 PM, amc@apache.org wrote:

> Repository: trafficserver
> Updated Branches:
>  refs/heads/master be428d6f4 -> 0c358a4b1
> 
> 
> TS-2863: Enable FQDN selection for shared sessions.

[snip]

> +bool
> +ServerSessionPool::match(HttpServerSession* ss, sockaddr const* addr, INK_MD5 const& hostname_hash, TSServerSessionSharingMatchType match_style)
> +{
> +  return TS_SERVER_SESSION_SHARING_MATCH_NONE != match_style && // if no matching allowed, fail immediately.
> +    // The hostname matches if we're not checking it or it (and the port!) is a match.
> +    (TS_SERVER_SESSION_SHARING_MATCH_IP == match_style ||  (ats_ip_port_cast(addr) == ats_ip_port_cast(ss->server_ip) && ss->hostname_hash == hostname_hash)) &&
> +    // The IP address matches if we're not checking it or it is a match.
> +    (TS_SERVER_SESSION_SHARING_MATCH_HOST == match_style || ats_ip_addr_port_eq(ss->server_ip, addr))
> +    ;
> +}

The comments help, but IMHO this would still be easier to read in a more declarative style:

switch (match_style) {
case TS_SERVER_SESSION_SHARING_MATCH_IP:
	return ats_ip_port_cast(addr) == ats_ip_port_cast(ss->server_ip) && ss->hostname_hash == hostname_hash;
case TS_SERVER_SESSION_SHARING_MATCH_HOST:
	return ats_ip_addr_port_eq(ss->server_ip, addr);
default:
	ink_assert(match_style == TS_SERVER_SESSION_SHARING_MATCH_NONE);
	return false;
}