Thanks for this patch, it's nice to be able to model bank contention! What about blocking the port when there is a bank conflict and some number of write buffers are occupied?

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

because the are defaults all these are going to override the defaults in Caches.py when ConfigCaches.py is run. we do this other places, but it's good to know and perhaps they should be the same latency as the other defaults?

Show all issues

1. Xiangyu Dong April 16, 2013, 10:37 a.m. (April 16, 2013, 10:37 a.m.)

   This patch also makes CacheConfig.py to check these options.

Thanks for the patch. I have some high-level comments at this point, most important is how to deal with the blocking.

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

high or low?

Show all issues

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

type?

Show all issues

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

For the address striping in the bus I opted for the high bit. I don't really care too much, but it would be good to keep it consistent.

Show all issues

1. Xiangyu Dong April 16, 2013, 10:31 a.m. (April 16, 2013, 10:31 a.m.)

   Changed from lowest bit to highest bit

src/mem/cache/base.hh (Diff revision 1)

This is rather unfortunate. Please keep as much as possible private/protected

Show all issues

1. Xiangyu Dong April 16, 2013, 10:31 a.m. (April 16, 2013, 10:31 a.m.)

   changed to protected

src/mem/cache/base.hh (Diff revision 1)

const

Show all issues

src/mem/cache/base.hh (Diff revision 1)

private for the attributes

Show all issues

src/mem/cache/base.hh (Diff revision 1)

See base/addr_range for an example of the use of the bit operators.

Show all issues

src/mem/cache/base.cc (Diff revision 1)

I would suggest parentheses to make the binding more clear

Show all issues

src/mem/cache/base.cc (Diff revision 1)

fatal?

Show all issues

src/mem/cache/base.cc (Diff revision 1)

const

Show all issues

src/mem/cache/base.cc (Diff revision 1)

return inService && nextIdleTick <= curTick()

Show all issues

src/mem/cache/cache_impl.hh (Diff revision 1)

csprintf used in most other places

Show all issues

src/mem/cache/cache_impl.hh (Diff revision 1)

bank.push_back?

Show all issues

1. Xiangyu Dong April 16, 2013, 10:31 a.m. (April 16, 2013, 10:31 a.m.)

   I claimed the vector size in the initialization list, and didn't want to dynamically change the vector size.

src/mem/cache/cache_impl.hh (Diff revision 1)

I would suggest iterators. At least use bank.size() for the limit

Show all issues

1. Xiangyu Dong April 16, 2013, 10:31 a.m. (April 16, 2013, 10:31 a.m.)

   Use bank.size() now.  And also use ++i instead of i++.  I will change them to iterators later if c++11 auto type is widely used.

2. Andreas Hansson April 16, 2013, 5:50 p.m. (April 16, 2013, 5:50 p.m.)

   You can go ahead and use the auto keyword if you want. Unfortunately no range-based for loops yet as they are not supported by gcc 4.4

src/mem/cache/cache_impl.hh (Diff revision 1)

Are there other cases here?

Show all issues

1. Xiangyu Dong April 16, 2013, 10:31 a.m. (April 16, 2013, 10:31 a.m.)

   lat was originally set from the inside of accessBlock().  There are other cases here, but I leave lat untouched in those cases.  Maybe I need to rewrite accessBlock() and set lat to the correct value at the very beginning.

src/mem/cache/cache_impl.hh (Diff revision 1)

Can we not use the same method for both cases and consider the bank one of the reasons used for blocking (and thus also call unblock once the bank is available)?

Show all issues

1. Xiangyu Dong April 16, 2013, 10:31 a.m. (April 16, 2013, 10:31 a.m.)

   I considered to model it in this way. But, a busy bank only "partially" blocks a cache.  The existing block causes indicate whether a cache is entirely blocked or not.  I need to think about how to combine these two schemes.

2. Andreas Hansson April 16, 2013, 5:50 p.m. (April 16, 2013, 5:50 p.m.)

   Ok, if you want to discuss it just start a thread on the dev list.

src/mem/cache/cache_impl.hh (Diff revision 1)

I am not sure I understand this bit. Is the bank blocked until a response comes back? If so, why? Is the idea not to block while the read/write port of the SRAM is busy?

Show all issues

