DMA: Refactor the DMA device and align timing and atomic

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

Description

Changeset 9146:a02007211735
---------------------------
DMA: Refactor the DMA device and align timing and atomic

This patch does a bunch of house-keeping updates on the DMA, including
indentation, and formatting, but most importantly breaks out the
response handling such that it can be shared between the atomic and
timing modes. It also removes a potential bug caused by the atomic
handling of responses only deleting the allocated request (pkt->req)
once the DMA action completes instead of doing so for every packet.

Before this patch, the handling of responses was near identical for
atomic and timing, but the code was simply duplicated. With this
patch, the handleResp method deals with the responses in both cases.

There are further updates to make after removing the NACKs, but that
will be part of a separate follow-up patch. This patch does not change
the behaviour of any regression.

Testing Done

util/regress all passing (disregarding t1000 and eio)

Issue Summary

Description	From	Last Updated	Status
use uint	Ali Saidi	July 21, 2012, 3:33 a.m.	Open
I don't see where we use 'uint' anywhere else in the code, and I don't believe it's a standard typedef. ...	Steve Reinhardt	Aug. 21, 2012, 7:10 p.m.	Resolved
I see that you're relocating the panic message at the bottom of the function to a more sensible location, but ...	Steve Reinhardt	Aug. 21, 2012, 7:13 p.m.	Resolved
blame me all you like... I meant uint32_t or whatever explicit type ;)... don't care that much though	Ali Saidi	Aug. 21, 2012, 7:13 p.m.	Resolved

haven't looked in detail, but isn't only a single request created for all packets that isn't ref counted? how can you delete it every time then?

Andreas Hansson July 21, 2012, 4:39 a.m. (July 21, 2012, 4:39 a.m.)

For timing it called delete pkt->req and put for every single response, and that also corresponds with how they are created. There is a single sender state for all of them, but every packet has a unique request.

For atomic, however, delete pkt->req was called along with delete state, and only done once the DMA action completes. This is wrong as far as I can tell, but if I got something wrong then please do let me know.

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

use uint

Show all issues

Andreas Hansson July 21, 2012, 4:39 a.m. (July 21, 2012, 4:39 a.m.)
```
Will do.
```

Ship It!

Description:

~		Changeset 9133:b6a0e6f32336
	~	Changeset 9133:beb437b45bd0

		DMA: Refactor the DMA device and align timing and atomic

		This patch does a bunch of house-keeping updates on the DMA, including
		indentation, and formatting, but most importantly breaks out the
		response handling such that it can be shared between the atomic and
		timing modes. It also removes a potential bug caused by the atomic
		handling of responses only deleting the allocated request (pkt->req)
		once the DMA action completes instead of doing so for every packet.

		Before this patch, the handling of responses was near identical for
		atomic and timing, but the code was simply duplicated. With this
		patch, the handleResp method deals with the responses in both cases.

		There are further updates to make after removing the NACKs, but that
		will be part of a separate follow-up patch. This patch does not change
		the behaviour of any regression.

Diff:

Revision 2 (+127 -127)

Show changes

	src/dev/Device.py
	src/dev/dma_device.hh
	src/dev/dma_device.cc

Description:

~		Changeset 9133:beb437b45bd0
	~	Changeset 9146:a02007211735

		DMA: Refactor the DMA device and align timing and atomic

		This patch does a bunch of house-keeping updates on the DMA, including
		indentation, and formatting, but most importantly breaks out the
		response handling such that it can be shared between the atomic and
		timing modes. It also removes a potential bug caused by the atomic
		handling of responses only deleting the allocated request (pkt->req)
		once the DMA action completes instead of doing so for every packet.

		Before this patch, the handling of responses was near identical for
		atomic and timing, but the code was simply duplicated. With this
		patch, the handleResp method deals with the responses in both cases.

		There are further updates to make after removing the NACKs, but that
		will be part of a separate follow-up patch. This patch does not change
		the behaviour of any regression.

Diff:

Revision 3 (+102 -107)

Show changes

	src/dev/dma_device.hh
	src/dev/dma_device.cc

Ship It!

Nice!  I really like patches that result in a net reduction of non-comment LOC...

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

Why the switch from list to deque?  Not that I have much preference, just wondering why it matters enough to change.

Andreas Hansson Aug. 6, 2012, 6:38 p.m. (Aug. 6, 2012, 6:38 p.m.)

Probably not measurable. It is merely to make it clear that we only push at the back and pop the front. There is never insertion in the middle.

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

I don't see where we use 'uint' anywhere else in the code, and I don't believe it's a standard typedef.  If signedness really matters, use 'unsigned' or 'unsigned int' (we seem to use the former predominantly, and the latter just occasionally).

Show all issues

Andreas Hansson Aug. 6, 2012, 5:41 p.m. (Aug. 6, 2012, 5:41 p.m.)

I blame Ali, originally it was unsigned int and he suggested uint. I will revert that change if everyone agrees :)

src/dev/dma_device.cc (Diff revision 3)

I see that you're relocating the panic message at the bottom of the function to a more sensible location, but the whole check seems redundant wrt the assert(state) below... if senderState is null, it will pass through dynamic_cast unmodified and we'll catch it there.  Thus I'd be in favor of just getting rid of this statement as effectively being an assertion that won't get compiled out with gem5.fast.

Show all issues

src/dev/dma_device.cc (Diff revision 3)

Is the distinction about whether we use the size in the request or the packet significant?  Shouldn't they be the same?  This comment makes it sound like that distinction is meaningful...

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

blame me all you like... I meant uint32_t or whatever explicit type ;)... don't care that much though

Show all issues

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 9133:b6a0e6f32336

Changeset 9133:beb437b45bd0

Diff:

Description:

Changeset 9133:beb437b45bd0

Changeset 9146:a02007211735

Diff:

Status: Closed (submitted)