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

Cycles as the type
Should you use divCeil here?
Should there not be overheads in addition to the packet size? 16 bytes (make it a parameter?)

Show all issues

1. Erfan Azarkhish July 29, 2015, 4:05 a.m. (July 29, 2015, 4:05 a.m.)

   I fixed Cycles.
   There are two latency parameters in HMC.py related to the serial link:
   
   1) link_latency:    this is modeled using the delay of the serial_link component
   
   2) link_overhead:   this is modeled using forward_latency and response_latency of the HMC controller (HMC.py)
   
   The correct solution would have been to implement both of these delays in the serial_link itself, but I chose the easier solution, and modeled link_overhead in the hmccontroller.
   I hope this makes sense

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

no need to do this, call schedTimingResp(pkt, t, true).
Why would changing the order be a problem?

Show all issues

1. Erfan Azarkhish July 29, 2015, 4:05 a.m. (July 29, 2015, 4:05 a.m.)

   There is no prototype for schedTimingResp(pkt, t, true), only schedTimingResp(pkt, t)
   Please correct me if I am wrong.
   
   If the processor sends two uncached requests towards HMC and the second one is smaller than the first one. It may happen that the second one crosses this link faster than the first one (because the packet waits in the link based on its size). This can reorder the received response.
   
   I think that this can cause a problem

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

Same as the other one in terms of units, divCeil, and flit overhead.
We should definitely add the pkt->headerDelay before zeroing it.
Should we not also add pkt->payloadDelay? Can we start serialising before the "link controller" has the entire packet?

Show all issues

1. Erfan Azarkhish July 29, 2015, 4:05 a.m. (July 29, 2015, 4:05 a.m.)

   I fixed the cycles.
   Yes, the serializer can start serialization, as soon as it has received the first flit. Because, if during the serialization an error occurs, the serializer will simply inform the receiver by modifying the error detection code in the tail flit. Then, the receiver will drop the packet, and later a retransmission will occur.
   About pkt->headerDelay, I am still not sure yet, I will look more into it separately.

Could we split this? The Changes to DRAMCtrl are good to go. The SerialLink needs a tiny bit of work.

configs/common/MemConfig.py (Diff revision 1)

Makes sense. The change to MemConfig.py is good to go.

Show all issues

Format this patch again.  Read the following page: http://gem5.org/Submitting_Contributions

You need to add copyrights to all the new files that are being added.

configs/common/HMC.py (Diff revision 7)

Why do we need to derive HMCSystem from SubSystem?

Show all issues

1. Erfan Azarkhish Oct. 17, 2015, 12:09 p.m. (Oct. 17, 2015, 12:09 p.m.)

   My goal has been to improve modularity, readability of the stats, and visualization of the HMC device.
   
   Also, based on HMC specs it should be possible to instantiate  multiple HMC devices and connect them to each other in different forms.
   
   I think having HMC as a subsystem can be helpful for the mentioned reasons.

2. Nilay Vaish Oct. 17, 2015, 10:47 p.m. (Oct. 17, 2015, 10:47 p.m.)

   Somehow I still do not like the current organization.  But I am failing to
   
   come with anything better.  So I'll take what you have.

3. Andreas Hansson Oct. 18, 2015, 7:15 a.m. (Oct. 18, 2015, 7:15 a.m.)

   I think a SubSystem is a great way of accomplishing modularity here. We don't use it enough imho. What is your reservation Nilay?

4. Nilay Vaish Oct. 20, 2015, 6:28 a.m. (Oct. 20, 2015, 6:28 a.m.)

   SubSystem does not have any configuration params, which means it is same as having a
   
   SimObject.  As of now, I don't see any use for SubSystem.

5. Andreas Hansson Oct. 20, 2015, 6:31 a.m. (Oct. 20, 2015, 6:31 a.m.)

   I re-read the diff, and it does not make sense to inherit from SubSystem (for the reasons you mention Nilay).
   The SubSystem class itself makes a lot of sense though.

configs/example/fs.py (Diff revision 7)

I think all of this code should be in MemConfig.py.

Show all issues

1. Andreas Hansson Oct. 18, 2015, 7:15 a.m. (Oct. 18, 2015, 7:15 a.m.)

   This is not as clear cut as you make it sound Nilay. The memory controller itself represents a single channel, and traditionally the MemConfig functions have created a number of channels, and attached them to a crossbar (usually the system.membus). In the HMC case we are dealing with a more complicated system, and you may even have different links connecting different "systems". I am not objecting to moving things into MemConfig, but "mem_type" is being slightly abused here as it normally refers only to the subclass of DRAMCtrl. Here the string is being hi-jacked to also mean insert a host-side controller and a number of serial links. I am tempted to suggest we leave this out of this patch, and rather allow an option "Buffer-on-Board" or similar, in addition to the mem_type.

2. Nilay Vaish Oct. 20, 2015, 6:36 a.m. (Oct. 20, 2015, 6:36 a.m.)

   I'll let Erfan and you decide how to organize the code here.  I just do not like
   
   adding this code in fs.py.  And I think HMC is not dependent on whether we are in
   
   fs or se mode.

configs/example/hmctest.py (Diff revision 7)

Drop this file.  Sooner or later it would become out of date.  Instead add a new regression test for HMC.

Show all issues

1. Erfan Azarkhish Oct. 17, 2015, 12:09 p.m. (Oct. 17, 2015, 12:09 p.m.)

   As you said, I dropped this file. I will add the regression test later in a separate patch. Because I don't know how to do it yet.

2. Nilay Vaish Oct. 17, 2015, 10:47 p.m. (Oct. 17, 2015, 10:47 p.m.)

   Absolutely fine with me.

3. Andreas Hansson Oct. 18, 2015, 7:15 a.m. (Oct. 18, 2015, 7:15 a.m.)

   No reason why the test cannot call the config in configs/dram or configs/example...

configs/common/HMC.py (Diff revision 8)

How about having system.hmc.controller?  It seems strange that this particular component is being treated differently compared all other components.

Show all issues

1. Erfan Azarkhish Oct. 18, 2015, 9:29 a.m. (Oct. 18, 2015, 9:29 a.m.)

   In principle, HMCController is a separate component which should sit between the host processors and the HMC device(s). It may be integrated with the host side SoC, but for sure it is not located on the HMC device itself.In current simulation configuration, this does not make much difference, but in a multi-HMC system there is not necessarily a one-to-one correspondense between HMCs and HMCControllers. We could connect one HMCController to more than one HMC device. This is because the serial links on HMC are very flexible each one capable of addressing the whole address space, so in theory it is possible to connect HMCs, HMCControllers, and host processors in many different topologies and configurations.
   
   For this reason, I suggest to keep HMCController as a separate component and leave it out from HMC subsystem.

configs/common/MemConfig.py (Diff revision 8)

Not required any longer.

Show all issues

Can we close the open issues (if they are resolved)?
I also think we should split out the changes to the HMC DRAM config into a separate patch. That can then go in with the serial link without having to close out the bigger config system issues.