ruby: garnet2.0

Information
Submitter:	Tushar Krishna
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	ahansson, bbeckman, jyin, mporemba

Description

ruby: garnet2.0
Revamped version of garnet with more optimized single-cycle routers,
more configurability, and cleaner code.

Testing Done

Issue Summary

Description	From	Last Updated	Status
The spacing is a bit odd here.	Matthew Poremba	Aug. 24, 2016, 7:57 a.m.	Resolved
Make this method virtual so classes can inherit this one to specify different custom routing computations.	Matthew Poremba	Sept. 14, 2016, 6:19 p.m.	Dropped
The space been the final two > can be removed in C++11.	Matthew Poremba	Aug. 23, 2016, 9:27 p.m.	Resolved
Could all the getter methods and all the setter methods be grouped? That would make this file more readable.	Matthew Poremba	Aug. 23, 2016, 9:19 p.m.	Resolved
white space	Jieming Yin	Sept. 23, 2016, 10:07 p.m.	Resolved
white space	Jieming Yin	Sept. 23, 2016, 10:07 p.m.	Resolved
white space	Jieming Yin	Sept. 23, 2016, 10:07 p.m.	Resolved
empty line	Jieming Yin	Sept. 23, 2016, 10:07 p.m.	Resolved
empty line	Jieming Yin	Sept. 23, 2016, 10:07 p.m.	Resolved
white space	Jieming Yin	Sept. 23, 2016, 10:07 p.m.	Resolved
Missing src_outport & dst_inport here, which will cause segmentation fault(tested in X86_MESI_Two_Level)	Zicong Wang	Sept. 22, 2016, 10:47 p.m.	Open
Missing src_outport & dst_inport here, which will cause segmentation fault(tested in X86_MESI_Two_Level)	Zicong Wang	Sept. 22, 2016, 10:47 p.m.	Open
Missing src_outport & dst_inport here, which will cause segmentation fault(tested in X86_MESI_Two_Level)	Zicong Wang	Sept. 22, 2016, 10:47 p.m.	Open
Missing src_outport & dst_inport here, which will cause segmentation fault(tested in X86_MESI_Two_Level)	Zicong Wang	Sept. 22, 2016, 10:47 p.m.	Open

Thanks for posting the patches Tushar. I think this code is a lot cleaner and i'm interested in trying out the unidirectional links and seperate routing latency for some studies.

One major comment: There is a lot of new code for routing algorithms in this patch. I think it would be useful to convert the RoutingUnit to a SimObject and allow the Router SimObject to accept a RoutingUnit SimObject as a parameter. In this way, you could implement multiple "custom" routing algorithms and swap them via python scripts. I marked two spots below where changes would need to be made.

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

The spacing is a bit odd here.

Show all issues

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

If RoutingUnit is a SimObject, this can be set using the Params struct and the RoutingUnit could be specified by the python scripts.

src/mem/ruby/network/garnet2.0/RoutingUnit.hh (Diff revision 1)

Make this method virtual so classes can inherit this one to specify different custom routing computations.

Show all issues

src/mem/ruby/network/garnet2.0/SwitchAllocator.hh (Diff revision 1)

The space been the final two > can be removed in C++11.

Show all issues

src/mem/ruby/network/garnet2.0/flit.hh (Diff revision 1)

Could all the getter methods and all the setter methods be grouped? That would make this file more readable.

Show all issues

All minor stylistic issues. Some white spaces also need to be removed, but weren't marked in this review since they are quite obvious.

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

Opening brace must be on the same line as the control keyword.

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

Opening brace.

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

'Else' keywords should follow the closing 'if' brace on the same line.

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

Add a space after 'if'. Also, a space before brace.

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

Opening brace.

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

'else' and braces.

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

Indentation

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

Space after 'if'

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

Opening brace

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

Opening brace

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

Opening brace

src/mem/ruby/network/garnet2.0/Router.hh (Diff revision 1)

Space before '{'

src/mem/ruby/network/garnet2.0/Router.hh (Diff revision 1)

Space before '{'

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

Opening brace

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

Opening brace

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

Space before '{'

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

Opening brace

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

Space after 'switch', and opening brace

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

Braces

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

