dev: add an ethernet switch model

Information
Submitter:	Tony Gutierrez
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10259:eb23219d83a4
---------------------------
dev: add an ethernet switch model

this patch adds a very simple ethernet switch model. the basic design
behind the switch is to modularize the interface, fabric, and overall
switch model as much as possible. The switch model:

1) buffers incoming/outgoing packets in the ports
2) utilizes Links from EtherLink as the point-to-point connections
   in the switch fabric
3) uses a simple least-recently-granted arbitration policy to forward packets,
   and doesn't provide any support for things like flow control. it also does
   not provide any features outside the link layer, which some switches may
   support e.g., ARP, filtering, etc.
* * *
junk

Testing Done

Successfully ran 3 systems with 1 memcached server and 2 memcached clients simultaneously issuing requests.

Issue Summary

Description	From	Last Updated	Status
port speed?	Andreas Hansson	July 22, 2014, 10:13 a.m.	Resolved
Seem to be a duplicate	Andreas Hansson	July 22, 2014, 10:13 a.m.	Resolved
has it gotten to a point where it's worth adding a src/dev/ether directory?	Andreas Hansson	July 10, 2014, 5:48 a.m.	Open
whitespace	Andreas Hansson	July 22, 2014, 10:13 a.m.	Resolved
safe_cast? Somehow this seems a bit dubious	Andreas Hansson	July 10, 2014, 5:48 a.m.	Open
dev	Andreas Hansson	July 22, 2014, 10:13 a.m.	Resolved
Should it really be ticked?	Andreas Hansson	July 22, 2014, 10:13 a.m.	Resolved
const	Andreas Hansson	July 22, 2014, 10:24 a.m.	Resolved
const	Andreas Hansson	July 22, 2014, 10:24 a.m.	Resolved
const	Andreas Hansson	July 22, 2014, 10:24 a.m.	Resolved
is this not just the length of the vector?	Andreas Hansson	July 10, 2014, 5:48 a.m.	Open
should this be public?	Andreas Hansson	July 22, 2014, 10:14 a.m.	Resolved
const?	Andreas Hansson	July 22, 2014, 10:23 a.m.	Dropped
const?	Andreas Hansson	July 22, 2014, 10:23 a.m.	Dropped
if you want: for (auto i: interfaces) delete i;	Andreas Hansson	July 22, 2014, 10:14 a.m.	Resolved
fatal? even fatal_if	Andreas Hansson	July 22, 2014, 10:14 a.m.	Resolved
why + 10?	Andreas Hansson	July 22, 2014, 10:14 a.m.	Resolved
why 1000? Should this whole thing not be event driven rather than ticked?	Andreas Hansson	July 22, 2014, 10:14 a.m.	Resolved
perhaps i just don't understand the rules here, should this not return true when successfully sending something?	Andreas Hansson	July 22, 2014, 10:22 a.m.	Resolved
auto and range-based for is your friend :-)	Andreas Hansson	July 22, 2014, 10:14 a.m.	Resolved
auto	Andreas Hansson	July 22, 2014, 10:14 a.m.	Resolved
seems like a bug to me...	Andreas Hansson	July 22, 2014, 10:14 a.m.	Resolved
Not sure what the style guide says about the spacing here, but I've been using for (auto i: list) Does ...	Andreas Hansson	June 26, 2015, 2:28 a.m.	Dropped
Could merge these	Andreas Hansson	July 22, 2014, 3:57 p.m.	Dropped
seems a bit excessive :-)	Andreas Hansson	July 22, 2014, 3:57 p.m.	Resolved
Why is the copy needed? Could you add a comment?	Andreas Hansson	July 24, 2014, 12:38 p.m.	Resolved

Testing Done:

Successfully ran 3 systems with 1 memcached server and 2 memcached clients simultaneously issuing requests.

src/dev/Ethernet.py (Diff revision 1)

port speed?

Show all issues

Tony Gutierrez July 22, 2014, 11:25 a.m. (July 22, 2014, 11:25 a.m.)

this is the fabric links' speed. i changed the name to be more descriptive.

src/dev/Ethernet.py (Diff revision 1)

Seem to be a duplicate

Show all issues

src/dev/SConscript (Diff revision 1)

