----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3753/#review9195 -----------------------------------------------------------
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 (line 139) <http://reviews.gem5.org/r/3753/#comment7895> This is a great change! Anyone wanting to add more stats can do that here. src/mem/ruby/network/garnet2.0/NetworkInterface.cc (line 200) <http://reviews.gem5.org/r/3753/#comment7896> 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. src/mem/ruby/network/garnet2.0/NetworkInterface.cc (line 213) <http://reviews.gem5.org/r/3753/#comment7900> 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) :) src/mem/ruby/network/garnet2.0/NetworkInterface.cc (line 238) <http://reviews.gem5.org/r/3753/#comment7898> 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. src/mem/ruby/network/garnet2.0/NetworkInterface.cc (line 245) <http://reviews.gem5.org/r/3753/#comment7897> 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. src/mem/ruby/network/garnet2.0/NetworkInterface.cc (line 257) <http://reviews.gem5.org/r/3753/#comment7899> 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. - Tushar Krishna On Dec. 8, 2016, 11:34 p.m., Matthew Poremba wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3753/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2016, 11:34 p.m.) > > > Review request for Default and Tushar Krishna. > > > Repository: gem5 > > > Description > ------- > > Changeset 11761:e47a6baba2f2 > --------------------------- > ruby: Check MessageBuffer space in garnet NetworkInterface > > Garnet's NetworkInterface does not consider the size of MessageBuffers when > ejecting a Message from the network. Add a size check for the MessageBuffer > and only enqueue if space is available. If space is not available, the > message if placed in a queue and the credit is held. A callback from the > MessageBuffer is implemented to wake the NetworkInterface. If there are > messages in the stalled queue, they are processed first, in a FIFO manner > and if succesfully ejected, the credit is finally sent back upstream. The > maximum size of the stall queue is equal to the number of valid VNETs > with MessageBuffers attached. > > > Diffs > ----- > > src/mem/ruby/network/MessageBuffer.hh > 78ef8daecd81de0c392034809b3bc155396bf983 > src/mem/ruby/network/MessageBuffer.cc > 78ef8daecd81de0c392034809b3bc155396bf983 > src/mem/ruby/network/garnet2.0/NetworkInterface.hh > 78ef8daecd81de0c392034809b3bc155396bf983 > src/mem/ruby/network/garnet2.0/NetworkInterface.cc > 78ef8daecd81de0c392034809b3bc155396bf983 > > Diff: http://reviews.gem5.org/r/3753/diff/ > > > Testing > ------- > > > Thanks, > > Matthew Poremba > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
