Quic pollcont, Reuse the NetHandler for UDP packet by scw00 · Pull Request #3056 · apache/trafficserver

scw00 · 2018-01-30T13:54:06Z

Base on @oknet design.

We create QUICPollCont to dispatch UDPacket to different qvc. And NetHandler will callback to qvc handler to process packet.

masaori335 · 2018-01-31T02:06:51Z

I could build this PR and complete handshake with ngtcp2 :) There're some TODOs, but this looks good.

masaori335 · 2018-01-31T02:07:34Z

I got this crash when I try our quic client.

Process 55750 stopped
* thread #2, name = '[ET_UDP 0]', stop reason = EXC_BAD_ACCESS (code=1, address=0x50)
    frame #0: 0x00000001000708ea traffic_quic`Mutex_trylock(location=0x0000000102519bc8, ahandler=0x0000000000000000, m=0x0000000000000000, t=0x000000010230d000) at I_Lock.h:288
   285  {
   286    ink_assert(t != 0);
   287    ink_assert(t == (EThread *)this_thread());
-> 288    if (m->thread_holding != t) {
   289      if (!ink_mutex_try_acquire(&m->the_mutex)) {
   290  #ifdef DEBUG
   291        lock_waiting(m->srcloc, m->handler);
Target 0: (traffic_quic) stopped.
(lldb) bt
* thread #2, name = '[ET_UDP 0]', stop reason = EXC_BAD_ACCESS (code=1, address=0x50)
  * frame #0: 0x00000001000708ea traffic_quic`Mutex_trylock(location=0x0000000102519bc8, ahandler=0x0000000000000000, m=0x0000000000000000, t=0x000000010230d000) at I_Lock.h:288
    frame #1: 0x0000000100070811 traffic_quic`MutexTryLock::MutexTryLock(this=0x0000000102519be0, location=0x0000000102519bc8, ahandler=0x0000000000000000, am=0x000000010230d020, t=0x000000010230d000) at I_Lock.h:554
    frame #2: 0x000000010006c045 traffic_quic`MutexTryLock::MutexTryLock(this=0x0000000102519be0, location=0x0000000102519bc8, ahandler=0x0000000000000000, am=0x000000010230d020, t=0x000000010230d000) at I_Lock.h:553
    frame #3: 0x0000000100078d88 traffic_quic`QUICNetVConnection::acceptEvent(this=0x00000001027c4aa0, event=2501, e=0x0000000102814c40) at QUICNetVConnection.cc:102
    frame #4: 0x000000010006c960 traffic_quic`Continuation::handleEvent(this=0x00000001027c4aa0, event=2501, data=0x0000000102814c40) at I_Continuation.h:153
    frame #5: 0x000000010010e0b3 traffic_quic`EThread::process_event(this=0x000000010230d000, e=0x0000000102814c40, calling_code=2501) at UnixEThread.cc:130
    frame #6: 0x000000010010e42d traffic_quic`EThread::process_queue(this=0x000000010230d000, NegativeQueue=0x0000000102519e08, ev_count=0x0000000102519dd8, nq_count=0x0000000102519ddc) at UnixEThread.cc:169
    frame #7: 0x000000010010ea71 traffic_quic`EThread::execute_regular(this=0x000000010230d000) at UnixEThread.cc:250
    frame #8: 0x000000010010f06a traffic_quic`EThread::execute(this=0x000000010230d000) at UnixEThread.cc:324
    frame #9: 0x000000010010d064 traffic_quic`spawn_thread_internal(a=0x0000000102017030) at Thread.cc:85
    frame #10: 0x00007fff6a0226c1 libsystem_pthread.dylib`_pthread_body + 340
    frame #11: 0x00007fff6a02256d libsystem_pthread.dylib`_pthread_start + 377
    frame #12: 0x00007fff6a021c5d libsystem_pthread.dylib`thread_start + 13

zizhong · 2018-01-31T18:36:47Z

iocore/net/QUICNet.cc

+  NetHandler *nh       = get_NetHandler(this->mutex->thread_holding);
+
+  // Process the ASLL
+  SList(UDPPacketInternal, alink) aq(inQueue.popall());


do you need both aq and result?

result is used to make sure all packet (which we pop from result) was in original order. Since template <class C, class L = typename C::Link_link> class SLL doesn't have dequeue method.

zizhong · 2018-01-31T18:50:50Z

iocore/net/QUICNet.cc

+
+  new ((ink_dummy_for_new *)quicpc) QUICPollCont(thread->mutex, nh);
+
+  thread->schedule_every(quicpc, -9);


a int const is better

Sounds reasonable!

I just copy the line from initialize_thread_for_udp_net(). Please update both of them. :-)

zizhong · 2018-01-31T18:52:09Z

iocore/net/QUICNet.cc

+    if (vc != nullptr) {
+      int isin = ink_atomic_swap(&vc->read.in_enabled_list, 1);
+      if (!isin) {
+        nh->read_enable_list.push(vc);


do we need to hold the lock to modify the nh?

read_enable_list is an atomic queue. so we don't need to hold the nh lock.

zizhong · 2018-01-31T19:11:50Z

iocore/net/P_UnixNet.h

 TS_INLINE int
 EventIO::refresh(int e)
 {
+  if (fd == NO_FD) {


no sure about this. can we figure out a better way?

EventIO is a interface for polling system. Because the QUIC Connection does not supported by polling system, I left the hack code here. I can add a new member within EventIO to indicate the situation.

zizhong · 2018-01-31T19:19:48Z

iocore/net/P_UnixNet.h

@@ -575,8 +575,15 @@ EventIO::start(EventLoop l, NetAccept *vc, int events)
 TS_INLINE int
 EventIO::start(EventLoop l, UnixNetVConnection *vc, int events)


here, http://people.apache.org/~oknet/quic_arch.2017112813.png, pollCont is meant to call back UnixUdpConnection but here it's call back to UnixNetVConnection?

Yes, but currently QCon doesn't inherit from Connection. So we just leave this here.

And UnixUDPConnection is used to recv socket, it is running in UDP thread.

oknet · 2018-02-03T10:47:22Z

iocore/net/UnixUDPNet.cc

  g_udp_numSendRetries = g_udp_numSendRetries < 0 ? 0 : g_udp_numSendRetries;

-  thread->schedule_every(get_UDPPollCont(thread), -9);
+  thread->schedule_every(get_UDPPollCont(thread), -UDP_PERIOD);


-HRTIME_MSECONDS(9) maybe better.

oknet · 2018-02-03T10:49:48Z

iocore/net/QUICNetVConnection.cc

+    free(t);
+    return EVENT_DONE;
+  }
+  this->read.enabled = 1;


This is a hack now, because no one calls do_io_xxx of QUICNetVC.

Can you add a comment here, so we can complete it later.

oknet · 2018-02-03T10:52:24Z

iocore/net/QUICNetVConnection.cc

+  if ((res = nh->startIO(this)) < 0) {
+    // FIXME: startIO only return 0 now! what should we do if it failed ?
+  }
+


Please add this->read.enabled = 1; at here.

oknet · 2018-02-03T11:10:05Z

iocore/net/QUICNetVConnection.cc

  this->_packet_handler              = packet_handler;
  this->_original_quic_connection_id = original_cid;
  this->_quic_connection_id.randomize();
+  this->ep.syscall = false;


Move to QUICNetProcessor::allocate_vc(EThread *t) ?

maskit · 2018-02-05T00:42:24Z

Where are packets in longInQueue and shortInQueue dequeued?

scw00 · 2018-02-05T00:46:27Z

longInQueue and shortInQueue are useless currently ! It is going to be used for reordering the rtt0 first packet and handshake packet .

maskit · 2018-02-05T01:09:22Z

iocore/net/QUICPacketHandler.cc

  }
+
+  // Push the packet into QUICPollCont
+  udp_packet->data.ptr = vc;


Hmm, I'm not so thrilled with having this data member. Do we have other options?

maskit · 2018-02-05T01:19:13Z

iocore/net/QUICNet.cc

+
+  new ((ink_dummy_for_new *)quicpc) QUICPollCont(thread->mutex, nh);
+
+  thread->schedule_every(quicpc, -UDP_PERIOD);


HRTIME_MSECONDS?

Why are these negative value?

negative value means quic pc should be inserted into the priority queue. It will be triggered every loop in evensystem.

@maskit yes, the same for DNS, no very intuitive though.

scw00 · 2018-02-06T06:10:26Z

Based on @maskit 's suggestion.

I introduced a new struct to packet the UDPPacket and QVC, and removed the hack in UDPPacket.

maskit · 2018-02-06T08:22:31Z

Looks good to me. Please remove WIP label when it's ready. (I assume some of comments from oknet have to be addressed.)

maskit

👍

oknet

Next, make the QUICNetVC inherits from RefCountObj, increase the reference count once a QUICPollEvent is created.

oknet · 2018-02-07T08:51:04Z

Current QUIC diagram:

The closedQueue will be added soon.

scw00 added the QUIC label Jan 30, 2018

scw00 added this to the 8.0.0 milestone Jan 30, 2018

scw00 force-pushed the QUICPollCont branch from b54555f to 5242991 Compare January 30, 2018 14:02

bryancall assigned scw00 Jan 30, 2018

zizhong reviewed Jan 31, 2018

View reviewed changes

oknet and others added 3 commits February 3, 2018 09:01

Add QUICPollCont

0dd3bca

complete pollcont

95bd1db

[QUIC Client] fix the quic client

2837410

scw00 force-pushed the QUICPollCont branch 2 times, most recently from bcca475 to 3b18ea3 Compare February 3, 2018 01:40

Add syscall into EventIO for qvc to avoid system call

ef15385

scw00 force-pushed the QUICPollCont branch from 3b18ea3 to ef15385 Compare February 3, 2018 01:43

scw00 added the WIP label Feb 3, 2018

scw00 force-pushed the QUICPollCont branch from b9661b9 to 0984cd2 Compare February 3, 2018 03:28

scw00 requested review from masaori335 and maskit February 3, 2018 04:06

oknet reviewed Feb 3, 2018

View reviewed changes

scw00 force-pushed the QUICPollCont branch from 0984cd2 to a201213 Compare February 3, 2018 11:38

connect_re processed by nethandler

c1b2b27

scw00 force-pushed the QUICPollCont branch from a201213 to c1b2b27 Compare February 3, 2018 11:59

maskit reviewed Feb 5, 2018

View reviewed changes

add QUICPollEvent to packet UDPPacket and QVC

d7be5ef

scw00 force-pushed the QUICPollCont branch from 147c29a to d7be5ef Compare February 6, 2018 08:16

scw00 removed the WIP label Feb 6, 2018

maskit approved these changes Feb 6, 2018

View reviewed changes

oknet approved these changes Feb 6, 2018

View reviewed changes

scw00 merged commit ba5ff99 into apache:quic-latest Feb 7, 2018

zwoop modified the milestones: 8.0.0, QUIC Mar 9, 2018

scw00 deleted the QUICPollCont branch March 15, 2018 06:29

scw00 mentioned this pull request Nov 19, 2018

QUIC: Fixed the signalActivity is not work on QUIC branch #4622

Merged

scw00 mentioned this pull request May 30, 2019

Update UDPNet for QUIC #5588

Merged


		new ((ink_dummy_for_new *)quicpc) QUICPollCont(thread->mutex, nh);

		thread->schedule_every(quicpc, -9);

		@@ -575,8 +575,15 @@ EventIO::start(EventLoop l, NetAccept *vc, int events)
		TS_INLINE int
		EventIO::start(EventLoop l, UnixNetVConnection *vc, int events)


		new ((ink_dummy_for_new *)quicpc) QUICPollCont(thread->mutex, nh);

		thread->schedule_every(quicpc, -UDP_PERIOD);

Conversation

scw00 commented Jan 30, 2018

Uh oh!

masaori335 commented Jan 31, 2018

Uh oh!

masaori335 commented Jan 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scw00 Feb 1, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maskit commented Feb 5, 2018

Uh oh!

scw00 commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scw00 Feb 5, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

scw00 commented Feb 6, 2018

Uh oh!

maskit commented Feb 6, 2018

Uh oh!

maskit left a comment

Choose a reason for hiding this comment

Uh oh!

oknet left a comment

Choose a reason for hiding this comment

Uh oh!

oknet commented Feb 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

scw00 Feb 1, 2018 •

edited

Loading

scw00 commented Feb 5, 2018 •

edited

Loading

scw00 Feb 5, 2018 •

edited

Loading