mem: Fix SenderState related cache deadlock

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

Description

Changeset 9542:563280b8bbc2
---------------------------
mem: Fix SenderState related cache deadlock

This patch fixes a potential deadlock in the caches. This deadlock
could occur when more than one cache is used in a system, and
pkt->senderState is modified in between the two caches. This happened
as the caches relied on the senderState remaining unchanged, and used
it for instantaneous upstream communication with other caches.

This issue has been addressed by iterating over the linked list of
senderStates until we are either able to cast to a MSHR* or
senderState is NULL. If the cast is successful, we know that the
packet has previously passed through another cache, and therefore
update the downstreamPending flag accordingly. Otherwise, we do
nothing.

Testing Done

All regressions passing (excluding t1000 and eio)

Ship It!

Hi Andreas,

This looks OK to me.

One optional enhancement would be to package the loop into a function (maybe a static method on MSHR) that returns an MSHR* (or NULL), such that you could just do a drop-in replacement for the line
  MSHR *mshr = dynamic_cast<MSHR*>(pkt->senderState);
in both places it occurs with a single line like
  MSHR *mshr = findNextMSHR(pkt->senderState);

Steve Reinhardt Feb. 16, 2013, 3:25 a.m. (Feb. 16, 2013, 3:25 a.m.)

Thinking about this a little more after my other review, it's probably even better to make this a templated method on Packet, e.g..

   MSHR *mshr = pkt->findNextSenderState<MSHR>();

Then if someone ever decides to address that dangerous assumption in the Ruby sequencer code, they can use this code there, too...

If you feel like doing only one of the two, I think this method is more useful than the push/pop stuff I mentioned in the other review.

Description:

~		Changeset 9514:2da186a0f7da
	~	Changeset 9542:563280b8bbc2

		mem: Fix SenderState related cache deadlock

		This patch fixes a potential deadlock in the caches. This deadlock
		could occur when more than one cache is used in a system, and
		pkt->senderState is modified in between the two caches. This happened
		as the caches relied on the senderState remaining unchanged, and used
		it for instantaneous upstream communication with other caches.

		This issue has been addressed by iterating over the linked list of
		senderStates until we are either able to cast to a MSHR* or
		senderState is NULL. If the cast is successful, we know that the
		packet has previously passed through another cache, and therefore
		update the downstreamPending flag accordingly. Otherwise, we do
		nothing.

Diff:

Revision 2 (+30 -2)

Show changes

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

Thanks!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Description:

Changeset 9514:2da186a0f7da

Changeset 9542:563280b8bbc2

Diff:

Status: Closed (submitted)