Discussion:
[proj4] LRU initialization seems buggy
(too old to reply)
Jeremy Cowles
2008-08-10 20:27:14 UTC
Permalink
Raw Message
I was just playing with the cache settings, and it seems like init_lru() is
only called *sometimes*. It will be called the first time cache is created,
but then not when the cache is modified - but this is not always true
either.

To reproduce:

- setup init_lru to assign a constant value to lru, like 1 or anything other
than 0
- create a new cache: 1, 4, 16, LRU, WB
- notice only the first set gets lru initialized

Am I missing something? It seems like init_lru should be called for all
cache blocks right?

I just noticed, it seems like init_lru is being called based on the
associativity, not the number of sets. So if associativity is 2 and there
are 4 sets, it will update 2 sets instead of 4.


Thanks,
Jeremy
Jeremy Cowles
2008-08-10 20:30:28 UTC
Permalink
Raw Message
I think I found the bug, the loops are reversed in flush cache (set loop
should be on the outside, not inside):

void flush_cache()
{
int s;
int b;

/* for each unit */
for( s=0; s < assoc; s++ )
{
/* for each block */
for( b=0; b < set_count; b++ )
{
cache[b].block[s].valid = INVALID;
cache[b].block[s].dirty = VIRGIN;
init_lru(s, b);
}
}
}

Jeremy
Post by Jeremy Cowles
I was just playing with the cache settings, and it seems like init_lru() is
only called *sometimes*. It will be called the first time cache is created,
but then not when the cache is modified - but this is not always true
either.
- setup init_lru to assign a constant value to lru, like 1 or anything
other than 0
- create a new cache: 1, 4, 16, LRU, WB
- notice only the first set gets lru initialized
Am I missing something? It seems like init_lru should be called for all
cache blocks right?
I just noticed, it seems like init_lru is being called based on the
associativity, not the number of sets. So if associativity is 2 and there
are 4 sets, it will update 2 sets instead of 4.
Thanks,
Jeremy
Jeremy Cowles
2008-08-10 20:34:36 UTC
Permalink
Raw Message
I take it back, that shouldn't matter
Post by Jeremy Cowles
I think I found the bug, the loops are reversed in flush cache (set loop
void flush_cache()
{
int s;
int b;
/* for each unit */
for( s=0; s < assoc; s++ )
{
/* for each block */
for( b=0; b < set_count; b++ )
{
cache[b].block[s].valid = INVALID;
cache[b].block[s].dirty = VIRGIN;
init_lru(s, b);
}
}
}
Jeremy
Post by Jeremy Cowles
I was just playing with the cache settings, and it seems like init_lru()
is only called *sometimes*. It will be called the first time cache is
created, but then not when the cache is modified - but this is not always
true either.
- setup init_lru to assign a constant value to lru, like 1 or anything
other than 0
- create a new cache: 1, 4, 16, LRU, WB
- notice only the first set gets lru initialized
Am I missing something? It seems like init_lru should be called for all
cache blocks right?
I just noticed, it seems like init_lru is being called based on the
associativity, not the number of sets. So if associativity is 2 and there
are 4 sets, it will update 2 sets instead of 4.
Thanks,
Jeremy
[Instr] Albert Chae
2008-08-11 01:13:26 UTC
Permalink
Raw Message
No, I've just been making these changes in the past 30 minutes. I was
just about to make an announcement about memory.c. This is what we get
for working with legacy code with horrible variable names.

Albert
Wait, I sort of see the problem...the file I have has "init_lru(s, b);" whereas
the memory.c file currently in the 61c proj folder has "init_lru(b, s);"...
Umm, in the newsgroup Albert said he "updated the code" online, but I thought
he only changed formal parameters in cachelogic.c. And on the website
announcements, he never said anything about a mandatory overwrite of the file,
especially for memory.c...
I feel like I'm missing something here...
I actually made that change. The bug was in lru_init and lru_string, this is
a problem with flush_cache transposing the arguments when calling init_lru.
Can you try the test to reproduce the bug that I posted earlier (just to be
sure)?
void init_lru(int assoc_index, int block_index)
Friday night, an update was posted on the web site that the formal
parameters
within the the body of this declaration should be swapped...
Works great with that change! No need to modify memory.c.
Post by Jeremy Cowles
init_lru(s, b);
Where
s = block within the set
b = set number
init_lru(int set_number, int assoc_value);
cache[assoc_index].block[block_index].lru.value = 1;
cache[block_index].block[assoc_index].lru.value = 1;
which explains the behavior I originally posted.
init_lru(b, s)
(sorry about spamming the newsgroup)
Jeremy
Jeremy Cowles
2008-08-10 20:42:00 UTC
Permalink
Raw Message
Ok, here is the real bug in flush_cache:

init_lru(s, b);

Where
s = block within the set
b = set number

but the prototype of init_lru is:
init_lru(int set_number, int assoc_value);

So instead of this:
cache[assoc_index].block[block_index].lru.value = 1;

it does this:
cache[block_index].block[assoc_index].lru.value = 1;

which explains the behavior I originally posted.
So that line in flush cache should be:
init_lru(b, s)


(sorry about spamming the newsgroup)

Jeremy
Jeremy Cowles
2008-08-10 23:54:54 UTC
Permalink
Raw Message
I actually made that change. The bug was in lru_init and lru_string, this is
a problem with flush_cache transposing the arguments when calling init_lru.

Can you try the test to reproduce the bug that I posted earlier (just to be
sure)?
void init_lru(int assoc_index, int block_index)
Friday night, an update was posted on the web site that the formal
parameters
within the the body of this declaration should be swapped...
Works great with that change! No need to modify memory.c.
Post by Jeremy Cowles
init_lru(s, b);
Where
s = block within the set
b = set number
init_lru(int set_number, int assoc_value);
cache[assoc_index].block[block_index].lru.value = 1;
cache[block_index].block[assoc_index].lru.value = 1;
which explains the behavior I originally posted.
init_lru(b, s)
(sorry about spamming the newsgroup)
Jeremy
Loading...