mem: Clean up packet data allocation

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

Description

Changeset 10572:ca0ba71c37f9
---------------------------
mem: Clean up packet data allocation

This patch attempts to make the rules for data allocation in the
packet explicit, understandable, and easy to verify. The constructor
that copies a packet is extended with an additional flag "alloc_data"
to enable the call site to explicitly say whether the newly created
packet is short-lived (a zero-time snoop), or has an unknown life-time
and therefore should allocate its own data (or copy a static pointer
in the case of static data).

The tricky case is the static data. In essence this is a
copy-avoidance scheme where the original source of the request (DMA,
CPU etc) does not ask the memory system to return data as part of the
packet, but instead provides a pointer, and then the memory system
carries this pointer around, and copies the appropriate data to the
location itself. Thus any derived packet actually never copies any
data. As the original source does not copy any data from the response
packet when arriving back at the source, we must maintain the copy of
the original pointer to not break the system. We might want to revisit
this one day and pay the price for a few extra memcpy invocations.

All in all this patch should make it easier to grok what is going on
in the memory system and how data is actually copied (or not).

Testing Done

Issue Summary

Description	From	Last Updated	Status
We need better explanation here. What do you mean by "nature of copying static pointers into forwarded packets"?	Nilay Vaish	Nov. 26, 2014, 2:05 a.m.	Resolved

It would be good to get this patch in with the other packet fix ups. Make sure you've had your morning coffee before giving it a go.

src/mem/packet.hh (Diff revision 1)

We don't need this code.

src/mem/packet.hh (Diff revision 1)

We need better explanation here.  What do you mean by "nature of copying static pointers into forwarded packets"?

Show all issues

Andreas Hansson Nov. 26, 2014, 2:06 a.m. (Nov. 26, 2014, 2:06 a.m.)

I have added far more elaborate comments to dataStatic and dataDynamic to explain what is going on.

Description:

~		Changeset 10549:f25a54e256d8
	~	Changeset 10572:ca0ba71c37f9

		mem: Clean up packet data allocation

		This patch attempts to make the rules for data allocation in the
		packet explicit, understandable, and easy to verify. The constructor
		that copies a packet is extended with an additional flag "alloc_data"
		to enable the call site to explicitly say whether the newly created
		packet is short-lived (a zero-time snoop), or has an unknown life-time
		and therefore should allocate its own data (or copy a static pointer
		in the case of static data).

		The tricky case is the static data. In essence this is a
		copy-avoidance scheme where the original source of the request (DMA,
		CPU etc) does not ask the memory system to return data as part of the
		packet, but instead provides a pointer, and then the memory system
		carries this pointer around, and copies the appropriate data to the
		location itself. Thus any derived packet actually never copies any
		data. As the original source does not copy any data from the response
		packet when arriving back at the source, we must maintain the copy of
		the original pointer to not break the system. We might want to revisit
		this one day and pay the price for a few extra memcpy invocations.

		All in all this patch should make it easier to grok what is going on
		in the memory system and how data is actually copied (or not).

Diff:

Revision 2 (+72 -23)

Show changes

	src/mem/packet.hh
	src/mem/cache/cache_impl.hh
	src/mem/cache/mshr.cc

Just minor comment-related stuff, no need to repost.

src/mem/cache/mshr.cc (Diff revision 2)

shouldn't this comment go above the constructor call?

src/mem/packet.hh (Diff revision 2)

'whenever'

src/mem/packet.hh (Diff revision 2)

'allocate'

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 10549:f25a54e256d8

Changeset 10572:ca0ba71c37f9

Diff:

Status: Closed (submitted)