dev: Add 'simLength' parameter in EthPacketData

Information
Submitter:	Michael LeBeane
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11637:cd6bb67002fa
---------------------------
dev: Add 'simLength' parameter in EthPacketData
Currently, all the network devices create a 16K buffer for the 'data' field
in EthPacketData, and use 'length' to keep track of the size of the packet
in the buffer. This patch introduces the 'simLength' parameter to
EthPacketData, which is used to hold the effective length of the packet used
for all timing calulations in the simulator. Serialization is performed using
only the useful data in the packet ('length') and not necessarily the entire
original buffer.

Testing Done

Issue Summary

Description	From	Last Updated	Status
Could you use 'dataLength' instead of 'bufferLength' here to serialize only the useful data?	Gabor Dozsa	June 29, 2016, 11:05 a.m.	Resolved
If you serialize the useful data only then you could use dataLength here, too. In that case, adding an assertion ...	Gabor Dozsa	June 29, 2016, 11:05 a.m.	Dropped
There is an implicit assumption that the EthPacketData object is "empty" (i.e. data==nullptr) at this point. It may be good ...	Gabor Dozsa	June 29, 2016, 10:59 a.m.	Resolved
I really don't like the way this is breaking checkpoints. The right thing to do here would be to implement ...	Andreas Sandberg	Oct. 12, 2016, 8:47 a.m.	Resolved

Thank you for the patch! It looks good, I have a few minor comments only.

src/dev/net/etherpkt.cc (Diff revision 1)

Could you use 'dataLength' instead of 'bufferLength' here to serialize only the useful data?

Show all issues

src/dev/net/etherpkt.cc (Diff revision 1)

If you serialize the useful data only then you could use dataLength here, too. In that case, adding an assertion to make sure dataLength <= bufferLength would be good, too.

Show all issues

Michael LeBeane June 29, 2016, 11:28 a.m. (June 29, 2016, 11:28 a.m.)

I removed the bufferLength field. Its only purpose was for serialization, which is no longer necessary after implementing your previous comment.

src/dev/net/etherpkt.cc (Diff revision 1)

There is an implicit assumption that the EthPacketData object is "empty" (i.e. data==nullptr) at this point. It may be good to add an assertion to check that exlicitly.

Show all issues

Change Summary:

Address review comments and reapply on tip.

Description:

~		Changeset 11479:7d5bb81e87de
~
~		dev: Redefine 'length' in EthPacketData
	~	Changeset 11549:573a7d349538
	~	---------------------------
	~	dev: Redefine 'length' in EthPacketData
		Currently, all the network devices create a 16K buffer for the 'data' field
		in EthPacketData, and use 'length' to keep track of the size of the packet
~		in the buffer. This patch introduces 'bufferLength,' 'dataLength,' and
~		'simLength' parameters to EthPacketData. The size of the 'data' buffer is
~		explicitly stored in 'bufferLength', which allows the packet to
~		serialize/unserialize without passing in the buffer size. 'dataLength' stores
~		the amount of space taken up by the packet in the 'data' buffer. Finally,
~		'simLength' is used to hold the effective length of the packet used for all
	~	in the buffer. This patch introduces 'dataLength' and 'simLength' parameters
	~	to EthPacketData. 'dataLength' stores the amount of space taken up by the
	~	packet in the 'data' buffer. 'simLength' is used to hold the effective length
	~	of the packet used for all timing calulations in the simulator. Serialization
	~	is performed using only the useful data in the packet ('dataLength') and not
	~	necessarily the entire original buffer.
-		timing calulations in the simulator.

Diff:

Revision 2 (+163 -138)

Show changes

	src/dev/net/dist_etherlink.cc
	src/dev/net/dist_iface.cc
	src/dev/net/dist_packet.hh
	src/dev/net/etherbus.cc
	src/dev/net/etherdump.cc
	src/dev/net/etherlink.cc
	src/dev/net/etherpkt.hh
	src/dev/net/etherpkt.cc
	8 more

