Bus: Replace tickNextIdle and inRetry with a state variable

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

Description

Changeset 9079:cb20403fc3be
---------------------------
Bus: Replace tickNextIdle and inRetry with a state variable

This patch adds a state enum and member variable in the bus, tracking
the bus state, thus eliminating the need for tickNextIdle and inRetry,
and fixing an issue that allowed the bus to be occupied by multiple
packets at once (hopefully it also makes it easier to understand the
code).

The bus, in its current form, uses tickNextIdle and inRetry to keep
track of the state of the bus. However, it only updates tickNextIdle
_after_ forwarding a packet using sendTiming, and the result is that
the bus is still seen as idle, and a module that receives the packet
and starts transmitting new packets in zero time will still see the
bus as idle (and this is done by a number of DMA devices). The issue
can also be seen in isOccupied where the bus calls reschedule on an
event instead of schedule.

This patch addresses the problem by marking the bus as _not_ idle
already by the time we conclude that the bus is not occupied and we
will deal with the packet.

As a result of not allowing multiple packets to occupy the bus, some
regressions have slight changes in their statistics. A separate patch
updates these accordingly.

Further ahead, a follow-on patch will introduce a separate state
variable for request/responses/snoop responses, and thus implement a
split request/response bus with separate flow control for the
different message types (even further ahead it will introduce a
multi-layer bus).

Testing Done

util/regress all passing (disregarding t1000 and eio) with a few exception that exhibit stat differences, e.g. 10.linux-boot/alpha/linux/tsunami-simple-timing with a magnitude of <0.4%, fs/10.linux-boot/arm/linux/realview-simple-timing with <0.01%, and 10.linux-boot/x86/linux/pc-simple-timing with a max magnitude of +38%(!)

Issue Summary

Description	From	Last Updated	Status
I strongly suggest that either a. the name of function isOccupied() be changed, or b. the code be organised in ...	Nilay Vaish	June 21, 2012, 1:14 a.m.	Resolved
This comment does not seem correct. In succeededTiming() there is an assert that state should be BUSY.	Nilay Vaish	June 20, 2012, 8:20 a.m.	Dropped
From the implementation of recvRetry() below, it seems that this check is not required.	Nilay Vaish	June 20, 2012, 8:20 a.m.	Dropped
You might want to remove the modulo operation here. You can replace the entire if block by -- now = ...	Nilay Vaish	June 20, 2012, 8:20 a.m.	Resolved
It is right. in succeededTiming before the assert state == BUSY there is also an if block if (state == ...	Andreas Hansson	June 19, 2012, 11:41 p.m.	Open
It is required to know if the neighbouring module calls sendTiming in 0 time and we end up in "our ...	Andreas Hansson	June 19, 2012, 11:41 p.m.	Open
Done!	Andreas Hansson	June 19, 2012, 11:41 p.m.	Open
This assert also seems unnecessary. Whenever occupyBus() is called, either the state was set to or was asserted to be ...	Nilay Vaish	June 21, 2012, 1:14 a.m.	Dropped
I am guessing the check here on state is also due to some future use.	Nilay Vaish	June 21, 2012, 12:18 a.m.	Resolved
It can be removed. It used to be a separate if rather than else if. I'll prune it.	Andreas Hansson	June 27, 2012, 9:43 p.m.	Resolved
I think tryTiming() goes well with succeededTiming()/failedTiming()	Steve Reinhardt	June 28, 2012, 2:46 a.m.	Resolved
There's a divCeil() function in src/base/intmath.hh that can replace the first subexpression in this equation. I thought we had some ...	Steve Reinhardt	June 28, 2012, 2:48 a.m.	Resolved
Same comment here. We really should have a roundToNextMultiple() method, or better yet some cleaner way of dealing with clocks ...	Steve Reinhardt	June 28, 2012, 2:48 a.m.	Resolved
The logic here is just a bit confusing... in fact you don't want/need to call either failedTiming() or succeededTiming() on ...	Steve Reinhardt	June 28, 2012, 2:51 a.m.	Resolved

Ship It!

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

I strongly suggest that either 
a. the name of function isOccupied() be changed, or
b. the code be organised in a different fashion.

As is the normal practice, a function named isOccupied() should not change anything at all.

Show all issues

Andreas Hansson June 18, 2012, 11:58 p.m. (June 18, 2012, 11:58 p.m.)

The name is the same for "historic" reasons, but I'm happy to change it. Any suggestions? updateAvailable?

Andreas Hansson June 20, 2012, 9:35 a.m. (June 20, 2012, 9:35 a.m.)
```
Any suggestions on the name?
```

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

This comment does not seem correct. In succeededTiming() there is an assert that state should be BUSY.

Show all issues

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

From the implementation of recvRetry() below, it seems that this check is not required.

Show all issues

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

You might want to remove the modulo operation here. You can replace the entire if block by --
now = ((now + clock - 1) / clock) * clock;

Show all issues

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

It is right. in succeededTiming before the assert state == BUSY there is also an if block if (state == RETRY) :)

