----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3230/#review7940 -----------------------------------------------------------
Sorry it took a while for me to get to this... overall this is nice but I'm a little confused on the high-level design, and what the FabricLinks are adding, and how flow-control is working via the FabricLinks. For example, let's say everything is idle, and then two sources (A and B) simultaneously transmit to the same destination (C). If I'm understanding the code correctly, since the A->C and B->C FabricLinks are both idle, both the A and B sender interfaces will transmit their packets along these respective links, but then when the links go to deliver to C's receive interface, one will win and the other will find the interface busy and drop the packet, right? If so, then that means we drop a packet despite having plenty of buffering available. If not, then what am I missing? Thanks! src/dev/net/etherswitch.hh (line 5) <http://reviews.gem5.org/r/3230/#comment6906> Is this license necessary? I believe this extra paragraph only applies to ARM contributions, so there's no reason to include it here. (Same goes for .cc file of course.) src/dev/net/etherswitch.hh (line 109) <http://reviews.gem5.org/r/3230/#comment6916> I think the int-to-bool conversion should be explicit here src/dev/net/etherswitch.hh (line 111) <http://reviews.gem5.org/r/3230/#comment6917> seems asymmetric that there's an inc but no dec src/dev/net/etherswitch.hh (line 117) <http://reviews.gem5.org/r/3230/#comment6918> is this ever not the same as broadcastPeers.size()? src/dev/net/etherswitch.cc (line 350) <http://reviews.gem5.org/r/3230/#comment6912> can these declarations be moved down to the initializations? src/dev/net/etherswitch.cc (line 365) <http://reviews.gem5.org/r/3230/#comment6913> why not 'for (auto sender : tmpInterfaces)'? src/dev/net/etherswitch.cc (line 374) <http://reviews.gem5.org/r/3230/#comment6911> why the memcpy calls? src/dev/net/etherswitch.cc (line 388) <http://reviews.gem5.org/r/3230/#comment6914> seems extreme to do a hash lookup on both ports... every interface already has a number, you really just need lookupDestPort() to return an integer, then have a std::vector<> with each interface that returns the appropriate link from this source to that destination src/dev/net/etherswitch.cc (line 444) <http://reviews.gem5.org/r/3230/#comment6910> would lastUseTime be more accurate than arrivalTime? Why are we tracking this if it's not being used? src/dev/net/etherswitch.cc (line 458) <http://reviews.gem5.org/r/3230/#comment6907> if you're going to be reordering things all the time like this, why not use a list instead of a vector so it's O(1) and not O(n)? src/dev/net/etherswitch.cc (line 474) <http://reviews.gem5.org/r/3230/#comment6908> I don't understand the "try again later"... is it the case that we're trying to do a second broadcast while the first is still in progress? If so, what happens to the second broadcast packet, does it get dropped? And does this mean that the initial broadcast only progresses when another broadcast comes along? src/dev/net/etherswitch.cc (line 500) <http://reviews.gem5.org/r/3230/#comment6909> seems like it would be a lot simpler to just initialize peers to interfaces on the initial broadcast, and then this function could have a single loop over peers that's used in either case - Steve Reinhardt On Jan. 10, 2016, 9:58 p.m., Mohammad Alian wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3230/ > ----------------------------------------------------------- > > (Updated Jan. 10, 2016, 9:58 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11292:ed7527fcc338 > --------------------------- > dist,dev: add an ethernet switch model > > > Diffs > ----- > > src/dev/net/Ethernet.py 9d2364203316 > src/dev/net/SConscript 9d2364203316 > src/dev/net/etherswitch.hh PRE-CREATION > src/dev/net/etherswitch.cc PRE-CREATION > > Diff: http://reviews.gem5.org/r/3230/diff/ > > > Testing > ------- > > several testing done with different benchmarks and different switch sizes > > > Thanks, > > Mohammad Alian > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