It looks good! Thank you for the fixes!

Any more comments on this? We would like to get a few more ship it's since this fairly large and will break checkpoints.

Andreas Hansson Aug. 31, 2016, 11:58 p.m. (Aug. 31, 2016, 11:58 p.m.)

If it breaks checkpoints there should be an update function added, or am I missing something?

Michael LeBeane Sept. 21, 2016, 10:26 a.m. (Sept. 21, 2016, 10:26 a.m.)

Hmmm, I could add an update function but I don't have any checkpoints to test it with.  Does anyone have any checkpoints out there with serialized packets?  Or are there some easy networking workloads that could be used to generate some test checkpoints?

Michael LeBeane Oct. 3, 2016, 8:42 a.m. (Oct. 3, 2016, 8:42 a.m.)

We plan on shipping this in a week unless someone can provide a workload that create serialized network packets. Thanks!

src/dev/net/etherpkt.cc (Diff revision 2)

I really don't like the way this is breaking checkpoints.
The right thing to do here would be to implement this in a way that doesn't break checkpoint. The simplest way would probably be to use length as the name for dataLength. If simLength is present (I'm going to push a helper function to test for that shortly), you just unserialize the value. If not, you use a fallback such as simLength = dataLength.
The other option is to provide a checkpoint updater, which is pretty tricky since you need to find all places where packets are serialized.

Show all issues

Michael LeBeane Oct. 10, 2016, 12:14 p.m. (Oct. 10, 2016, 12:14 p.m.)

The first option sounds reasonable.  Did you push the helper function that tests if something is present in a checkpoint?  I don't think I've seen it in the commit logs recently.

Andreas Sandberg Oct. 10, 2016, 1:36 p.m. (Oct. 10, 2016, 1:36 p.m.)

I committed the helper functions last week (https://github.com/gem5/gem5/commit/18135ce6abc0ee02e36aef424be183cd7238a55). I thought a bit more about this and the optParamIn helper probably implements the functionality you need.

Michael LeBeane Oct. 18, 2016, 12:22 p.m. (Oct. 18, 2016, 12:22 p.m.)

I reposted using your suggestions. Does this work for you? Thanks!

Summary:

-	dev: Redefine 'length' in EthPacketData
+	dev: Add 'simLength' parameter in EthPacketData

Description:

~		Changeset 11549:573a7d349538
	~	Changeset 11637:cd6bb67002fa
		---------------------------
~		dev: Redefine 'length' in EthPacketData
	~	dev: Add 'simLength' parameter in EthPacketData
		Currently, all the network devices create a 16K buffer for the 'data' field
		in EthPacketData, and use 'length' to keep track of the size of the packet
~		in the buffer. This patch introduces 'dataLength' and 'simLength' parameters
~		to EthPacketData. 'dataLength' stores the amount of space taken up by the
~		packet in the 'data' buffer. 'simLength' is used to hold the effective length
~		of the packet used for all timing calulations in the simulator. Serialization
~		is performed using only the useful data in the packet ('dataLength') and not
	~	in the buffer. This patch introduces the 'simLength' parameter to
	~	EthPacketData, which is used to hold the effective length of the packet used
	~	for all timing calulations in the simulator. Serialization is performed using
	~	only the useful data in the packet ('length') and not necessarily the entire
	~	original buffer.
-		necessarily the entire original buffer.

Diff:

Revision 3 (+53 -28)

Show changes

	src/dev/net/dist_etherlink.cc
	src/dev/net/dist_iface.cc
	src/dev/net/dist_packet.hh
	src/dev/net/etherbus.cc
	src/dev/net/etherlink.cc
	src/dev/net/etherpkt.hh
	src/dev/net/etherpkt.cc
	src/dev/net/etherswitch.cc
	6 more

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Change Summary:

Description:

Changeset 11479:7d5bb81e87de

Diff:

Summary:

Description:

Diff:

Status: Closed (submitted)