Show all issues

Nilay Vaish June 20, 2012, 12:22 a.m. (June 20, 2012, 12:22 a.m.)

But occupyBus() is the last statement in the function succeededTiming().
So it really does not matter to succeededTiming() what the state is
after it calls occupyBus().

Andreas Hansson June 20, 2012, 12:24 a.m. (June 20, 2012, 12:24 a.m.)

succeededTiming needs to know if it was part of a retry such that it can take the port of the retrylist before going to busy

Nilay Vaish June 20, 2012, 8:12 a.m. (June 20, 2012, 8:12 a.m.)

May be I need to put thing down in a better fashion. Let me
ask you a series of questions --

1) Is the call to occupyBus() the last statement in succeededTiming()? Would this not mean that 
succeededTiming() will never get to see what occupyBus() did?
2) Is there a code path through which occupyBus() can get called with
state == RETRY?

As per my understanding of the code, 'yes' is the answer to both parts of the first question,
and 'no' is the answer to the second question. This would mean that the comment in question
would be incorrect on two different counts.

Andreas Hansson June 20, 2012, 8:25 a.m. (June 20, 2012, 8:25 a.m.)

occupyBus is the last part of succeedTiming, but it is the start of succeedTiming that need to know that we came from RETRY. Makes sense?

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

It is required to know if the neighbouring module calls sendTiming in 0 time and we end up in "our own" recvTiming.

Show all issues

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

Done!

Show all issues

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

This assert also seems unnecessary. Whenever occupyBus() is called, either the state was set to or was asserted to be BUSY before the call.

Show all issues

Andreas Hansson June 20, 2012, 6:14 a.m. (June 20, 2012, 6:14 a.m.)

Aren't all asserts unnecessary in that case? :) It was added as part of the development process, and I am more than happy to remove it...but it might still be useful in the future.

Nilay Vaish June 20, 2012, 8:12 a.m. (June 20, 2012, 8:12 a.m.)

Effectively, what you have done over here is --

a = 5;
assert(a == 5);

Are we trying to test if there is a some kind transient fault, or memory
corruption. Why not have such asserts all over the place?

If need be, the assert can be added in future.

Andreas Hansson June 20, 2012, 8:23 a.m. (June 20, 2012, 8:23 a.m.)

There might be cases where occupyBus is not called from failedTiming or succeedTiming, and it is just kept "in case". If you really do not like to have this assert there I am happy to remove it...it won't do any harm though, so I do not see why we won't keep it. Reasonable?

Andreas Hansson June 27, 2012, 9:44 p.m. (June 27, 2012, 9:44 p.m.)
```
You happy with the new revision Nilay?
```

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

I am guessing the check here on state is also due to some future use.

Show all issues

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

It can be removed. It used to be a separate if rather than else if. I'll prune it.

Show all issues

Description:

~		Changeset 9071:e6145aa18228
	~	Changeset 9077:b0828925816c

		Bus: Replace tickNextIdle and inRetry with a state variable

		This patch adds a state enum and member variable in the bus, tracking
		the bus state, thus eliminating the need for tickNextIdle and inRetry,
		and fixing an issue that allowed the bus to be occupied by multiple
		packets at once (hopefully it also makes it easier to understand the
		code).

		The bus, in its current form, uses tickNextIdle and inRetry to keep
		track of the state of the bus. However, it only updates tickNextIdle
		after forwarding a packet using sendTiming, and the result is that
		the bus is still seen as idle, and a module that receives the packet
		and starts transmitting new packets in zero time will still see the
		bus as idle (and this is done by a number of DMA devices). The issue
		can also be seen in isOccupied where the bus calls reschedule on an
		event instead of schedule.

		This patch addresses the problem by marking the bus as not idle
		already by the time we conclude that the bus is not occupied and we
		will deal with the packet.

		As a result of not allowing multiple packets to occupy the bus, some
		regressions have slight changes in their statistics. A separate patch
		updates these accordingly.

		Further ahead, a follow-on patch will introduce a separate state
		variable for request/responses/snoop responses, and thus implement a
		split request/response bus with separate flow control for the
		different message types (even further ahead it will introduce a
		multi-layer bus).

Diff:

Revision 2 (+143 -104)

Show changes

	src/mem/bus.hh
	src/mem/bus.cc
	src/mem/coherent_bus.cc
	src/mem/noncoherent_bus.cc

Thanks!  Overall this is much clearer IMO.  Sorry for the nit-picking.

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

I think tryTiming() goes well with succeededTiming()/failedTiming()

Show all issues

src/mem/bus.cc (Diff revision 2)

There's a divCeil() function in src/base/intmath.hh that can replace the first subexpression in this equation.  I thought we had some utility functions for clocks that could replace this whole expression (didn't Nate do some work on this?) but I can't find them.

Show all issues

src/mem/bus.cc (Diff revision 2)

Same comment here.  We really should have a roundToNextMultiple() method, or better yet some cleaner way of dealing with clocks (again, I thought Nate had done some work on this).  For now just using divCeil() is fine though.

Show all issues

src/mem/coherent_bus.cc (Diff revision 2)

The logic here is just a bit confusing... in fact you don't want/need to call either failedTiming() or succeededTiming() on express snoops, but no explicit check is needed for the former because they can't fail.  It would be nice for the control flow to be a little more symmetric.  How about:

if (!is_express_snoop) {
    if (success) {
        succeededTiming(...);
    } else {
        // existing fail case code
    }
}

or if you don't like the extra indentation:

if (is_express_snoop) {
    assert(success);
    return true;
}

if (success) {
    succeededTiming(...);
} else {
    // existing fail case code
}

in the latter version, you could even factor the return statements out and just put a "return success;" at the end.

Show all issues

Description:

~		Changeset 9077:b0828925816c
	~	Changeset 9079:cb20403fc3be

		Bus: Replace tickNextIdle and inRetry with a state variable

		This patch adds a state enum and member variable in the bus, tracking
		the bus state, thus eliminating the need for tickNextIdle and inRetry,
		and fixing an issue that allowed the bus to be occupied by multiple
		packets at once (hopefully it also makes it easier to understand the
		code).

		The bus, in its current form, uses tickNextIdle and inRetry to keep
		track of the state of the bus. However, it only updates tickNextIdle
		after forwarding a packet using sendTiming, and the result is that
		the bus is still seen as idle, and a module that receives the packet
		and starts transmitting new packets in zero time will still see the
		bus as idle (and this is done by a number of DMA devices). The issue
		can also be seen in isOccupied where the bus calls reschedule on an
		event instead of schedule.

		This patch addresses the problem by marking the bus as not idle
		already by the time we conclude that the bus is not occupied and we
		will deal with the packet.

		As a result of not allowing multiple packets to occupy the bus, some
		regressions have slight changes in their statistics. A separate patch
		updates these accordingly.

		Further ahead, a follow-on patch will introduce a separate state
		variable for request/responses/snoop responses, and thus implement a
		split request/response bus with separate flow control for the
		different message types (even further ahead it will introduce a
		multi-layer bus).

Diff:

Revision 3 (+163 -119)

Show changes

	src/base/intmath.hh
	src/mem/bus.hh
	src/mem/bus.cc
	src/mem/coherent_bus.cc
	src/mem/noncoherent_bus.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 9071:e6145aa18228

Changeset 9077:b0828925816c

Diff:

Description:

Changeset 9077:b0828925816c

Changeset 9079:cb20403fc3be

Diff:

Status: Closed (submitted)