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

That is 64 kB of implicit storage! As I've already mentioned many times, this check has been instrumental in finding lots of problems in the memory system, and I am against changing it.
I think the best option here is a compile-time switch for disabling this sanity check. Perhaps we can call it "ludicrous mode" :-)
If you want to set that from Ruby and remove the safety net that's fine.

Show all issues

1. Brad Beckmann Nov. 12, 2015, 10:40 a.m. (Nov. 12, 2015, 10:40 a.m.)

   First off, thank you for suggesting a solution this time around rather than just stating you are against the patch!
   Can you further elaborate on what sort of compile-time switch you will approve?  Would you be ok with the src/mem/SConscript doing something like this?
   
   if env['PROTOCOL'] == 'None':
   
       #define MAX_PKT_Q_SIZE = 100
   
   else:
   
       #define MAX_PKT_Q_SIZE = 10000
   Also don't worry about increasing the packet queue size for Ruby.  Ruby already includes a deadlock safety net that identifies when things have gone off the rails.

2. Andreas Hansson Nov. 12, 2015, 10:58 a.m. (Nov. 12, 2015, 10:58 a.m.)

   How about a constructor parameter disable_sanity_check with a default false, Ruby can set it to true.
   I still find it odd that there are "correct" systems where we want to have 64 kByte of invisible storage :-)

3. Tony Gutierrez Nov. 12, 2015, 2:58 p.m. (Nov. 12, 2015, 2:58 p.m.)

   Added this and updated the patch.

src/mem/packet_queue.hh (Diff revision 3)

should also be an added @param here

Show all issues

src/mem/packet_queue.hh (Diff revision 3)

bool disable_sanity_check to comply with the style guide. please also add a space around =

Show all issues

1. Tony Gutierrez Nov. 12, 2015, 3:16 p.m. (Nov. 12, 2015, 3:16 p.m.)

   Can you point out where in the style guide this is mentioned? Is there a newer style guide than this: http://gem5.org/Coding_Style ? This guide explicitly states "no space around '=' when used in parameter/argument lists, either to bind default parameter values (in Python or C++) or to bind keyword arguments (in Python)". Also, it specifices member variables must use camel-case, whereas local variables use snake-case; it doesn't mention parameters/aguments afaik. I don't mind changing it, but I think if we're going to have a style guide any new standards need to be maintained on the wiki in one location.

2. Andreas Hansson Nov. 12, 2015, 3:18 p.m. (Nov. 12, 2015, 3:18 p.m.)

   The constructor parameter is a local variable. At least that is how it is treated everywhere else.
   To the best of my knowledge the only exception to the spacing is in the Python world.

src/mem/packet_queue.cc (Diff revision 3)

see comment from header on naming

Show all issues

src/mem/packet_queue.cc (Diff revision 3)

could swap these around, up to you

Show all issues

1. Tony Gutierrez Nov. 12, 2015, 3:56 p.m. (Nov. 12, 2015, 3:56 p.m.)

   Fixed. Also, the style guide should be updated to reflect that parameters/arguments must explicitly use snake-case, and to remove the spacing requirements around '=' in argument/parameter lists to be Python only.

Could you add a sentence or two in the patch descripton explaining why we are adding this?

Ship It!