mem: Use the packet delays and do not just zero them out

Information
Submitter:	Andreas Hansson
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11191:d5063101794c
---------------------------
mem: Use the packet delays and do not just zero them out

This patch updates the I/O devices, bridge and simple memory to take
the packet header and payload delay into account in their latency
calculations. In all cases we add the header delay, i.e. the
accumulated pipeline delay of any crossbars, and the payload delay
needed for deserialisation of any payload.

Due to the additional unknown latency contribution, the packet queue
of the simple memory is changed to use insertion sorting based on the
time stamp. Moreover, since the memory hands out exclusive (non
shared) responses, we also need to ensure ordering for reads to the
same address.

Testing Done

Issue Summary

Description	From	Last Updated	Status
minor, but 'additional_latency' is rather nondescript. maybe 'transmission_delay' or 'receive_delay' or 'receive_latency' or something like thet?	Steve Reinhardt	Nov. 4, 2015, 1:10 a.m.	Resolved
why ticksToCycles()? aren't you adding a cycle value to a tick value?	Steve Reinhardt	Nov. 4, 2015, 1:11 a.m.	Resolved
likewise	Steve Reinhardt	Nov. 4, 2015, 1:11 a.m.	Resolved

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

minor, but 'additional_latency' is rather nondescript. maybe 'transmission_delay' or 'receive_delay' or 'receive_latency' or something like thet?

Show all issues

Andreas Hansson Nov. 3, 2015, 11:51 p.m. (Nov. 3, 2015, 11:51 p.m.)

It is headerDelay (pipeline delay) and payloadDelay (de-serialisation delay). I am happy to change, but I don't think there is a short and snappy word describing it.

Steve Reinhardt Nov. 4, 2015, 5:36 p.m. (Nov. 4, 2015, 5:36 p.m.)

I see you changed it anyway, thanks. 'receive_delay' may not be perfectly descriptive, but it's an improvement I think.

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

out of scope for now, but just a thought: arguably this zeroing of the delay fields should probably take place in Packet::makeResponse()

Andreas Hansson Nov. 3, 2015, 11:54 p.m. (Nov. 3, 2015, 11:54 p.m.)

No. It relly should never implicitly be zeroed.

If we re-use the packet without paying for the latency on the request path, we rely on it being paid for on the response path.

Steve Reinhardt Nov. 4, 2015, 5:36 p.m. (Nov. 4, 2015, 5:36 p.m.)
```
ok, makes sense
```

src/mem/bridge.cc (Diff revision 1)

why ticksToCycles()?  aren't you adding a cycle value to a tick value?

Show all issues

Andreas Hansson Nov. 3, 2015, 11:54 p.m. (Nov. 3, 2015, 11:54 p.m.)
```
My bad. Will fix.
```

src/mem/bridge.cc (Diff revision 1)

likewise

Show all issues

src/mem/simple_mem.cc (Diff revision 1)

this code looks awfully familiar :). is there a way to use PacketQueue here to avoid the redundancy?

Andreas Hansson Nov. 3, 2015, 11:54 p.m. (Nov. 3, 2015, 11:54 p.m.)

It was historically using the packet queue. I The main reason for moving away from it is to be able to bound the number of outstanding responses. Till now that hasn't happened, but I am inclined to not use the SimpleTimingPort here.

Steve Reinhardt Nov. 4, 2015, 5:36 p.m. (Nov. 4, 2015, 5:36 p.m.)

ok. would be nice to factor out the common code somehow, but it's a small enough thing that the redundancy is tolerable I suppose.

Description:

~		Changeset 11188:353cf88682b9
	~	Changeset 11191:d5063101794c

		mem: Use the packet delays and do not just zero them out

		This patch updates the I/O devices, bridge and simple memory to take
		the packet header and payload delay into account in their latency
		calculations. In all cases we add the header delay, i.e. the
		accumulated pipeline delay of any crossbars, and the payload delay
		needed for deserialisation of any payload.

		Due to the additional unknown latency contribution, the packet queue
		of the simple memory is changed to use insertion sorting based on the
		time stamp. Moreover, since the memory hands out exclusive (non
		shared) responses, we also need to ensure ordering for reads to the
		same address.

Diff:

Revision 2 (+52 -18)

Show changes

	src/dev/io_device.cc
	src/dev/pcidev.cc
	src/mem/bridge.cc
	src/mem/simple_mem.hh
	src/mem/simple_mem.cc

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 11188:353cf88682b9

Changeset 11191:d5063101794c

Diff:

Status: Closed (submitted)