It would be great if we can add a Param.MemoryBandwdith parameter for each direction of the link, and a basic overhead parameter for how many bytes we tack on the packet. Then we could quite easily create a throughput limiter when scheduling the next packet.

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

One general observation here is that this only models latency, and not throughput. Would it not make sense to also model throughput "properly"?

Show all issues

1. Erfan Azarkhish Aug. 19, 2015, 7:21 a.m. (Aug. 19, 2015, 7:21 a.m.)

   If we take a look at the whole system, we will see that from the performance point of view, all the following parameters have been modeled:
   
   - latency of the serial links (modeled by link_latency)
   
   - bandwidth of the serial links (modeled by link_frequency)
   
   - packetization overhead added by the serial links (modeled by link_overhead)
   
   However, as you have noticed, only latency is modeled in SerialLink, while bandwidth and packet overhead are modeled in the HMCController class.
   Since, HMCController is responsible for packetization and conversion of host transactions to HMC packets, then modeling packetization overhead in it makes sense.
   
   For bandwidth, I agree that it should be modeled inside the SerialLink itself. But, since I have used Bridge as my reference for implementation of the SerialLink and Bridge cannot model bandwidth, I have used the already available functionality inside HMCController (which is inherited from XBar) to model bandwidth (Each master port of the HMCController class deliveres a maximum bandwidth equal to the nominal bandwidth of a serial link).
   To summarize, the functionality (latency, bandwidth, overhead) is correct, but bandwidth is modeled in HMCController rather than the SerialLink itself.
   
   I hope that this makes sense.

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

spacing

Show all issues

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

Same as for responses, we seem to ignore throughput here. Also, we ignore any packetisation overhead.

Show all issues

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

This should take throughput into account imho

Show all issues

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

throughput please

Show all issues

oud

Hi Erfan,
I was thinking of something much simpler for the bandwidth throttling, much like what the SimpleMemory is doing, but here we would do it for the "popping" from the send queue. Thus, the incoming packets would still just look at the queues being full or not. We would simply throttle the send events based on the lane bandwidth (you can use Param.MemoryBandwdith for that), and the size of each flit/packet. There should be no need for any rounding or epochs. Simply do not schedule the next send event straight away, but wait enough ticks to make up for the message we just sent. Does that make sense?

Overall it looks great. Some very minor things (should not take many minutes), then I think we're done. Thanks again!

src/mem/SerialLink.py (Diff revision 5)

Could you add a comment that the bandwidth of the link is determined by the clock speed (of the comain the link is in) and the number of lanes?

Show all issues

src/mem/serial_link.hh (Diff revision 5)

Having these only for debug printouts seems a bit excessive :-)

Show all issues

src/mem/serial_link.hh (Diff revision 5)

Same as above

Show all issues

src/mem/serial_link.hh (Diff revision 5)

Only used for debug it seems

Show all issues

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

Very minor, but please stick to the style guidelines and add space around operators.

Show all issues

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

A bit excessive in my opinion...is it really important to keep this debug information?

Show all issues

Excellent. Thanks!
The summary should officially start with a keyword, so perhaps make it "mem: Add a serial link model for HMC" or similar.

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

Overall the patch seems fine to me, just one small change.

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

const ref

Show all issues