has it gotten to a point where it's worth adding a src/dev/ether directory?

Show all issues

Tony Gutierrez July 22, 2014, 11:25 a.m. (July 22, 2014, 11:25 a.m.)
```
That is probably a good idea. I don't know what others think.
```

src/dev/etherlink.hh (Diff revision 1)

whitespace

Show all issues

src/dev/etherlink.cc (Diff revision 1)

safe_cast?

Somehow this seems a bit dubious

Show all issues

Tony Gutierrez July 22, 2014, 11:25 a.m. (July 22, 2014, 11:25 a.m.)

Because this code is only used with EtherLink objects this should be safe. I could make it a dynamic cast if you'd like.

src/dev/etherswitch.hh (Diff revision 1)

dev

Show all issues

src/dev/etherswitch.hh (Diff revision 1)

Should it really be ticked?

Show all issues

src/dev/etherswitch.hh (Diff revision 1)

const

Show all issues

src/dev/etherswitch.hh (Diff revision 1)

const

Show all issues

src/dev/etherswitch.hh (Diff revision 1)

const

Show all issues

src/dev/etherswitch.hh (Diff revision 1)

is this not just the length of the vector?

Show all issues

Tony Gutierrez July 22, 2014, 11:25 a.m. (July 22, 2014, 11:25 a.m.)

this keeps track of how many links are being broadcast to, the broadcast peers keeps track of which peers still need to be broadcast to, in case the link is busy.

src/dev/etherswitch.hh (Diff revision 1)

should this be public?

Show all issues

src/dev/etherswitch.hh (Diff revision 1)

const?

Show all issues

Tony Gutierrez July 22, 2014, 11:25 a.m. (July 22, 2014, 11:25 a.m.)

can't be made const, calls methods that may change certain fields.

src/dev/etherswitch.hh (Diff revision 1)

const?

Show all issues

Tony Gutierrez July 22, 2014, 11:25 a.m. (July 22, 2014, 11:25 a.m.)

can't be made const, calls methods that may change certain fields.

src/dev/etherswitch.cc (Diff revision 1)

if you want:

for (auto i: interfaces)
    delete i;

Show all issues

Tony Gutierrez July 22, 2014, 11:25 a.m. (July 22, 2014, 11:25 a.m.)

Fixed throughout. Need to remind myself to use c++11 for things like this...

src/dev/etherswitch.cc (Diff revision 1)

fatal?

even fatal_if

Show all issues

src/dev/etherswitch.cc (Diff revision 1)

why + 10?

Show all issues

src/dev/etherswitch.cc (Diff revision 1)

why 1000?

Should this whole thing not be event driven rather than ticked?

Show all issues

src/dev/etherswitch.cc (Diff revision 1)

perhaps i just don't understand the rules here, should this not return true when successfully sending something?

Show all issues

src/dev/etherswitch.cc (Diff revision 1)

auto and range-based for is your friend :-)

Show all issues

src/dev/etherswitch.cc (Diff revision 1)

auto

Show all issues

src/dev/etherswitch.cc (Diff revision 1)

seems like a bug to me...

Show all issues

This looks like a great start. There are plenty places where auto and range-based for loops will make the code a lot easier to read.

In general I am a bit surprised to see the ticked interface being used, as the switch clearly just does what ever it is asked to do, and should be able to rely on events scheduled when something arrives.

Tony Gutierrez July 22, 2014, 11:25 a.m. (July 22, 2014, 11:25 a.m.)

Sorry the ticking was just for temporary testing purposes. I shouldn't have submitted it that way.

Description:

~		Changeset 10243:d135e653e6eb
	~	Changeset 10259:2d25a9f9969a

		dev: add an ethernet switch model

		this patch adds a very simple ethernet switch model. the basic design
		behind the switch is to modularize the interface, fabric, and overall
		switch model as much as possible. The switch model:

		1) buffers incoming/outgoing packets in the ports
		2) utilizes Links from EtherLink as the point-to-point connections
		in the switch fabric
		3) uses a simple least-recently-granted arbitration policy to forward packets,
		and doesn't provide any support for things like flow control. it also does
		not provide any features outside the link layer, which some switches may
		support e.g., ARP, filtering, etc.
	+
	+
	+
	+	junk