1. Xiangyu Dong April 16, 2013, 10:31 a.m. (April 16, 2013, 10:31 a.m.)

   Ideally, the code should unblock the bank as long as curTick() reaches bank's nextIdleTick.  However, I don't want to create a new event to do that.  Instead, I add these check-and-unblock codes at the beginning of each existing event handler.  I will add some comments here.

2. Andreas Hansson April 16, 2013, 5:50 p.m. (April 16, 2013, 5:50 p.m.)

   I would suggest we add an event (probably one per bank even).

Hi Xiangyu, Any chance of getting the cache banking/blocking patches going again? They would be a very useful addition and I don't think there's too much work involved finishing this off. Thanks in advance

Some minor concerns that hopefully can be fixed quickly. Thanks for the update.

configs/common/CacheConfig.py (Diff revision 10)

Stick to "a=b" or "a = b" throughout. This applies to the I and D cache as well.

Show all issues

configs/common/Options.py (Diff revision 10)

I'm tempted to say we group these based on L1, L2 and L3.

Any other opinions on this one?

Show all issues

1. Xiangyu Dong July 19, 2013, 2:05 p.m. (July 19, 2013, 2:05 p.m.)

   I see. But, should we have some mechanisms to let the command-line override the default setting in Caches.py?

2. Andreas Hansson July 20, 2013, 2:52 a.m. (July 20, 2013, 2:52 a.m.)

   I was just thinking a simple grouping of the l1-xyz and l2-xyz options in the Options.py

src/mem/cache/BaseCache.py (Diff revision 10)

I think we might have touched on this already, but I'm all in favour of changing this to read_latency as part of this patch.

Show all issues

src/mem/cache/BaseCache.py (Diff revision 10)

Is there a point in having these defaults if the command-line options override then anyways?

Int -> Unsigned?

Show all issues

1. Xiangyu Dong July 19, 2013, 2:05 p.m. (July 19, 2013, 2:05 p.m.)

   I see. You prefer to set the default in Caches.py instead of here, right?  If so, I can move these default settings to Caches.py
   
   I checked other parameters, like mshrs, write_buffers.  They all use Int.  So, I also use Int to make them look consistent.

2. Andreas Hansson July 20, 2013, 2:52 a.m. (July 20, 2013, 2:52 a.m.)

   I'd say: lead the way and do it properly.
   
   There is no reason any of the former should be Ints (as far as I am aware at least).

src/mem/cache/BaseCache.py (Diff revision 10)

Unsigned

Show all issues

1. Xiangyu Dong July 19, 2013, 2:05 p.m. (July 19, 2013, 2:05 p.m.)

   Same above

src/mem/cache/base.hh (Diff revision 10)

A line or two per method (in doxygen format) would be greatly appreciated.

Show all issues

src/mem/cache/base.hh (Diff revision 10)

I'd say do the rename as part of this patch.

Anyone else got opinions on this one?

Show all issues

1. Xiangyu Dong July 19, 2013, 2:05 p.m. (July 19, 2013, 2:05 p.m.)

   I've renamed all hitLatency to readLatency (except for tag and ruby model).

2. Andreas Hansson July 20, 2013, 2:52 a.m. (July 20, 2013, 2:52 a.m.)

   Thanks!

src/mem/cache/base.hh (Diff revision 10)

Already do this once in the constructor.

Show all issues

1. Xiangyu Dong July 19, 2013, 2:05 p.m. (July 19, 2013, 2:05 p.m.)

   made bankIntlvBits a instance variable

src/mem/cache/base.hh (Diff revision 10)

I would suggest to precompute the mask and shift in the constructor as I imagine this will be called a lot.

Any idea what this patch does to the simulator performance btw?

Show all issues

1. Xiangyu Dong July 19, 2013, 2:05 p.m. (July 19, 2013, 2:05 p.m.)

   add bankIntlvLowBit and bankIntlvMask const variable. and use them to do the operation.

src/mem/cache/cache_impl.hh (Diff revision 10)

should officially be bank_id (as it is a local variable)

Show all issues

src/mem/cache/cache_impl.hh (Diff revision 10)

This is fine, just wanted to point out that you can also do:

for (auto b = cache->bank.begin(); b != cache->bank.end(); ++b)
 if ((*b)->serviceDone()) etc

