Discussion:
[gem5-dev] Review Request 3251: mem: Do not use sender state to track forwarded snoops in cache
(too old to reply)
Andreas Hansson
2015-12-09 23:51:39 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3251/
-----------------------------------------------------------

Review request for Default.


Repository: gem5


Description
-------

Changeset 11259:8d9c05513c3c
---------------------------
mem: Do not use sender state to track forwarded snoops in cache

This patch changes how the cache tracks which snoops are forwarded,
and which ones are created locally. Previously the identification was
based on an empty sender state of a specific class, but this method
fails to distinguish which cache actually attached the sender
state. Instead we use the same mechanism as the crossbar, and keep
track of the requests that have outstanding snoops.


Diffs
-----

src/mem/cache/cache.hh 8ed230a0b28c
src/mem/cache/cache.cc 8ed230a0b28c

Diff: http://reviews.gem5.org/r/3251/diff/


Testing
-------


Thanks,

Andreas Hansson
Tony Gutierrez
2015-12-17 23:15:06 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3251/#review7731
-----------------------------------------------------------

Ship it!


Ship It!

- Tony Gutierrez
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3251/
-----------------------------------------------------------
(Updated Dec. 9, 2015, 3:51 p.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11259:8d9c05513c3c
---------------------------
mem: Do not use sender state to track forwarded snoops in cache
This patch changes how the cache tracks which snoops are forwarded,
and which ones are created locally. Previously the identification was
based on an empty sender state of a specific class, but this method
fails to distinguish which cache actually attached the sender
state. Instead we use the same mechanism as the crossbar, and keep
track of the requests that have outstanding snoops.
Diffs
-----
src/mem/cache/cache.hh 8ed230a0b28c
src/mem/cache/cache.cc 8ed230a0b28c
Diff: http://reviews.gem5.org/r/3251/diff/
Testing
-------
Thanks,
Andreas Hansson
Steve Reinhardt
2015-12-24 00:23:40 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3251/#review7742
-----------------------------------------------------------


Doesn't the current mechanism work because a new ForwardResponseRecord would be created at each level of forwarding, and then peeled off at each level of response? Not saying this is robust, just curious if there's an actual bug here or just a concern.

Another alternative would be to add a cache pointer to ForwardResponseRecord, then the receiver can check/verify that the contained pointer points to 'this' before proceeding.


src/mem/cache/cache.cc
<http://reviews.gem5.org/r/3251/#comment6674>

how does this change relate to snoop forwarding?


- Steve Reinhardt
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3251/
-----------------------------------------------------------
(Updated Dec. 9, 2015, 3:51 p.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11259:8d9c05513c3c
---------------------------
mem: Do not use sender state to track forwarded snoops in cache
This patch changes how the cache tracks which snoops are forwarded,
and which ones are created locally. Previously the identification was
based on an empty sender state of a specific class, but this method
fails to distinguish which cache actually attached the sender
state. Instead we use the same mechanism as the crossbar, and keep
track of the requests that have outstanding snoops.
Diffs
-----
src/mem/cache/cache.hh 8ed230a0b28c
src/mem/cache/cache.cc 8ed230a0b28c
Diff: http://reviews.gem5.org/r/3251/diff/
Testing
-------
Thanks,
Andreas Hansson
Andreas Hansson
2015-12-24 08:16:48 UTC
Permalink
Post by Andreas Hansson
Post by Steve Reinhardt
Doesn't the current mechanism work because a new ForwardResponseRecord would be created at each level of forwarding, and then peeled off at each level of response? Not saying this is robust, just curious if there's an actual bug here or just a concern.
Another alternative would be to add a cache pointer to ForwardResponseRecord, then the receiver can check/verify that the contained pointer points to 'this' before proceeding.
There should not be a correctness issue here, merely a clunky mechanism replaced with a more intuitive one imho.
Post by Andreas Hansson
Post by Steve Reinhardt
src/mem/cache/cache.cc, line 2382
<http://reviews.gem5.org/r/3251/diff/1/?file=52203#file52203line2382>
how does this change relate to snoop forwarding?
My bad. I'll move this fix to a separate patch.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3251/#review7742
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3251/
-----------------------------------------------------------
(Updated Dec. 9, 2015, 11:51 p.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11259:8d9c05513c3c
---------------------------
mem: Do not use sender state to track forwarded snoops in cache
This patch changes how the cache tracks which snoops are forwarded,
and which ones are created locally. Previously the identification was
based on an empty sender state of a specific class, but this method
fails to distinguish which cache actually attached the sender
state. Instead we use the same mechanism as the crossbar, and keep
track of the requests that have outstanding snoops.
Diffs
-----
src/mem/cache/cache.hh 8ed230a0b28c
src/mem/cache/cache.cc 8ed230a0b28c
Diff: http://reviews.gem5.org/r/3251/diff/
Testing
-------
Thanks,
Andreas Hansson
Andreas Hansson
2015-12-24 08:22:17 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3251/
-----------------------------------------------------------

(Updated Dec. 24, 2015, 8:22 a.m.)


Review request for Default.


Repository: gem5


Description (updated)
-------

Changeset 11277:b0171dac1864
---------------------------
mem: Do not use sender state to track forwarded snoops in cache

This patch changes how the cache tracks which snoops are forwarded,
and which ones are created locally. Previously the identification was
based on an empty sender state of a specific class, but this method
fails to distinguish which cache actually attached the sender
state. Instead we use the same mechanism as the crossbar, and keep
track of the requests that have outstanding snoops.


Diffs (updated)
-----

src/mem/cache/cache.hh d9a0136ab8cc
src/mem/cache/cache.cc d9a0136ab8cc

Diff: http://reviews.gem5.org/r/3251/diff/


Testing
-------


Thanks,

Andreas Hansson
Steve Reinhardt
2015-12-28 20:27:29 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3251/#review7779
-----------------------------------------------------------


Hey, I posted some comments but never gave this a ship it... I was going to say that neither solution (previous or current) seems all that great to me. Seems like if we really just want to know whether this request was issued by this cache or not, we ought to be able to do something straightforward like check req->masterId(), no?

Not that I'm a total purist or anything (and apparently I missed you doing the same trick elsewhere), but I'm not totally comfortable with using RequestPtr as a transaction ID or an STL set to model anything other than a fully associative hardware table.

FYI, the idea with SenderState is that it's really a shortcut for a master's outstanding transaction table, where normally you'd allocate a table entry and put a table index in the transaction as a tag, then do a table lookup based on that tag as a response. I thought this was discussed in the SenderState comment, but I see that it's not, unfortunately. The dynamic_cast is an abuse of this model as well, which is why I'm not defending the status quo here, just hoping that we can find a more straightforward replacement for it.

- Steve Reinhardt
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3251/
-----------------------------------------------------------
(Updated Dec. 24, 2015, 12:22 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11277:b0171dac1864
---------------------------
mem: Do not use sender state to track forwarded snoops in cache
This patch changes how the cache tracks which snoops are forwarded,
and which ones are created locally. Previously the identification was
based on an empty sender state of a specific class, but this method
fails to distinguish which cache actually attached the sender
state. Instead we use the same mechanism as the crossbar, and keep
track of the requests that have outstanding snoops.
Diffs
-----
src/mem/cache/cache.hh d9a0136ab8cc
src/mem/cache/cache.cc d9a0136ab8cc
Diff: http://reviews.gem5.org/r/3251/diff/
Testing
-------
Thanks,
Andreas Hansson
Andreas Hansson
2015-12-29 13:46:19 UTC
Permalink
Post by Andreas Hansson
Post by Steve Reinhardt
Hey, I posted some comments but never gave this a ship it... I was going to say that neither solution (previous or current) seems all that great to me. Seems like if we really just want to know whether this request was issued by this cache or not, we ought to be able to do something straightforward like check req->masterId(), no?
Not that I'm a total purist or anything (and apparently I missed you doing the same trick elsewhere), but I'm not totally comfortable with using RequestPtr as a transaction ID or an STL set to model anything other than a fully associative hardware table.
FYI, the idea with SenderState is that it's really a shortcut for a master's outstanding transaction table, where normally you'd allocate a table entry and put a table index in the transaction as a tag, then do a table lookup based on that tag as a response. I thought this was discussed in the SenderState comment, but I see that it's not, unfortunately. The dynamic_cast is an abuse of this model as well, which is why I'm not defending the status quo here, just hoping that we can find a more straightforward replacement for it.
My apologies for the misunderstanding. Are you happy with this for now?

The caches do not have a masterId, and adding it is definitely a possibility, but far more invasive as it will change all existing master Ids around (and thus change all regressions). The other option is to add tags to the packets to allow tracking. Again this is a bigger change. I avoided this in the crossbar and used the constant request pointer instead. If you are against this approach then by all means let's change both the crossbar and the cache in one go.

What do you suggest?


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3251/#review7779
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3251/
-----------------------------------------------------------
(Updated Dec. 24, 2015, 8:22 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11277:b0171dac1864
---------------------------
mem: Do not use sender state to track forwarded snoops in cache
This patch changes how the cache tracks which snoops are forwarded,
and which ones are created locally. Previously the identification was
based on an empty sender state of a specific class, but this method
fails to distinguish which cache actually attached the sender
state. Instead we use the same mechanism as the crossbar, and keep
track of the requests that have outstanding snoops.
Diffs
-----
src/mem/cache/cache.hh d9a0136ab8cc
src/mem/cache/cache.cc d9a0136ab8cc
Diff: http://reviews.gem5.org/r/3251/diff/
Testing
-------
Thanks,
Andreas Hansson
Steve Reinhardt
2015-12-29 16:05:52 UTC
Permalink
Post by Andreas Hansson
Post by Steve Reinhardt
Hey, I posted some comments but never gave this a ship it... I was going to say that neither solution (previous or current) seems all that great to me. Seems like if we really just want to know whether this request was issued by this cache or not, we ought to be able to do something straightforward like check req->masterId(), no?
Not that I'm a total purist or anything (and apparently I missed you doing the same trick elsewhere), but I'm not totally comfortable with using RequestPtr as a transaction ID or an STL set to model anything other than a fully associative hardware table.
FYI, the idea with SenderState is that it's really a shortcut for a master's outstanding transaction table, where normally you'd allocate a table entry and put a table index in the transaction as a tag, then do a table lookup based on that tag as a response. I thought this was discussed in the SenderState comment, but I see that it's not, unfortunately. The dynamic_cast is an abuse of this model as well, which is why I'm not defending the status quo here, just hoping that we can find a more straightforward replacement for it.
My apologies for the misunderstanding. Are you happy with this for now?
The caches do not have a masterId, and adding it is definitely a possibility, but far more invasive as it will change all existing master Ids around (and thus change all regressions). The other option is to add tags to the packets to allow tracking. Again this is a bigger change. I avoided this in the crossbar and used the constant request pointer instead. If you are against this approach then by all means let's change both the crossbar and the cache in one go.
What do you suggest?
No need to roll this back, as the old version wasn't all that great either.

Seems like in this particular case we're really just looking at prefetches; maybe prefetchers should have a masterId so they can set it on the requests? It seems sketchy that we're sending requests out that don't have valid masterIds anyway. I don't know how masterIds are currently assigned; maybe there's a way to do all the prefetchers last so that the current assignments don't change.

What is the likely hardware implementation of the function being performed by the similar code in the crossbar? It seems like the function is different, so there's no reason to expect that we should have the same replacement despite using similar code at this point in time.


- Steve


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3251/#review7779
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3251/
-----------------------------------------------------------
(Updated Dec. 24, 2015, 12:22 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11277:b0171dac1864
---------------------------
mem: Do not use sender state to track forwarded snoops in cache
This patch changes how the cache tracks which snoops are forwarded,
and which ones are created locally. Previously the identification was
based on an empty sender state of a specific class, but this method
fails to distinguish which cache actually attached the sender
state. Instead we use the same mechanism as the crossbar, and keep
track of the requests that have outstanding snoops.
Diffs
-----
src/mem/cache/cache.hh d9a0136ab8cc
src/mem/cache/cache.cc d9a0136ab8cc
Diff: http://reviews.gem5.org/r/3251/diff/
Testing
-------
Thanks,
Andreas Hansson
Andreas Hansson
2015-12-29 16:31:17 UTC
Permalink
Post by Andreas Hansson
Post by Steve Reinhardt
Hey, I posted some comments but never gave this a ship it... I was going to say that neither solution (previous or current) seems all that great to me. Seems like if we really just want to know whether this request was issued by this cache or not, we ought to be able to do something straightforward like check req->masterId(), no?
Not that I'm a total purist or anything (and apparently I missed you doing the same trick elsewhere), but I'm not totally comfortable with using RequestPtr as a transaction ID or an STL set to model anything other than a fully associative hardware table.
FYI, the idea with SenderState is that it's really a shortcut for a master's outstanding transaction table, where normally you'd allocate a table entry and put a table index in the transaction as a tag, then do a table lookup based on that tag as a response. I thought this was discussed in the SenderState comment, but I see that it's not, unfortunately. The dynamic_cast is an abuse of this model as well, which is why I'm not defending the status quo here, just hoping that we can find a more straightforward replacement for it.
My apologies for the misunderstanding. Are you happy with this for now?
The caches do not have a masterId, and adding it is definitely a possibility, but far more invasive as it will change all existing master Ids around (and thus change all regressions). The other option is to add tags to the packets to allow tracking. Again this is a bigger change. I avoided this in the crossbar and used the constant request pointer instead. If you are against this approach then by all means let's change both the crossbar and the cache in one go.
What do you suggest?
No need to roll this back, as the old version wasn't all that great either.
Seems like in this particular case we're really just looking at prefetches; maybe prefetchers should have a masterId so they can set it on the requests? It seems sketchy that we're sending requests out that don't have valid masterIds anyway. I don't know how masterIds are currently assigned; maybe there's a way to do all the prefetchers last so that the current assignments don't change.
What is the likely hardware implementation of the function being performed by the similar code in the crossbar? It seems like the function is different, so there's no reason to expect that we should have the same replacement despite using similar code at this point in time.
The main complication with the masterId idea is that the strided prefetcher allows the user to specify that prefetches should be sent out with the masterId of the "original" packet, rather than the masterId of the prefetcher. Even worse, the queued prefetcher does not do this, and uses the port master id. The strided prefetcher does either the original or 0. Chaos...

The good news is that the prefetcher port actually has a masterId already, it is just not used consistently.

The hardware implementation in the crossbar is likely to be a fully associative tag structure, so I don't think it's too contriversial to implement it as currently is done. It is a model after all.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3251/#review7779
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3251/
-----------------------------------------------------------
(Updated Dec. 24, 2015, 8:22 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11277:b0171dac1864
---------------------------
mem: Do not use sender state to track forwarded snoops in cache
This patch changes how the cache tracks which snoops are forwarded,
and which ones are created locally. Previously the identification was
based on an empty sender state of a specific class, but this method
fails to distinguish which cache actually attached the sender
state. Instead we use the same mechanism as the crossbar, and keep
track of the requests that have outstanding snoops.
Diffs
-----
src/mem/cache/cache.hh d9a0136ab8cc
src/mem/cache/cache.cc d9a0136ab8cc
Diff: http://reviews.gem5.org/r/3251/diff/
Testing
-------
Thanks,
Andreas Hansson
Steve Reinhardt
2015-12-29 16:44:40 UTC
Permalink
Post by Andreas Hansson
Post by Steve Reinhardt
Hey, I posted some comments but never gave this a ship it... I was going to say that neither solution (previous or current) seems all that great to me. Seems like if we really just want to know whether this request was issued by this cache or not, we ought to be able to do something straightforward like check req->masterId(), no?
Not that I'm a total purist or anything (and apparently I missed you doing the same trick elsewhere), but I'm not totally comfortable with using RequestPtr as a transaction ID or an STL set to model anything other than a fully associative hardware table.
FYI, the idea with SenderState is that it's really a shortcut for a master's outstanding transaction table, where normally you'd allocate a table entry and put a table index in the transaction as a tag, then do a table lookup based on that tag as a response. I thought this was discussed in the SenderState comment, but I see that it's not, unfortunately. The dynamic_cast is an abuse of this model as well, which is why I'm not defending the status quo here, just hoping that we can find a more straightforward replacement for it.
My apologies for the misunderstanding. Are you happy with this for now?
The caches do not have a masterId, and adding it is definitely a possibility, but far more invasive as it will change all existing master Ids around (and thus change all regressions). The other option is to add tags to the packets to allow tracking. Again this is a bigger change. I avoided this in the crossbar and used the constant request pointer instead. If you are against this approach then by all means let's change both the crossbar and the cache in one go.
What do you suggest?
No need to roll this back, as the old version wasn't all that great either.
Seems like in this particular case we're really just looking at prefetches; maybe prefetchers should have a masterId so they can set it on the requests? It seems sketchy that we're sending requests out that don't have valid masterIds anyway. I don't know how masterIds are currently assigned; maybe there's a way to do all the prefetchers last so that the current assignments don't change.
What is the likely hardware implementation of the function being performed by the similar code in the crossbar? It seems like the function is different, so there's no reason to expect that we should have the same replacement despite using similar code at this point in time.
The main complication with the masterId idea is that the strided prefetcher allows the user to specify that prefetches should be sent out with the masterId of the "original" packet, rather than the masterId of the prefetcher. Even worse, the queued prefetcher does not do this, and uses the port master id. The strided prefetcher does either the original or 0. Chaos...
The good news is that the prefetcher port actually has a masterId already, it is just not used consistently.
The hardware implementation in the crossbar is likely to be a fully associative tag structure, so I don't think it's too contriversial to implement it as currently is done. It is a model after all.
So why do we allow the user to control the masterId? And if it doesn't always work anyway, would anyone notice if we took the option away? Based on your description, it seems to me that the solution would be to force the prefetcher to consistently set the masterId to its masterId, then just check that on the responses, replacing the code in this patch.

As far as the crossbar, I am fine with using an STL set to model a fully associative table (as I said in my original comment, I'm just uncomfortable using a set to model anything other than that). It would be nice if there were a more "realistic" transaction ID that could be used rather than the RequestPtr, but if no such thing is readily available, then it's not the worst thing ever.


- Steve


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3251/#review7779
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3251/
-----------------------------------------------------------
(Updated Dec. 24, 2015, 12:22 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11277:b0171dac1864
---------------------------
mem: Do not use sender state to track forwarded snoops in cache
This patch changes how the cache tracks which snoops are forwarded,
and which ones are created locally. Previously the identification was
based on an empty sender state of a specific class, but this method
fails to distinguish which cache actually attached the sender
state. Instead we use the same mechanism as the crossbar, and keep
track of the requests that have outstanding snoops.
Diffs
-----
src/mem/cache/cache.hh d9a0136ab8cc
src/mem/cache/cache.cc d9a0136ab8cc
Diff: http://reviews.gem5.org/r/3251/diff/
Testing
-------
Thanks,
Andreas Hansson
Loading...