Optimize: If failed on migrateToCurrentThread, put the server session back to global server session pool. by oknet · Pull Request #2467 · apache/trafficserver

oknet · 2017-09-01T14:25:20Z

To be rebased after PR #2440 merged.

If global server session pool is enabled,

one server session is acquired
then try to migrate it to current EThread
if the migration failed, the server session will be closed.

The server session should be send back rather than do_io_close it.

The PR put the server session back to the global server session pool.

SolidWallOfCode · 2017-09-01T20:27:21Z

I'm not sure this is worth it. It's a lot of code to try to save a single outbound connection. Might it not be better to validate the VC can be moved first, rather than move it and then move it back?

oknet · 2017-09-02T04:40:49Z

@SolidWallOfCode

This PR try to move the netvc first and then migrate it if it is not moved for now.

The move has better performance than migrate.

zwoop · 2017-09-06T16:03:50Z

Just as the CI, I can't get this to build either :-).

gmake[2]: Entering directory '/usr/local/src/trafficserver/iocore/net'
  CXX      UnixNetVConnection.o
In file included from /usr/local/src/trafficserver/iocore/eventsystem/I_Continuation.h:41:0,
                 from /usr/local/src/trafficserver/iocore/eventsystem/I_Action.h:30,
                 from /usr/local/src/trafficserver/iocore/eventsystem/I_EventSystem.h:31,
                 from /usr/local/src/trafficserver/iocore/eventsystem/P_EventSystem.h:36,
                 from P_Net.h:94,
                 from UnixNetVConnection.cc:24:
UnixNetVConnection.cc: In member function ‘UnixNetVConnection* UnixNetVConnection::migrateToCurrentThread(Continuation*, EThread*)’:
/usr/local/src/trafficserver/iocore/eventsystem/I_Lock.h:72:97: error: no matching function for call to ‘MutexTryLock::MutexTryLock(SourceLocation, char*, NetHandler*&, EThread*&)’
 #define MUTEX_TRY_LOCK(_l, _m, _t) MutexTryLock _l(MakeSourceLocation(), (char *)nullptr, _m, _t)
                                                                                                 ^
UnixNetVConnection.cc:1447:3: note: in expansion of macro ‘MUTEX_TRY_LOCK’
   MUTEX_TRY_LOCK(lock_src, this->nh, t);
   ^~~~~~~~~~~~~~
In file included from /usr/local/src/trafficserver/iocore/eventsystem/I_Continuation.h:41:0,
                 from /usr/local/src/trafficserver/iocore/eventsystem/I_Action.h:30,
                 from /usr/local/src/trafficserver/iocore/eventsystem/I_EventSystem.h:31,
                 from /usr/local/src/trafficserver/iocore/eventsystem/P_EventSystem.h:36,
                 from P_Net.h:94,
                 from UnixNetVConnection.cc:24:
/usr/local/src/trafficserver/iocore/eventsystem/I_Lock.h:576:3: note: candidate: MutexTryLock::MutexTryLock(const SourceLocation&, const char*, Ptr<ProxyMutex>&, EThread*, int)
   MutexTryLock(
   ^~~~~~~~~~~~
/usr/local/src/trafficserver/iocore/eventsystem/I_Lock.h:576:3: note:   candidate expects 5 arguments, 4 provided
/usr/local/src/trafficserver/iocore/eventsystem/I_Lock.h:562:3: note: candidate: MutexTryLock::MutexTryLock(const SourceLocation&, const char*, ProxyMutex*, EThread*, int)
   MutexTryLock(
   ^~~~~~~~~~~~
/usr/local/src/trafficserver/iocore/eventsystem/I_Lock.h:562:3: note:   candidate expects 5 arguments, 4 provided
/usr/local/src/trafficserver/iocore/eventsystem/I_Lock.h:548:3: note: candidate: MutexTryLock::MutexTryLock(const SourceLocation&, const char*, Ptr<ProxyMutex>&, EThread*)
   MutexTryLock(
   ^~~~~~~~~~~~
/usr/local/src/trafficserver/iocore/eventsystem/I_Lock.h:548:3: note:   no known conversion for argument 3 from ‘NetHandler*’ to ‘Ptr<ProxyMutex>&’
/usr/local/src/trafficserver/iocore/eventsystem/I_Lock.h:534:3: note: candidate: MutexTryLock::MutexTryLock(const SourceLocation&, const char*, ProxyMutex*, EThread*)
   MutexTryLock(
   ^~~~~~~~~~~~
/usr/local/src/trafficserver/iocore/eventsystem/I_Lock.h:534:3: note:   no known conversion for argument 3 from ‘NetHandler*’ to ‘ProxyMutex*’
/usr/local/src/trafficserver/iocore/eventsystem/I_Lock.h:527:7: note: candidate: MutexTryLock::MutexTryLock(const MutexTryLock&)
 class MutexTryLock
       ^~~~~~~~~~~~
/usr/local/src/trafficserver/iocore/eventsystem/I_Lock.h:527:7: note:   candidate expects 1 argument, 4 provided
UnixNetVConnection.cc:1452:15: error: ‘class NetHandler’ has no member named ‘stopIO’
     this->nh->stopIO(this);
               ^~~~~~
UnixNetVConnection.cc:1454:5: error: ‘vc’ was not declared in this scope
     vc->thread = t;
     ^~
UnixNetVConnection.cc:1455:16: error: ‘class NetHandler’ has no member named ‘startIO’
     client_nh->startIO(this);
                ^~~~~~~
At global scope:
cc1plus: warning: unrecognized command line option ‘-Wno-format-truncation’
Makefile:1000: recipe for target 'UnixNetVConnection.o' failed
gmake[2]: *** [UnixNetVConnection.o] Error 1
gmake[2]: Leaving directory '/usr/local/src/trafficserver/iocore/net'
Makefile:596: recipe for target 'all-recursive' failed
gmake[1]: *** [all-recursive] Error 1
gmake[1]: Leaving directory '/usr/local/src/trafficserver/iocore'
Makefile:829: recipe for target 'all-recursive' failed
gmake: *** [all-recursive] Error 1

oknet · 2017-09-07T03:32:22Z

@zwoop This will be rebased after #2440 merged.

shinrich

Overall looks good to me. Once the dependent PR gets merged in.

shinrich · 2017-09-12T15:49:50Z


  if (h->startIO(this) < 0) {
+    con_in.move(this->con);
    Debug("iocore_net", "populate : Failed to add to epoll list");


I assume this con_in.move was added to ensure the socket get's cleaned up?

If startIO failed, move the con.fd back to con_in.fd. To avoid close the con_in.fd.

shinrich · 2017-09-12T15:50:44Z

+
+  // Try to get the mutex lock for NetHandler of this NetVC
+  MUTEX_TRY_LOCK(lock_src, this->nh->mutex, t);
+  if (lock_src.is_locked()) {


I like the move logic. If you can in fact get both locks, this is more efficient.

shinrich · 2017-09-12T15:51:59Z

-              }
+            if (new_vc->thread != ethread) {
+              // Failed to migrate, put it back to global session pool
+              m_g_pool->releaseSession(to_return);


I don't seeing doing the release buying us much, but it doesn't cost us much either. If the releaseSession doesn't get the lock, it will close the netvc anyway.

The m_g_pool is locked at L312.

311 EThread *ethread = this_ethread(); 312 ProxyMutex *pool_mutex = (TS_SERVER_SESSION_SHARING_POOL_THREAD == sm->t_state.http_config_param->server_session_sharing_poo l) ? 313 ethread->server_session_pool->mutex.get() : 314 m_g_pool->mutex.get(); 315 MUTEX_TRY_LOCK(lock, pool_mutex, ethread); 316 if (lock.is_locked()) {

The to_return will be released to the pool.

scw00

+1 looks good to me !

oknet · 2017-09-15T02:23:09Z

I will rebase this by PR #2501 later.

scw00

+1

oknet · 2017-09-15T13:57:53Z

+    // Since we moved the con context, the fd will not be closed
+    // Go ahead and remove the fd from the original thread's epoll structure, so it is not
+    // processed on two threads simultaneously
+    this->ep.stop();


@shinrich @SolidWallOfCode Can we remove this?

The read.enabled and write.enabled set to 0 by do_io_close() that means the netvc is disabled.

It is not necessary to call ep.stop() here.

Sounds reasonable.

oknet · 2017-09-19T08:24:54Z

ping @shinrich @scw00

… back to global server session pool

shinrich

Looks good to me.

oknet added the HTTP label Sep 1, 2017

oknet added this to the 8.0.0 milestone Sep 1, 2017

oknet self-assigned this Sep 1, 2017

zwoop requested a review from shinrich September 1, 2017 19:04

SolidWallOfCode mentioned this pull request Sep 1, 2017

Optimize: Add startIO & stopIO for NetHandler #2440

Merged

oknet force-pushed the i2467 branch 3 times, most recently from 3ad8dfd to 2d6142f Compare September 9, 2017 05:31

shinrich previously approved these changes Sep 12, 2017

View reviewed changes

scw00 previously approved these changes Sep 14, 2017

View reviewed changes

oknet dismissed stale reviews from scw00 and shinrich via 1c3ae83 September 15, 2017 02:27

oknet force-pushed the i2467 branch from 2d6142f to 1c3ae83 Compare September 15, 2017 02:27

scw00 previously approved these changes Sep 15, 2017

View reviewed changes

oknet mentioned this pull request Sep 15, 2017

Optimize: rename close_UnixNetVConnection to NetHandler::free_netvc. #2503

Merged

oknet dismissed scw00’s stale review via 01cc731 September 15, 2017 12:38

oknet force-pushed the i2467 branch from 1c3ae83 to 01cc731 Compare September 15, 2017 12:38

oknet commented Sep 15, 2017

View reviewed changes

Optimize: If failed on migrateToCurrentThread, put the server session…

8159fe0

… back to global server session pool

oknet force-pushed the i2467 branch from 01cc731 to 8159fe0 Compare September 20, 2017 10:26

shinrich approved these changes Oct 12, 2017

View reviewed changes

oknet merged commit c776c66 into apache:master Oct 13, 2017

Conversation

oknet commented Sep 1, 2017

Uh oh!

SolidWallOfCode commented Sep 1, 2017

Uh oh!

oknet commented Sep 2, 2017

Uh oh!

zwoop commented Sep 6, 2017

Uh oh!

oknet commented Sep 7, 2017

Uh oh!

shinrich left a comment

Choose a reason for hiding this comment

Uh oh!

shinrich Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

oknet Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shinrich Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

shinrich Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

oknet Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

scw00 left a comment

Choose a reason for hiding this comment

Uh oh!

oknet commented Sep 15, 2017

Uh oh!

scw00 left a comment

Choose a reason for hiding this comment

Uh oh!

oknet Sep 15, 2017

Choose a reason for hiding this comment

Uh oh!

shinrich Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

oknet commented Sep 19, 2017

Uh oh!

shinrich left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

oknet Sep 13, 2017 •

edited

Loading