bneradt commented on code in PR #13806:
URL: https://github.com/apache/trafficserver/pull/13806#discussion_r4288535455
##########
src/iocore/net/UnixNetVConnection.cc:
##########
@@ -1311,43 +1311,63 @@ UnixNetVConnection::is_default_inactivity_timeout()
/*
* Close down the current netVC. Save aside the socket and SSL information
- * and create new netVC in the current thread/netVC
+ * so a new netVC can be created from them in another thread.
*/
-UnixNetVConnection *
-UnixNetVConnection::migrateToCurrentThread(Continuation *cont, EThread *t)
+bool
+UnixNetVConnection::detachForMigration(MigrationState &ms)
{
- NetHandler *client_nh = get_NetHandler(t);
- ink_assert(client_nh);
- if (this->nh == client_nh) {
- // We're already there!
- return this;
+ // Remove the fd from the original thread's poller first, so it is not
processed on two threads
+ // simultaneously. While this VC still owns the socket and SSL information a
failure only needs
+ // an ordinary close.
+ //
+ // This stop is not synchronized with the original thread. That thread may
have already seen
+ // this VC as open and enabled in NetHandler::process_ready_list, in which
case it goes on to
+ // net_read_io, fails the try lock on the VIO mutex held by the caller, and
reschedules the VC.
+ // The reschedule calls ep.refresh on the EventIO stopped here, so
EventIO::modify and
+ // EventIO::refresh do nothing on a stopped EventIO. A delay before the try
lock in net_read_io
+ // makes the original thread hit this reliably.
+ //
+ // Unlike an ordinary close, migration keeps the fd open, so kqueue
registrations must
+ // also be removed here before the old VC can be freed.
+ if (this->ep.stop_for_migration() != 0) {
+ Error("Failed to remove migrating socket %d from its event loop, errno=%d
(%s)", this->con.sock.get_fd(), errno,
+ strerror(errno));
+ ink_assert(!"stop_for_migration failed");
+ this->do_io_close();
+ return false;
}
- Connection hold_con;
- hold_con.move(this->con);
-
- void *arg = this->_prepareForMigration();
+ ms.con.move(this->con);
+ ms.arg = this->_prepareForMigration();
+ ms.options = this->options;
+ ms.context = this->get_context();
+ ms.inactivity_timeout_in = this->inactivity_timeout_in;
+ ms.processor = this->_getNetProcessor();
// 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();
+ // Do not mark this closed until the end so it does not get freed by the
other thread too soon
+ this->do_io_close();
Review Comment:
[P1] Synchronize publication of the completed detach before reclamation
Following up on Bryan's reported TSan races: after moving
`stop_for_migration()` before `ms.con.move()` and the state copies, its atomic
`event_loop.exchange(nullptr)` cannot publish those later accesses to the
owning thread. The remaining publication is `do_io_close()` writing the plain
`NetEvent::closed` integer; `INK_WRITE_MEMORY_BARRIER` is empty. Meanwhile
`NetHandler::process_ready_list()` reads `closed` and calls `free_netevent()`
without acquiring the pool/VIO mutex. Holding that mutex here therefore does
not order `Connection::close()` / `options.reset()` in `free_thread()` after
these moves/copies. On a weakly ordered target the owner can observe the close
before the socket relinquishment, close the transferred fd, or reset state that
detach is still reading. Please add a release/acquire handoff covering the
completed detach and the owner's reclamation path (or otherwise synchronize
reclamation); moving the stop first for the failure fallback needs this
corresponding lif
etime ordering.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
