> On Sept. 2, 2015, 2:02 p.m., Jason Power wrote: > > Is it not possible to delete it when the entry is set to NotPresent? It > > seems weird to delete it in the allocate function. > > Nilay Vaish wrote: > I think the problem here is with the coherence protocols maintaining two > invalid states: NP and I. > NP is supposed to be the state when the entry has not been allocated, > while in I state, the entry > is allocated but is invalid. I think this was supposed to be an > optimization that helps in avoiding > memory allocation and deallocation. > > Joel, if whatever I have said above is right, I would suggest that you > eliminate either of the states and > always allocate and deallocate entries. I think the greater problem is > SLICC's lack of constructors. > > Joel Hestness wrote: > Nilay's clarification on the original intent is correct. The problem is > that the interface for allocating entries is too loose: > > A specific problem here is that the cache entry constructor sets > permissions to NotPresent, and the allocation can occur anywhere in a .sm > file (this is a memory management issue exposed to the programmer :/). As a > result, there's no way of knowing how a protocol author might use the entry > (e.g. placing an entry in a TBE, and later moving it into the cache, and the > permission may not get updated). This allows the cache to contain allocated, > NotPresent lines that can be leaked in allocate(). > > We'd need to make a much broader change to address sources of this leak, > and that could affect a lot of downstream code. I've validated this simple > fix, and it will work with any downstream code. > > Jason Power wrote: > Ah, this makes sense to me. Thanks for clarifying. I like this fix as is.
I am in opposition to this change. Joel, I think protocols should be fixed. I am not asking you to fix them but I don't think we should go with this change either. In fact we should have an assert in the allocate() function that says (set[i] == nullptr || set[i]->permission != not present || set[i] == entry). What you are suggesting would make the optimization meaningless. - Nilay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3087/#review7090 ----------------------------------------------------------- On Sept. 2, 2015, 1:46 p.m., Joel Hestness wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3087/ > ----------------------------------------------------------- > > (Updated Sept. 2, 2015, 1:46 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11078:30183e60a824 > --------------------------- > ruby: Fix CacheMemory allocate leak > > If a cache entry permission was previously set to NotPresent, but the entry > was > not deleted, a following cache allocation can cause the entry to be leaked by > setting the entry pointer to a newly allocated entry. To eliminate this > possibility, check if the new entry is different from the old one, and if so, > delete the old one. > > > Diffs > ----- > > src/mem/ruby/structures/CacheMemory.cc 2763a59c73ff > > Diff: http://reviews.gem5.org/r/3087/diff/ > > > Testing > ------- > > > Thanks, > > Joel Hestness > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