Diff:

Revision 2 (+573 -14)

Show changes

	src/dev/Ethernet.py
	src/dev/SConscript
	src/dev/etherint.hh
	src/dev/etherlink.hh
	src/dev/etherlink.cc
	src/dev/etherswitch.hh
	src/dev/etherswitch.cc

Description:

		Changeset 10259:2d25a9f9969a

		dev: add an ethernet switch model

		this patch adds a very simple ethernet switch model. the basic design
		behind the switch is to modularize the interface, fabric, and overall
		switch model as much as possible. The switch model:

		1) buffers incoming/outgoing packets in the ports
		2) utilizes Links from EtherLink as the point-to-point connections
		in the switch fabric
		3) uses a simple least-recently-granted arbitration policy to forward packets,
		and doesn't provide any support for things like flow control. it also does
		not provide any features outside the link layer, which some switches may
		support e.g., ARP, filtering, etc.
-
-
-
-		junk

src/dev/etherswitch.cc (Diff revision 2)

Not sure what the style guide says about the spacing here, but I've been using

for (auto i: list)

Does anyone have a strong opinion?

Show all issues

Tony Gutierrez July 24, 2014, 12:38 p.m. (July 24, 2014, 12:38 p.m.)

Style guide doesn't mention anything about colons except when they appear with labels. In current code there is never a space if it appears at the end of a label or as part of the scope operator. Only other place colons are used are as part of initialization lists, and in that case there is always a space. I don't have a strong opinion, but I typically associate a colon with a label, and just to differentiate I like to put the space.

src/dev/etherswitch.cc (Diff revision 2)

Could merge these

Show all issues

Tony Gutierrez July 24, 2014, 12:42 p.m. (July 24, 2014, 12:42 p.m.)

broadcasting() basically checks the number broadcasting, so after decrementing broadcasting() could change.

src/dev/etherswitch.cc (Diff revision 2)

seems a bit excessive :-)

Show all issues

src/dev/etherswitch.cc (Diff revision 2)

Why is the copy needed? Could you add a comment?

Show all issues

Some very minor things. Thanks for all the changes.

Description:

~		Changeset 10259:2d25a9f9969a
	~	Changeset 10259:eb23219d83a4

		dev: add an ethernet switch model

		this patch adds a very simple ethernet switch model. the basic design
		behind the switch is to modularize the interface, fabric, and overall
		switch model as much as possible. The switch model:

		1) buffers incoming/outgoing packets in the ports
		2) utilizes Links from EtherLink as the point-to-point connections
		in the switch fabric
		3) uses a simple least-recently-granted arbitration policy to forward packets,
		and doesn't provide any support for things like flow control. it also does
		not provide any features outside the link layer, which some switches may
		support e.g., ARP, filtering, etc.
	+
	+
	+
	+	junk

Diff:

Revision 3 (+577 -14)

Show changes

	src/dev/Ethernet.py
	src/dev/SConscript
	src/dev/etherint.hh
	src/dev/etherlink.hh
	src/dev/etherlink.cc
	src/dev/etherswitch.hh
	src/dev/etherswitch.cc

Ship It!

Andreas Hansson Jan. 5, 2015, 3:27 p.m. (Jan. 5, 2015, 3:27 p.m.)
```
What is the state of this?
```

Andreas Hansson Jan. 6, 2015, 4:50 a.m. (Jan. 6, 2015, 4:50 a.m.)

It might be worth adding the switch to a regression, either a new one, or as an intermediate switch in the existing ALPHA twosys regression.

Ship It!

src/dev/etherint.hh (Diff revision 3)

I don't understand why this needs to be virtual... I mean, I see that you override it on EtherSwitch::Interface, but part of the idea behind these port objects is that you only pay for the virtual function dispatch once.  I'm not sure why this broke down here, but I'd like to figure that out before we commit this code.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Testing Done:

Description:

Changeset 10243:d135e653e6eb

Changeset 10259:2d25a9f9969a

Diff:

Description:

Changeset 10259:2d25a9f9969a

Description:

Changeset 10259:2d25a9f9969a

Changeset 10259:eb23219d83a4

Diff:

Status: Closed (submitted)