This is an excellent patch! Something that has been long due.

(BTW does the same problem occur in simple network as well?)

src/mem/ruby/network/garnet2.0/NetworkInterface.cc (Diff revision 1)

This is a great change! Anyone wanting to add more stats can do that here.

src/mem/ruby/network/garnet2.0/NetworkInterface.cc (Diff revision 1)

Stylistic comment:

Can we move this piece of code to a new function which returns true/false and sets the value of unstalledMessage?
Otherwise the wakeup function is very long now and performs the actual action of reading from the link only at the very end.

Show all issues

1. Matthew Poremba Dec. 8, 2016, 5:59 p.m. (Dec. 8, 2016, 5:59 p.m.)

   Moved this to a new function.

src/mem/ruby/network/garnet2.0/NetworkInterface.cc (Diff revision 1)

These three lines should become a new send_credit function.
Credit *credit_flit = new Credit(stallFlit->get_vc(), true, curCycle());
outCreditQueue->insert(credit_flit);

outCreditLink->scheduleEventAbsolute(clockEdge(Cycles(1)));
The function can be called here and later.

Having two pieces of code where credits are being created and sent can lead to a lot of bugs whenever anyone makes changes and forgets to make changes at both places (have seen that a lot from experience) :)

Show all issues

1. Matthew Poremba Dec. 8, 2016, 5:59 p.m. (Dec. 8, 2016, 5:59 p.m.)

   Good point. Moved this to a new function to reduce code reuse.

src/mem/ruby/network/garnet2.0/NetworkInterface.cc (Diff revision 1)

Looks like only HEAD_TAIL and TAIL_ flits set extLinkAvail to false and are consumed here, while other flits are consumed later in the code. It will be a little confusing for a newbie looking at the code.

Show all issues

src/mem/ruby/network/garnet2.0/NetworkInterface.cc (Diff revision 1)

Adding to the point above - 
My worry is that someone new looking at the code might get a little confused about the fact that the consumeLink happens twice. And that the HEAD and HEAD_TAIL are sometimes consumed here, and sometimes later in the code.
See my suggestion below on some code restructuring.

Show all issues

src/mem/ruby/network/garnet2.0/NetworkInterface.cc (Diff revision 1)

Looks like the link not consumed if there are stalled messages.

But this would mean that the router will keep inserting flits into the link which may not get consumed, effectively modeling a large queue inside the link which is incorrect.

The NI always consumes the link. This is because the router checked for credits before sending to the NI.


Here is my suggestion for this piece of code:

