Discussion:
[gem5-dev] Review Request 3044: mem: Reflect that packet address and size are always valid
(too old to reply)
Andreas Hansson
2015-08-19 09:06:14 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3044/
-----------------------------------------------------------

Review request for Default.


Repository: gem5


Description
-------

Changeset 11055:dc0a15e6e113
---------------------------
mem: Reflect that packet address and size are always valid

This patch simplifies the packet, and removes the possibility of
creating a packet without a valid address and/or size. Under no
circumstances are these fields set at a later point, and thus they
really have to be provided at construction time.

The patch also fixes a case there the MinorCPU creates a packet
without a valid address and size, only to later delete it.


Diffs
-----

src/cpu/minor/lsq.cc 110cce93d398
src/mem/packet.hh 110cce93d398

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


Testing
-------


Thanks,

Andreas Hansson
Steve Reinhardt
2015-08-19 14:56:01 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3044/#review7015
-----------------------------------------------------------



src/mem/packet.hh (line 572)
<http://reviews.gem5.org/r/3044/#comment6017>

Looks like you need to update this comment. I assume that no one ever used the capability described here?


- Steve Reinhardt
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3044/
-----------------------------------------------------------
(Updated Aug. 19, 2015, 2:06 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11055:dc0a15e6e113
---------------------------
mem: Reflect that packet address and size are always valid
This patch simplifies the packet, and removes the possibility of
creating a packet without a valid address and/or size. Under no
circumstances are these fields set at a later point, and thus they
really have to be provided at construction time.
The patch also fixes a case there the MinorCPU creates a packet
without a valid address and size, only to later delete it.
Diffs
-----
src/cpu/minor/lsq.cc 110cce93d398
src/mem/packet.hh 110cce93d398
Diff: http://reviews.gem5.org/r/3044/diff/
Testing
-------
Thanks,
Andreas Hansson
Andreas Hansson
2015-08-20 21:00:51 UTC
Permalink
Post by Andreas Hansson
src/mem/packet.hh, line 580
<http://reviews.gem5.org/r/3044/diff/1/?file=49083#file49083line580>
Looks like you need to update this comment. I assume that no one ever used the capability described here?
Indeed, it was never used. I'll update the comment.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3044/#review7015
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3044/
-----------------------------------------------------------
(Updated Aug. 19, 2015, 9:06 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11055:dc0a15e6e113
---------------------------
mem: Reflect that packet address and size are always valid
This patch simplifies the packet, and removes the possibility of
creating a packet without a valid address and/or size. Under no
circumstances are these fields set at a later point, and thus they
really have to be provided at construction time.
The patch also fixes a case there the MinorCPU creates a packet
without a valid address and size, only to later delete it.
Diffs
-----
src/cpu/minor/lsq.cc 110cce93d398
src/mem/packet.hh 110cce93d398
Diff: http://reviews.gem5.org/r/3044/diff/
Testing
-------
Thanks,
Andreas Hansson
Joel Hestness
2015-08-23 19:12:52 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3044/#review7028
-----------------------------------------------------------


Hi Andreas. I'm wondering why this patch was checked in (http://repo.gem5.org/gem5/rev/842f56345a42). It doesn't have a 'Ship It', and the review request is only 4 days old.

This change also breaks some packet use cases we have in gem5-gpu. Specifically, this change now requires that Requests be translated or otherwise find a valid physical address before a packet can be constructed. I'm not convinced that this is a reasonable requirement, especially since packets can be used between components that operate completely on virtual addresses.

Can you please revert this change?

- Joel Hestness
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3044/
-----------------------------------------------------------
(Updated Aug. 19, 2015, 9:06 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11055:dc0a15e6e113
---------------------------
mem: Reflect that packet address and size are always valid
This patch simplifies the packet, and removes the possibility of
creating a packet without a valid address and/or size. Under no
circumstances are these fields set at a later point, and thus they
really have to be provided at construction time.
The patch also fixes a case there the MinorCPU creates a packet
without a valid address and size, only to later delete it.
Diffs
-----
src/cpu/minor/lsq.cc 110cce93d398
src/mem/packet.hh 110cce93d398
Diff: http://reviews.gem5.org/r/3044/diff/
Testing
-------
Thanks,
Andreas Hansson
Andreas Hansson
2015-08-24 07:37:59 UTC
Permalink
Post by Andreas Hansson
Post by Joel Hestness
Hi Andreas. I'm wondering why this patch was checked in (http://repo.gem5.org/gem5/rev/842f56345a42). It doesn't have a 'Ship It', and the review request is only 4 days old.
This change also breaks some packet use cases we have in gem5-gpu. Specifically, this change now requires that Requests be translated or otherwise find a valid physical address before a packet can be constructed. I'm not convinced that this is a reasonable requirement, especially since packets can be used between components that operate completely on virtual addresses.
Can you please revert this change?
Hi Joel,

I called out for feedback, and Steve was the only one to respond. Since the patch was fairly trivial and the functionality was never used I opted to not gate other changes on this minor modification.

Could you explain your use-case a bit better? Packets _need_ a physical address. Previously there was an option to create a va-only request, then create the packet, then do the va->pa. Note that the packet always had to hava physical address in both cases. The late setting was _never_ used in the main gem5 codebase and opened up for nasty bugs (trust me on this one).

How is the gem5-gpu use-case different from what is happening in the main codebase? Why not first create the request (do any translation), and then create the packet?


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3044/#review7028
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3044/
-----------------------------------------------------------
(Updated Aug. 19, 2015, 9:06 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11055:dc0a15e6e113
---------------------------
mem: Reflect that packet address and size are always valid
This patch simplifies the packet, and removes the possibility of
creating a packet without a valid address and/or size. Under no
circumstances are these fields set at a later point, and thus they
really have to be provided at construction time.
The patch also fixes a case there the MinorCPU creates a packet
without a valid address and size, only to later delete it.
Diffs
-----
src/cpu/minor/lsq.cc 110cce93d398
src/mem/packet.hh 110cce93d398
Diff: http://reviews.gem5.org/r/3044/diff/
Testing
-------
Thanks,
Andreas Hansson
Joel Hestness
2015-08-24 14:49:13 UTC
Permalink
Post by Andreas Hansson
Post by Joel Hestness
Hi Andreas. I'm wondering why this patch was checked in (http://repo.gem5.org/gem5/rev/842f56345a42). It doesn't have a 'Ship It', and the review request is only 4 days old.
This change also breaks some packet use cases we have in gem5-gpu. Specifically, this change now requires that Requests be translated or otherwise find a valid physical address before a packet can be constructed. I'm not convinced that this is a reasonable requirement, especially since packets can be used between components that operate completely on virtual addresses.
Can you please revert this change?
Hi Joel,
I called out for feedback, and Steve was the only one to respond. Since the patch was fairly trivial and the functionality was never used I opted to not gate other changes on this minor modification.
Could you explain your use-case a bit better? Packets _need_ a physical address. Previously there was an option to create a va-only request, then create the packet, then do the va->pa. Note that the packet always had to hava physical address in both cases. The late setting was _never_ used in the main gem5 codebase and opened up for nasty bugs (trust me on this one).
How is the gem5-gpu use-case different from what is happening in the main codebase? Why not first create the request (do any translation), and then create the packet?
Thank you for reverting the change. I would have liked more than 2 days to provide feedback on this change, especially since it lacked a 'Ship It'.

There are a number of use cases where this change causes strange requirements. The clearest in gem5-gpu is in the GPU where we communicate virtual addresses from the memory pipeline dispatch to the load-store queue (which does coalescing first). Coalescing occurs before address translation, so Requests to the LSQ can only contain a virtual address. This patch would require calling req->setPaddr(<something>) just to create the packet to communicate to the load-store queue. Given there isn't a physical address yet, including setPaddr() in the code is strange and misleading both in writing and reading the code.

Other use cases involve communicating commands other than loads/store, such as control messages or fence commands. I don't see why we should require that a request have a valid physical address to create a packet. These packet commands don't even require a virtual address but do require communicating things like the command, request flags, master ID, thread ID, sender states, etc.

Finally, there is a use case in which we extend packet to store GPU access decoalescing state, and we don't have a single *virtual* address before instantiating the state and packet. Other recent gem5 changes dictate that we're going to need to change this, but this is another instance that indicates the desire to be able to create packets without valid physical addresses.


- Joel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3044/#review7028
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3044/
-----------------------------------------------------------
(Updated Aug. 24, 2015, 9:04 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11058:2c40795e9a88
---------------------------
mem: Reflect that packet address and size are always valid
This patch simplifies the packet, and removes the possibility of
creating a packet without a valid address and/or size. Under no
circumstances are these fields set at a later point, and thus they
really have to be provided at construction time.
Diffs
-----
src/mem/packet.hh 842f56345a42
Diff: http://reviews.gem5.org/r/3044/diff/
Testing
-------
Thanks,
Andreas Hansson
Andreas Hansson
2015-08-24 15:23:54 UTC
Permalink
Post by Andreas Hansson
Post by Joel Hestness
Hi Andreas. I'm wondering why this patch was checked in (http://repo.gem5.org/gem5/rev/842f56345a42). It doesn't have a 'Ship It', and the review request is only 4 days old.
This change also breaks some packet use cases we have in gem5-gpu. Specifically, this change now requires that Requests be translated or otherwise find a valid physical address before a packet can be constructed. I'm not convinced that this is a reasonable requirement, especially since packets can be used between components that operate completely on virtual addresses.
Can you please revert this change?
Hi Joel,
I called out for feedback, and Steve was the only one to respond. Since the patch was fairly trivial and the functionality was never used I opted to not gate other changes on this minor modification.
Could you explain your use-case a bit better? Packets _need_ a physical address. Previously there was an option to create a va-only request, then create the packet, then do the va->pa. Note that the packet always had to hava physical address in both cases. The late setting was _never_ used in the main gem5 codebase and opened up for nasty bugs (trust me on this one).
How is the gem5-gpu use-case different from what is happening in the main codebase? Why not first create the request (do any translation), and then create the packet?
Thank you for reverting the change. I would have liked more than 2 days to provide feedback on this change, especially since it lacked a 'Ship It'.
There are a number of use cases where this change causes strange requirements. The clearest in gem5-gpu is in the GPU where we communicate virtual addresses from the memory pipeline dispatch to the load-store queue (which does coalescing first). Coalescing occurs before address translation, so Requests to the LSQ can only contain a virtual address. This patch would require calling req->setPaddr(<something>) just to create the packet to communicate to the load-store queue. Given there isn't a physical address yet, including setPaddr() in the code is strange and misleading both in writing and reading the code.
Other use cases involve communicating commands other than loads/store, such as control messages or fence commands. I don't see why we should require that a request have a valid physical address to create a packet. These packet commands don't even require a virtual address but do require communicating things like the command, request flags, master ID, thread ID, sender states, etc.
Finally, there is a use case in which we extend packet to store GPU access decoalescing state, and we don't have a single *virtual* address before instantiating the state and packet. Other recent gem5 changes dictate that we're going to need to change this, but this is another instance that indicates the desire to be able to create packets without valid physical addresses.
I'd say it's your use of packets that is "strange" (or at least not in line with the rest of gem5) :-)

Why do you not coalesce/split the requests, as done in other places, before you create the packets?

The address is fundamentally needed by the memory system (caches, crossbars etc), and throughout the entire gem5 codebase you can always assume that a packet has a valid (physical) address once created. Note that this requirement is also in line with SystemC TLM, and I'd argue there is reason to keep gem5's assumptions as similar as possible.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3044/#review7028
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3044/
-----------------------------------------------------------
(Updated Aug. 24, 2015, 9:04 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11058:2c40795e9a88
---------------------------
mem: Reflect that packet address and size are always valid
This patch simplifies the packet, and removes the possibility of
creating a packet without a valid address and/or size. Under no
circumstances are these fields set at a later point, and thus they
really have to be provided at construction time.
Diffs
-----
src/mem/packet.hh 842f56345a42
Diff: http://reviews.gem5.org/r/3044/diff/
Testing
-------
Thanks,
Andreas Hansson
Joel Hestness
2015-08-24 17:43:51 UTC
Permalink
Post by Andreas Hansson
Post by Joel Hestness
Hi Andreas. I'm wondering why this patch was checked in (http://repo.gem5.org/gem5/rev/842f56345a42). It doesn't have a 'Ship It', and the review request is only 4 days old.
This change also breaks some packet use cases we have in gem5-gpu. Specifically, this change now requires that Requests be translated or otherwise find a valid physical address before a packet can be constructed. I'm not convinced that this is a reasonable requirement, especially since packets can be used between components that operate completely on virtual addresses.
Can you please revert this change?
Hi Joel,
I called out for feedback, and Steve was the only one to respond. Since the patch was fairly trivial and the functionality was never used I opted to not gate other changes on this minor modification.
Could you explain your use-case a bit better? Packets _need_ a physical address. Previously there was an option to create a va-only request, then create the packet, then do the va->pa. Note that the packet always had to hava physical address in both cases. The late setting was _never_ used in the main gem5 codebase and opened up for nasty bugs (trust me on this one).
How is the gem5-gpu use-case different from what is happening in the main codebase? Why not first create the request (do any translation), and then create the packet?
Thank you for reverting the change. I would have liked more than 2 days to provide feedback on this change, especially since it lacked a 'Ship It'.
There are a number of use cases where this change causes strange requirements. The clearest in gem5-gpu is in the GPU where we communicate virtual addresses from the memory pipeline dispatch to the load-store queue (which does coalescing first). Coalescing occurs before address translation, so Requests to the LSQ can only contain a virtual address. This patch would require calling req->setPaddr(<something>) just to create the packet to communicate to the load-store queue. Given there isn't a physical address yet, including setPaddr() in the code is strange and misleading both in writing and reading the code.
Other use cases involve communicating commands other than loads/store, such as control messages or fence commands. I don't see why we should require that a request have a valid physical address to create a packet. These packet commands don't even require a virtual address but do require communicating things like the command, request flags, master ID, thread ID, sender states, etc.
Finally, there is a use case in which we extend packet to store GPU access decoalescing state, and we don't have a single *virtual* address before instantiating the state and packet. Other recent gem5 changes dictate that we're going to need to change this, but this is another instance that indicates the desire to be able to create packets without valid physical addresses.
I'd say it's your use of packets that is "strange" (or at least not in line with the rest of gem5) :-)
Why do you not coalesce/split the requests, as done in other places, before you create the packets?
The address is fundamentally needed by the memory system (caches, crossbars etc), and throughout the entire gem5 codebase you can always assume that a packet has a valid (physical) address once created. Note that this requirement is also in line with SystemC TLM, and I'd argue there is reason to keep gem5's assumptions as similar as possible.
Why do you not coalesce/split the requests, as done in other places, before you create the packets?
This route only addresses one of our use cases, and as I stated, we're going to need to change this particular case. I gave 2-3 other use cases for which it doesn't make sense to require a valid physical address. Do you have recommendations for addressing those? I don't see what is 'strange' about them, or even how they can be any different.
Post by Andreas Hansson
The address is fundamentally needed by the memory system (caches, crossbars etc), and throughout the entire gem5 codebase you can always assume that a packet has a valid (physical) address once created.
How would one model virtually-addressed caches? This patch would disallow using packets to send to a virtually-addressed cache.

I apologize if I'm missing something obvious, but I'm hoping you can clarify your overall argument. I agree that packets to the memory system require addresses (obvious if needed for routing). I can also acknowledge that for all current gem5 uses of packets within memory systems, it may make sense to require the physical address (there are no virtually-addressed memory system components currently). However, I don't understand why having a physical address is an appropriate requirement for packets that do not access memory locations (i.e. are not loads/stores) and do not need to be routed based on address in the memory system. I also don't understand why a physical address should be required if communicating between components outside a cache hierarchy where addresses may not be required (i.e. communicating through ports). This latter use of packets has always been allowed in gem5.


Separate from the semantics of this specific patch, this sort of change is something we've been grappling with as a collaborative community: Trying to remove components/functionality and increase interface strictness can be exceptionally painful for anyone that has internal patches derived from the code being removed/restricted. AMD has vetoed a handful of such patches recently. I feel this case is very similar to those vetoes, and especially without more clarity on how existing packet use should be changed.


- Joel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3044/#review7028
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3044/
-----------------------------------------------------------
(Updated Aug. 24, 2015, 9:04 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11058:2c40795e9a88
---------------------------
mem: Reflect that packet address and size are always valid
This patch simplifies the packet, and removes the possibility of
creating a packet without a valid address and/or size. Under no
circumstances are these fields set at a later point, and thus they
really have to be provided at construction time.
Diffs
-----
src/mem/packet.hh 842f56345a42
Diff: http://reviews.gem5.org/r/3044/diff/
Testing
-------
Thanks,
Andreas Hansson
Andreas Hansson
2015-08-24 20:48:58 UTC
Permalink
Post by Andreas Hansson
Post by Joel Hestness
Hi Andreas. I'm wondering why this patch was checked in (http://repo.gem5.org/gem5/rev/842f56345a42). It doesn't have a 'Ship It', and the review request is only 4 days old.
This change also breaks some packet use cases we have in gem5-gpu. Specifically, this change now requires that Requests be translated or otherwise find a valid physical address before a packet can be constructed. I'm not convinced that this is a reasonable requirement, especially since packets can be used between components that operate completely on virtual addresses.
Can you please revert this change?
Hi Joel,
I called out for feedback, and Steve was the only one to respond. Since the patch was fairly trivial and the functionality was never used I opted to not gate other changes on this minor modification.
Could you explain your use-case a bit better? Packets _need_ a physical address. Previously there was an option to create a va-only request, then create the packet, then do the va->pa. Note that the packet always had to hava physical address in both cases. The late setting was _never_ used in the main gem5 codebase and opened up for nasty bugs (trust me on this one).
How is the gem5-gpu use-case different from what is happening in the main codebase? Why not first create the request (do any translation), and then create the packet?
Thank you for reverting the change. I would have liked more than 2 days to provide feedback on this change, especially since it lacked a 'Ship It'.
There are a number of use cases where this change causes strange requirements. The clearest in gem5-gpu is in the GPU where we communicate virtual addresses from the memory pipeline dispatch to the load-store queue (which does coalescing first). Coalescing occurs before address translation, so Requests to the LSQ can only contain a virtual address. This patch would require calling req->setPaddr(<something>) just to create the packet to communicate to the load-store queue. Given there isn't a physical address yet, including setPaddr() in the code is strange and misleading both in writing and reading the code.
Other use cases involve communicating commands other than loads/store, such as control messages or fence commands. I don't see why we should require that a request have a valid physical address to create a packet. These packet commands don't even require a virtual address but do require communicating things like the command, request flags, master ID, thread ID, sender states, etc.
Finally, there is a use case in which we extend packet to store GPU access decoalescing state, and we don't have a single *virtual* address before instantiating the state and packet. Other recent gem5 changes dictate that we're going to need to change this, but this is another instance that indicates the desire to be able to create packets without valid physical addresses.
I'd say it's your use of packets that is "strange" (or at least not in line with the rest of gem5) :-)
Why do you not coalesce/split the requests, as done in other places, before you create the packets?
The address is fundamentally needed by the memory system (caches, crossbars etc), and throughout the entire gem5 codebase you can always assume that a packet has a valid (physical) address once created. Note that this requirement is also in line with SystemC TLM, and I'd argue there is reason to keep gem5's assumptions as similar as possible.
Post by Joel Hestness
Why do you not coalesce/split the requests, as done in other places, before you create the packets?
This route only addresses one of our use cases, and as I stated, we're going to need to change this particular case. I gave 2-3 other use cases for which it doesn't make sense to require a valid physical address. Do you have recommendations for addressing those? I don't see what is 'strange' about them, or even how they can be any different.
Post by Joel Hestness
The address is fundamentally needed by the memory system (caches, crossbars etc), and throughout the entire gem5 codebase you can always assume that a packet has a valid (physical) address once created.
How would one model virtually-addressed caches? This patch would disallow using packets to send to a virtually-addressed cache.
I apologize if I'm missing something obvious, but I'm hoping you can clarify your overall argument. I agree that packets to the memory system require addresses (obvious if needed for routing). I can also acknowledge that for all current gem5 uses of packets within memory systems, it may make sense to require the physical address (there are no virtually-addressed memory system components currently). However, I don't understand why having a physical address is an appropriate requirement for packets that do not access memory locations (i.e. are not loads/stores) and do not need to be routed based on address in the memory system. I also don't understand why a physical address should be required if communicating between components outside a cache hierarchy where addresses may not be required (i.e. communicating through ports). This latter use of packets has always been allowed in gem5.
Separate from the semantics of this specific patch, this sort of change is something we've been grappling with as a collaborative community: Trying to remove components/functionality and increase interface strictness can be exceptionally painful for anyone that has internal patches derived from the code being removed/restricted. AMD has vetoed a handful of such patches recently. I feel this case is very similar to those vetoes, and especially without more clarity on how existing packet use should be changed.
My comment regarding it being "strange" was merely acknowledging the fact that the use-cases you mention are not part of the gem5 codebase. Perhaps unconventional would be a better adjective.

The reason for the change is simplicity. The memory system is riddled with assumptions, making it harder than necessary to understand, debug and extend. This patch is merely one small step towards making the assumptions more explicit, and making life easier for any developer. It is impossible for me or anyone else to know what assumptions people make outside the main codebase. I think the guiding principle should be to firm things up as much as possible, so if there are unconventional use-cases we need to accommodate, it would be good to get at least part of that into the main codebase so that it is visible.

On the grand scheme of things, if this patch is causing problems, let's just discard it. There are definitely more fruitful issues to spend our reviewing efforts on.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3044/#review7028
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3044/
-----------------------------------------------------------
(Updated Aug. 24, 2015, 9:04 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11058:2c40795e9a88
---------------------------
mem: Reflect that packet address and size are always valid
This patch simplifies the packet, and removes the possibility of
creating a packet without a valid address and/or size. Under no
circumstances are these fields set at a later point, and thus they
really have to be provided at construction time.
Diffs
-----
src/mem/packet.hh 842f56345a42
Diff: http://reviews.gem5.org/r/3044/diff/
Testing
-------
Thanks,
Andreas Hansson
Joel Hestness
2015-08-25 14:12:30 UTC
Permalink
Post by Andreas Hansson
Post by Joel Hestness
Hi Andreas. I'm wondering why this patch was checked in (http://repo.gem5.org/gem5/rev/842f56345a42). It doesn't have a 'Ship It', and the review request is only 4 days old.
This change also breaks some packet use cases we have in gem5-gpu. Specifically, this change now requires that Requests be translated or otherwise find a valid physical address before a packet can be constructed. I'm not convinced that this is a reasonable requirement, especially since packets can be used between components that operate completely on virtual addresses.
Can you please revert this change?
Hi Joel,
I called out for feedback, and Steve was the only one to respond. Since the patch was fairly trivial and the functionality was never used I opted to not gate other changes on this minor modification.
Could you explain your use-case a bit better? Packets _need_ a physical address. Previously there was an option to create a va-only request, then create the packet, then do the va->pa. Note that the packet always had to hava physical address in both cases. The late setting was _never_ used in the main gem5 codebase and opened up for nasty bugs (trust me on this one).
How is the gem5-gpu use-case different from what is happening in the main codebase? Why not first create the request (do any translation), and then create the packet?
Thank you for reverting the change. I would have liked more than 2 days to provide feedback on this change, especially since it lacked a 'Ship It'.
There are a number of use cases where this change causes strange requirements. The clearest in gem5-gpu is in the GPU where we communicate virtual addresses from the memory pipeline dispatch to the load-store queue (which does coalescing first). Coalescing occurs before address translation, so Requests to the LSQ can only contain a virtual address. This patch would require calling req->setPaddr(<something>) just to create the packet to communicate to the load-store queue. Given there isn't a physical address yet, including setPaddr() in the code is strange and misleading both in writing and reading the code.
Other use cases involve communicating commands other than loads/store, such as control messages or fence commands. I don't see why we should require that a request have a valid physical address to create a packet. These packet commands don't even require a virtual address but do require communicating things like the command, request flags, master ID, thread ID, sender states, etc.
Finally, there is a use case in which we extend packet to store GPU access decoalescing state, and we don't have a single *virtual* address before instantiating the state and packet. Other recent gem5 changes dictate that we're going to need to change this, but this is another instance that indicates the desire to be able to create packets without valid physical addresses.
I'd say it's your use of packets that is "strange" (or at least not in line with the rest of gem5) :-)
Why do you not coalesce/split the requests, as done in other places, before you create the packets?
The address is fundamentally needed by the memory system (caches, crossbars etc), and throughout the entire gem5 codebase you can always assume that a packet has a valid (physical) address once created. Note that this requirement is also in line with SystemC TLM, and I'd argue there is reason to keep gem5's assumptions as similar as possible.
Post by Joel Hestness
Why do you not coalesce/split the requests, as done in other places, before you create the packets?
This route only addresses one of our use cases, and as I stated, we're going to need to change this particular case. I gave 2-3 other use cases for which it doesn't make sense to require a valid physical address. Do you have recommendations for addressing those? I don't see what is 'strange' about them, or even how they can be any different.
Post by Joel Hestness
The address is fundamentally needed by the memory system (caches, crossbars etc), and throughout the entire gem5 codebase you can always assume that a packet has a valid (physical) address once created.
How would one model virtually-addressed caches? This patch would disallow using packets to send to a virtually-addressed cache.
I apologize if I'm missing something obvious, but I'm hoping you can clarify your overall argument. I agree that packets to the memory system require addresses (obvious if needed for routing). I can also acknowledge that for all current gem5 uses of packets within memory systems, it may make sense to require the physical address (there are no virtually-addressed memory system components currently). However, I don't understand why having a physical address is an appropriate requirement for packets that do not access memory locations (i.e. are not loads/stores) and do not need to be routed based on address in the memory system. I also don't understand why a physical address should be required if communicating between components outside a cache hierarchy where addresses may not be required (i.e. communicating through ports). This latter use of packets has always been allowed in gem5.
Separate from the semantics of this specific patch, this sort of change is something we've been grappling with as a collaborative community: Trying to remove components/functionality and increase interface strictness can be exceptionally painful for anyone that has internal patches derived from the code being removed/restricted. AMD has vetoed a handful of such patches recently. I feel this case is very similar to those vetoes, and especially without more clarity on how existing packet use should be changed.
My comment regarding it being "strange" was merely acknowledging the fact that the use-cases you mention are not part of the gem5 codebase. Perhaps unconventional would be a better adjective.
The reason for the change is simplicity. The memory system is riddled with assumptions, making it harder than necessary to understand, debug and extend. This patch is merely one small step towards making the assumptions more explicit, and making life easier for any developer. It is impossible for me or anyone else to know what assumptions people make outside the main codebase. I think the guiding principle should be to firm things up as much as possible, so if there are unconventional use-cases we need to accommodate, it would be good to get at least part of that into the main codebase so that it is visible.
On the grand scheme of things, if this patch is causing problems, let's just discard it. There are definitely more fruitful issues to spend our reviewing efforts on.
Ok. Thanks for the notes. I suspect we might see the fence packet use case with the incoming AMD GPU patches. I'm still not sure if/when we might try to get gem5-gpu code into gem5.


- Joel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3044/#review7028
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/3044/
-----------------------------------------------------------
(Updated Aug. 24, 2015, 9:04 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11058:2c40795e9a88
---------------------------
mem: Reflect that packet address and size are always valid
This patch simplifies the packet, and removes the possibility of
creating a packet without a valid address and/or size. Under no
circumstances are these fields set at a later point, and thus they
really have to be provided at construction time.
Diffs
-----
src/mem/packet.hh 842f56345a42
Diff: http://reviews.gem5.org/r/3044/diff/
Testing
-------
Thanks,
Andreas Hansson
Andreas Hansson
2015-08-24 09:04:56 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3044/
-----------------------------------------------------------

(Updated Aug. 24, 2015, 9:04 a.m.)


Review request for Default.


Repository: gem5


Description (updated)
-------

Changeset 11058:2c40795e9a88
---------------------------
mem: Reflect that packet address and size are always valid

This patch simplifies the packet, and removes the possibility of
creating a packet without a valid address and/or size. Under no
circumstances are these fields set at a later point, and thus they
really have to be provided at construction time.


Diffs (updated)
-----

src/mem/packet.hh 842f56345a42

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


Testing
-------


Thanks,

Andreas Hansson
Loading...