The unfortunate bit here is that we iterate over all the banks for every cache access. I don't know what the performance impact is, but it worries me a bit.

The benefit of having events per bank is that they only do what they have to, and only when they have to.

Show all issues

1. Xiangyu Dong July 19, 2013, 2:05 p.m. (July 19, 2013, 2:05 p.m.)

   I think usually bank number won't beyond 8 or 16. In most cases, I expect it to be 2 or 4.  So, the performance impact shall be negligible.
   
   But, I agree that these codes should eventually be converted to event-driven.
   
   Shall we delay the bank event queue to a later patch? I'm afraid it might be too much for this patch.

src/mem/cache/cache_impl.hh (Diff revision 10)

if (blocked) ?

Show all issues

1. Xiangyu Dong July 19, 2013, 2:05 p.m. (July 19, 2013, 2:05 p.m.)

   you are right. they are equivalent.

2. Andreas Hansson July 20, 2013, 2:52 a.m. (July 20, 2013, 2:52 a.m.)

   Yeah I just figured if (blocked) is more clear as that is really what you want to test (as far as I understand)

src/mem/cache/cache_impl.hh (Diff revision 10)

80 char

Show all issues

src/mem/cache/cache_impl.hh (Diff revision 10)

Same concern as above. I'd suggest to add an event per bank to do the clearing.

Show all issues

src/mem/cache/cache_impl.hh (Diff revision 11)

Why is this here and not in base.cc?

Alternatively, move all the things from base.hh to cache.hh, I honestly do not know which is more suitable. I would actually guess the latter.

Show all issues

1. Xiangyu Dong July 26, 2013, 5:20 p.m. (July 26, 2013, 5:20 p.m.)

   you are right. I moved it to base.cc

src/mem/cache/cache_impl.hh (Diff revision 11)

Same as above. How come this is not in base.cc?

Show all issues

src/mem/cache/cache_impl.hh (Diff revision 11)

Will this not create problems?

If there is a retry event scheduled, then surely we should not get another attempt at giving us a new packet. If we "sink" the second request then there will only be a single retry and the protocol would break down. Should this not say assert(!sendRetryEvent.scheduled()); and then schedule a send retry?

Show all issues

1. Xiangyu Dong July 26, 2013, 5:20 p.m. (July 26, 2013, 5:20 p.m.)

   I'm not 100% familiar with the packet protocol.  I will convert this code from "if (!scheduled) scheduleRetry;" to "assert(!scheduled); scheduleRetry;"  and see if the assertion fails.

2. Xiangyu Dong July 31, 2013, 2:58 p.m. (July 31, 2013, 2:58 p.m.)

   I ran a local regression test with SPEC2006 workloads for several days, no assertion failure.

A few more things that are popping up. Thanks for all the updates. It would be great if Ali or Steve could also have a look before we ship this.

Almost there :-)

Do all the normal regressions pass (with the 1 bank default)? If so, do you know what the performance impact is?

configs/common/Caches.py (Diff revision 12)

Could we not let num_banks be 1 by default and leave this out here?

Similary so for the interleaving bit. Make it 0 by default and if num_banks is 1 then it is never needed.

Show all issues

1. Xiangyu Dong Aug. 1, 2013, 12:08 a.m. (Aug. 1, 2013, 12:08 a.m.)

   I think I have some confusion here.  If the command-line never explicitly specify a parameter value, where is the standard place we set the default value in the code?  In BaseCache.py?

2. Andreas Hansson Aug. 1, 2013, 12:47 a.m. (Aug. 1, 2013, 12:47 a.m.)

   Yes I was thinking BaseCache.py

configs/common/Options.py (Diff revision 12)

Could you add "(cycles)" at the end?

Show all issues

1. Xiangyu Dong Aug. 1, 2013, 12:08 a.m. (Aug. 1, 2013, 12:08 a.m.)

   added

configs/common/Options.py (Diff revision 12)

I am not sure it's worth removing this functionality, but would we ever want to interleave on anything besides cache line granularity?

Show all issues

1. Xiangyu Dong Aug. 1, 2013, 12:08 a.m. (Aug. 1, 2013, 12:08 a.m.)

   I will set the default to 0, and 0 means "automatically" set the interleaving bit to cache line granularity.  The user can override this decision by setting this parameter.