if (inNetLink->isReady(curCycle()) {

flit *t_flit = inNetLink->consumeLink();
// Now check if message buffer has slots.

// If it does, then { (i) insert msg into msg buffer, (ii) call send_credit function. }

// else { insert msg into stall queue.}
// Implement a send_credit function that can be called from here or when the stall queue is read.
...
The extLinkAvail piece of code is not needed. It gets subsumed here.

Show all issues

1. Matthew Poremba Dec. 8, 2016, 5:59 p.m. (Dec. 8, 2016, 5:59 p.m.)

   Good suggestion. I consolidated this and the previous two issues into one block.

src/mem/ruby/network/garnet2.0/NetworkInterface.cc (Diff revision 2)

If a flit is waiting for the output buffer to become free, this will get counted in its network delay component in the current implementation.

It should be counted in its queueing delay component I think ...

Queueing delay is the delay at the message buffers and NICs, network delay is from src NI to dest NI and includes delay in routers and links.

Show all issues

1. Matthew Poremba Dec. 9, 2016, 10:56 a.m. (Dec. 9, 2016, 10:56 a.m.)

   That's true. I think this will need to be changed to account for queueing latency at the source plus time in stall queue. I could add a member/method to flit like get/set_dequeue_time which is set when the link is consumed. Then the calculation becomes network_delay = t_flit->get_dequeue_time() - t_flit->get_enqueue_time() and queueing_delay = t_flit->get_src_delay() + (curCycle() - t_flit->get_dequeue_time()). Will that suffice? We could also split the source and "destination" delays into two stats if we want. That would still require modifying flit class, though.

2. Tushar Krishna Dec. 9, 2016, 11:55 a.m. (Dec. 9, 2016, 11:55 a.m.)

   Yes this suffices. I don't think we need to break the input queueing delay and output queueing delay by default.
   
   But we can show them separately in case someone wants to pring both separately.
   
   How about doing something like this:
   
   int source_queueing_delay = t_flit->get_src_delay();
   
   int destination_queueing_delay = (curCycle() - t_flit->get_dequeue_time());
   // source_queueing_delay and destination_queueing_delay can be printed as 
   
   // separate stats if required
   
   queueing_delay = source_queueing_delay + destination_queueing_delay.

src/mem/ruby/network/garnet2.0/NetworkInterface.cc (Diff revision 2)

See my comment below in the function definition regarding the name of this variable ...

Show all issues

src/mem/ruby/network/garnet2.0/NetworkInterface.cc (Diff revision 2)

We should consume the link irrespective of whether the state of the stall queue. If we do not consume, effectively we are modeling a quueue inside the network link.
I can see why you added this condition though - you don't want more than one enqueue into the message buffer in the same cycle.

If the stall queue already enqueued something, you are making the one from the link wait.
How about the following:
bool unstalledMessage = checkStallQueue();
if (inNetLink->isReady(curCycle()) {

flit *t_flit = inNetLink->consumeLink();
...

if (outNode_ptr[vnet]->areNSlotsAvailable(1, curTime) && !unstalledMessage) {

// enqueue into protocol buffer

}

else {

// enqueue into stall queue

}

Show all issues

1. Matthew Poremba Dec. 9, 2016, 10:56 a.m. (Dec. 9, 2016, 10:56 a.m.)

   Ok, I understand this now. The networkLink is internally modeled as a std::vector and becomes a queue. I will make the suggested change.

2. Matthew Poremba Dec. 12, 2016, 3:08 p.m. (Dec. 12, 2016, 3:08 p.m.)

   Another thing this conditional did was prevent multiple credits from being enqueued in the same cycle. I've change the latest diff to always consume the link, but it is possible enqueue multiple credit flits now. To make sure there are no deadlocks due to multiple credits in the credit queue, I also added a size check to the credit queue to schedule another wakeup in the next cycle if the queue is not empty.

src/mem/ruby/network/garnet2.0/NetworkInterface.cc (Diff revision 2)

The name of this variable is a little confusing.

Hmm I think I can see why you named it like that. If a message that was stalled gets unstalled, then this variable becomes true.
But if the stall queue is empty, unstalledMessage is returned false, which means stalledMessage is true .. which is not quite right.

Show all issues

1. Matthew Poremba Dec. 9, 2016, 10:56 a.m. (Dec. 9, 2016, 10:56 a.m.)

   I see. How about messageEnqueued or similar to imply only one message per cycle is enqueued?

2. Tushar Krishna Dec. 9, 2016, 11:55 a.m. (Dec. 9, 2016, 11:55 a.m.)

   yeah ...
   
   messageEnqueuedThisCycle .. ?

Any thoughts on the latest diff?

src/mem/ruby/network/garnet2.0/NetworkInterface.cc (Diff revision 3)

Shall the NI reschedule itself if all stall flits in the m_stall_queue fail ejecting to outNode_ptr? It seems possible that none of the outNode_ptr is able to accecpt flits, while the router is not sending any flit to the NI.

Show all issues

1. Matthew Poremba Jan. 6, 2017, 3:59 p.m. (Jan. 6, 2017, 3:59 p.m.)

   When something is placed in m_stall_queue, a callback is registered with the corresponding outNode_ptr's MessageBuffer to wakeup the NI. This is preferable to checking if checkStallQueue returns false, since that could result in waking up unnecssarily every cycle until something is ejected (if there is nothing else needs to wakeup). There does seem to be one corner case left, however. The callback is unregistered once any message from a vnet is ejected. There could be a situation where all VCs are stalled on a vnet and the callback is unregistered, resulting in potential deadlock if a wakeup never again occurs to eject the remaining stalled VCs. I've added a per-vnet counter which will only unregister the callback if all VCs have been ejected.

Ship It!

Ship It!

src/mem/ruby/network/garnet2.0/NetworkInterface.cc (Diff revision 4)

Hey Matt,

There is a minor change I have been wanting to push in which you can add as part of this patch:
We count network_delay as dequeue_time - enqueue_time.
A minor issue is that the "enqueue_time" includes an additional cycle at the source NIC when the packet was created.
For example:

Suppose we have a 1-flit packet that we have to send one hop from Node 0 to Node 1.

We create in Cycle 1.
Cycle 1: flit enqueued by NIC 0

Cycle 2: flit in link_NIC0-Router0

Cycle 3: flit in Router 0

Cycle 4: flit in link_Router0-Router1

Cycle 5: flit in Router 1

Cycle 6: flit in link_Router1-NIC1

Cycle 7: flit dequeued by NIC 1.
Network delay will be calculated as 7-1 = 6 cycles.

But the actual "network" delay was 5 cycles (cycle 2 to cycle 6).
The additional 1-cycle in NIC0 is going to be part of the src_queueing_delay so it is being accounted for.
Does this make sense?
If yes, can you update network delay to 

dequeue_time - enqueue_time - 1 ?
And then ship it.
Thanks,

Tushar

Show all issues

1. Matthew Poremba Jan. 10, 2017, 11:53 a.m. (Jan. 10, 2017, 11:53 a.m.)

   I think I follow. Just to make sure I understand, the additional cycle comes from the source side, correct? Since flitisizeMessage places the flit in a VC output buffer but it is not injected until the next cycle? If that's correct, I'll fold that change into this patch.

2. Tushar Krishna Jan. 10, 2017, 12:13 p.m. (Jan. 10, 2017, 12:13 p.m.)

   Yes that's correct.
   Thanks.

Ship It!