Skip to content

Optimize: If failed on migrateToCurrentThread, put the server session back to global server session pool. #2467

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
oknet merged 1 commit into from
Oct 13, 2017
Merged

Optimize: If failed on migrateToCurrentThread, put the server session back to global server session pool. #2467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 51 additions & 23 deletions iocore/net/UnixNetVConnection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1244,6 +1244,7 @@ UnixNetVConnection::populate(Connection &con_in, Continuation *c, void *arg)
}

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

@shinrich shinrich Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@oknet oknet Sep 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return EVENT_ERROR;
}
Expand Down Expand Up @@ -1430,44 +1431,71 @@ UnixNetVConnection::migrateToCurrentThread(Continuation *cont, EThread *t)
return this;
}

Connection hold_con;
hold_con.move(this->con);
SSLNetVConnection *sslvc = dynamic_cast<SSLNetVConnection *>(this);

SSL *save_ssl = (sslvc) ? sslvc->ssl : nullptr;
if (save_ssl) {
SSLNetVCDetach(sslvc->ssl);
sslvc->ssl = nullptr;
// Lock the NetHandler first in order to put the new NetVC into NetHandler and InactivityCop.
// It is safe and no performance issue to get the mutex lock for a NetHandler of current ethread.
SCOPED_MUTEX_LOCK(lock, client_nh->mutex, t);

// 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()) {
Copy link
Copy Markdown
Member

@shinrich shinrich Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Deattach this NetVC from original NetHandler & InactivityCop.
this->nh->stopCop(this);
this->nh->stopIO(this);
// Put this NetVC into current NetHandler & InactivityCop.
this->thread = t;
client_nh->startIO(this);
client_nh->startCop(this);
// Move this NetVC to current EThread Successfully.
return this;
}

// Do_io_close will signal the VC to be freed on the original thread
// 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();
this->do_io_close();
// Failed to get the mutex lock for original NetHandler.
// Try to migrate it by create a new NetVC and then move con.fd and ssl ctx.
SSLNetVConnection *sslvc = dynamic_cast<SSLNetVConnection *>(this);
SSL *save_ssl = (sslvc) ? sslvc->ssl : nullptr;

UnixNetVConnection *ret_vc = nullptr;

// Create new VC:
if (save_ssl) {
SSLNetVConnection *sslvc = static_cast<SSLNetVConnection *>(sslNetProcessor.allocate_vc(t));
if (sslvc->populate(hold_con, cont, save_ssl) != EVENT_DONE) {
sslvc->do_io_close();
sslvc = nullptr;
if (sslvc->populate(this->con, cont, save_ssl) != EVENT_DONE) {
sslvc->free(t);
sslvc = nullptr;
ret_vc = this;
} else {
sslvc->set_context(get_context());
ret_vc = dynamic_cast<UnixNetVConnection *>(sslvc);
}
return sslvc;
// Update the SSL fields
} else {
UnixNetVConnection *netvc = static_cast<UnixNetVConnection *>(netProcessor.allocate_vc(t));
if (netvc->populate(hold_con, cont, save_ssl) != EVENT_DONE) {
netvc->do_io_close();
netvc = nullptr;
if (netvc->populate(this->con, cont, save_ssl) != EVENT_DONE) {
netvc->free(t);
netvc = nullptr;
ret_vc = this;
} else {
netvc->set_context(get_context());
ret_vc = netvc;
}
return netvc;
}

// clear con.fd and ssl ctx from this NetVC since a new NetVC is created.
if (ret_vc != this) {
if (save_ssl) {
SSLNetVCDetach(sslvc->ssl);
sslvc->ssl = nullptr;
}
ink_assert(this->con.fd == NO_FD);

// Do_io_close will signal the VC to be freed on the original thread
// 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();
Copy link
Copy Markdown
Member Author

@oknet oknet Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@shinrich shinrich Oct 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable.

this->do_io_close();
}

return ret_vc;
}

void
Expand Down
24 changes: 10 additions & 14 deletions proxy/http/HttpSessionManager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,21 +326,17 @@ HttpSessionManager::acquire_session(Continuation * /* cont ATS_UNUSED */, sockad
UnixNetVConnection *server_vc = dynamic_cast<UnixNetVConnection *>(to_return->get_netvc());
if (server_vc) {
UnixNetVConnection *new_vc = server_vc->migrateToCurrentThread(sm, ethread);
// The VC moved, free up the original one
if (new_vc != server_vc) {
ink_assert(new_vc == nullptr || new_vc->nh != nullptr);
if (!new_vc) {
// Close out to_return, we were't able to get a connection
to_return->do_io_close();
to_return = nullptr;
retval = HSM_NOT_FOUND;
} else {
// Keep things from timing out on us
new_vc->set_inactivity_timeout(new_vc->get_inactivity_timeout());
to_return->set_netvc(new_vc);
}
if (new_vc->thread != ethread) {
// Failed to migrate, put it back to global session pool
m_g_pool->releaseSession(to_return);
Copy link
Copy Markdown
Member

@shinrich shinrich Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@oknet oknet Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

to_return = nullptr;
retval = HSM_NOT_FOUND;
} else if (new_vc != server_vc) {
// The VC migrated, keep things from timing out on us
new_vc->set_inactivity_timeout(new_vc->get_inactivity_timeout());
to_return->set_netvc(new_vc);
} else {
// Keep things from timing out on us
// The VC moved, keep things from timing out on us
server_vc->set_inactivity_timeout(server_vc->get_inactivity_timeout());
}
}
Expand Down