2. Andreas Hansson Aug. 1, 2013, 12:47 a.m. (Aug. 1, 2013, 12:47 a.m.)

   Great. Thanks!

src/mem/cache/BaseCache.py (Diff revision 12)

See previous remark. I suggest making this 1 by default

Show all issues

src/mem/cache/base.hh (Diff revision 12)

perhaps add a sentence that this is to model contention and does not actually hold any data

Show all issues

src/mem/cache/base.hh (Diff revision 12)

port -> bank

Show all issues

src/mem/cache/base.cc (Diff revision 12)

I would suggest p->bank_intlv_high_bit ? p->bank_intlv_high_bit : ceilLog2(blkSize) + bankIntlvBits

Show all issues

1. Xiangyu Dong Aug. 1, 2013, 12:08 a.m. (Aug. 1, 2013, 12:08 a.m.)

   I see. But, it should be "ceilLog2(blkSize) + bankIntlvBits - 1", right?

2. Andreas Hansson Aug. 1, 2013, 12:47 a.m. (Aug. 1, 2013, 12:47 a.m.)

   My bad, should have had my morning coffee first.

src/mem/cache/base.cc (Diff revision 12)

The name is a bit misleading, is it not?

Show all issues

1. Xiangyu Dong Aug. 1, 2013, 12:08 a.m. (Aug. 1, 2013, 12:08 a.m.)

   Combine this function with clearInService(), and rename it to checkAndUnmarkInService()

src/mem/cache/cache_impl.hh (Diff revision 12)

else? fatal? lat = readLatency?

I would think if pkt->isWrite(), lat = writeLatency, and in all other cases lat = readLatency.

Show all issues

1. Xiangyu Dong Aug. 1, 2013, 12:08 a.m. (Aug. 1, 2013, 12:08 a.m.)

   Here is the part I really want to discuss with you more.  What does &lat returned from tags->accessBlock() mean?  I can see most of the time it returns a readLatency, but in some cases, cache->ticksToCycles(blk->whenReady - curTick()) is returned.
   
   I'm not sure how it exactly works.  My understanding is: the "lat" returned from tags->accessBlock() should be the "tag lookup latency", and depending on whether we access tag and data in parallel or not, the final latency should be max(lat, read(write)Latency) or lat+read(write)Latency.

2. Andreas Hansson Aug. 1, 2013, 12:47 a.m. (Aug. 1, 2013, 12:47 a.m.)

   It would be good to get some input from e.g. Steve here as I am not too familiar with the intention here.
   
   What you say makes a lot of sense to me, and the accessBlock latency calculation should probably be changed accordingly. Will you do so (possibly in a separate patch if this is getting too much)?

src/mem/cache/cache_impl.hh (Diff revision 12)

Do outside the if/else?

Show all issues

src/mem/cache/cache_impl.hh (Diff revision 12)

I would really consider this a todo and a next follow-on patch

Show all issues

1. Xiangyu Dong Aug. 1, 2013, 12:08 a.m. (Aug. 1, 2013, 12:08 a.m.)

   add a "@todo" here about the future event-driven implementation

src/mem/cache/cache_impl.hh (Diff revision 12)

bank_busy?

Show all issues

src/mem/cache/cache_impl.hh (Diff revision 12)

..., wait until the cache is unblocked and then send a retry

Show all issues

src/mem/cache/cache_impl.hh (Diff revision 12)

Could it be extended?

Show all issues

1. Xiangyu Dong Aug. 1, 2013, 12:08 a.m. (Aug. 1, 2013, 12:08 a.m.)

   As you point out, this could happen.  extendService() function is a workaround in case we cannot block incoming traffic from memory side during bank busy.  We need to apply another patch (#1826) to block the cache installation (from memory side) when bank is busy, and after that, extendService() function can be removed.

2. Andreas Hansson Aug. 1, 2013, 12:47 a.m. (Aug. 1, 2013, 12:47 a.m.)

   Ok, what I was a bit nervous about was that we schedule the retry based on what we see right now. Then if the service is extended the cache is still blocked. This is highlighting that the event-based unblocking is the way to go. I am not sure it would ever cause problems, but it is confusing that we schedule a retry (and claim that we know when it is free), and then later on it is not free when we say go. Does that make sense?