> On Dec. 9, 2016, 12:26 a.m., Tushar Krishna wrote: > > This is an excellent patch! Something that has been long due. > > (BTW does the same problem occur in simple network as well?)
Thanks for the fast review! I've updated it based on your comments and i'm rerunning some longer tests to make sure everything is still OK. Also- SimpleNetwork's Throttles take care of the machanism provided by this patch already, so it only needs to be implemented in Garnet2.0. > On Dec. 9, 2016, 12:26 a.m., Tushar Krishna wrote: > > src/mem/ruby/network/garnet2.0/NetworkInterface.cc, line 200 > > <http://reviews.gem5.org/r/3753/diff/1/?file=63957#file63957line200> > > > > 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. Moved this to a new function. > On Dec. 9, 2016, 12:26 a.m., Tushar Krishna wrote: > > src/mem/ruby/network/garnet2.0/NetworkInterface.cc, line 213 > > <http://reviews.gem5.org/r/3753/diff/1/?file=63957#file63957line213> > > > > 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) :) Good point. Moved this to a new function to reduce code reuse. > On Dec. 9, 2016, 12:26 a.m., Tushar Krishna wrote: > > src/mem/ruby/network/garnet2.0/NetworkInterface.cc, line 257 > > <http://reviews.gem5.org/r/3753/diff/1/?file=63957#file63957line257> > > > > 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. Good suggestion. I consolidated this and the previous two issues into one block. - Matthew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3753/#review9195 ----------------------------------------------------------- On Dec. 9, 2016, 1:58 a.m., Matthew Poremba wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3753/ > ----------------------------------------------------------- > > (Updated Dec. 9, 2016, 1:58 a.m.) > > > Review request for Default and Tushar Krishna. > > > Repository: gem5 > > > Description > ------- > > Changeset 11761:fd4126ef5e73 > --------------------------- > 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 [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