Opening brace

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

Opening brace

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

Opening brace

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

Opening brace

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

Opening brace

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

Braces

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

Opening brace

Summary:

-	network: garnet2.0
+	ruby: garnet2.0

Description:

~		network: garnet2.0
	~	ruby: garnet2.0
		Revamped version of garnet with more optimized single-cycle routers,
		more configurability, and cleaner code.

Diff:

Revision 2 (+4935 -33)

Show changes

	configs/network/Network.py
	configs/topologies/Crossbar.py
	configs/topologies/MeshDirCorners_XY.py
	configs/topologies/Mesh_XY.py
	configs/topologies/Mesh_westfirst.py
	configs/topologies/Pt2Pt.py
	src/base/statistics.cc
	src/mem/ruby/network/BasicRouter.py
	36 more

Looks good overall.

configs/topologies/Mesh_westfirst.py (Diff revision 2)

white space

Show all issues

configs/topologies/Pt2Pt.py (Diff revision 2)

white space

Show all issues

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

white space

Show all issues

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

empty line

Show all issues

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

empty line

Show all issues

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

white space

Show all issues

Missing src_outport & dst_inport when create the mesh links in MeshDirCorners_XY.py, which will cause segmentation fault(tested in X86_MESI_Two_Level)

Tushar Krishna Sept. 23, 2016, 8:58 a.m. (Sept. 23, 2016, 8:58 a.m.)

Hi Zicong,
Thanks for testing the patch on your end. Can you send me the command you used that gave you the segmentation fault?
src_outport and dst_inport are supposed to be optional parameters.
I can add them in to MeshDirCorners_XY but want to find out why they gave an error.

Thanks,
Tushar

Zicong Wang Sept. 24, 2016, 9:11 a.m. (Sept. 24, 2016, 9:11 a.m.)

Hi, Tushar,
Thanks for your working garnet2.0, and the network becomes easier to use for researching.
The command I used is showed as below:

./build/X86_MESI_Two_Level/gem5.debug \
configs/example/se.py \
--cmd=tests/test-progs/hello/bin/x86/linux/hello \
--cpu-type=detailed --ruby --num-cpus=16 \
--caches --cacheline_size=128 \
--l1i_size=16kB --l1i_assoc=2 \
--l1d_size=16kB --l1d_assoc=2 \
--l2cache --l2_size=128kB --l2_assoc=4 --num-l2caches=16 \
--topology=MeshDirCorners_XY --mesh-rows=4 \
--num-dirs=4 --mem-size=4GB \
--sys-clock=1GHz --ruby-clock=1GHz --cpu-clock=1GHz \
--maxinsts=10000 \
--network=garnet2.0 |tee m5out/runscript.log

I test it on a latest changeset(fd783bff017c) which patched with rb35{49,50,54,55,56,57,58}.patch. In addition, I got the same segmentation fault running on my own gem5 project(based on a changeset several month before released). I checked the core file and found out nullptr at NetworkInterface.cc:178 caused the breakdown(a packet injected to vnet 0 incorrectly enqueued at NI 0).
What's more, It works well in X86_MESI_Two_Level(Mesh_XY) and Garnet_standalone. When I add src_outport & dst_inport into MeshDirCorners_XY, it works as well as Mesh_XY.
I hope the information are useful to your working.

Zicong

configs/topologies/MeshDirCorners_XY.py (Diff revision 2)

Missing src_outport & dst_inport here, which will cause segmentation fault(tested in X86_MESI_Two_Level)

Show all issues

configs/topologies/MeshDirCorners_XY.py (Diff revision 2)

Missing src_outport & dst_inport here, which will cause segmentation fault(tested in X86_MESI_Two_Level)

Show all issues

configs/topologies/MeshDirCorners_XY.py (Diff revision 2)

Missing src_outport & dst_inport here, which will cause segmentation fault(tested in X86_MESI_Two_Level)

Show all issues

configs/topologies/MeshDirCorners_XY.py (Diff revision 2)

Missing src_outport & dst_inport here, which will cause segmentation fault(tested in X86_MESI_Two_Level)

Show all issues

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Summary:

Description:

Diff:

Status: Closed (submitted)