> On Sept. 2, 2015, 2:02 p.m., Jason Lowe-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 Lowe-Power wrote: > Ah, this makes sense to me. Thanks for clarifying. I like this fix as is. > > Nilay Vaish wrote: > 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. > > Joel Hestness wrote: > @Nilay: I agree it would be nice to include such an assertion, but I > don't feel it makes sense to do so. Since we can just delete the entry (the > way this patch suggests), we don't need to assertion fail if allocate() finds > a NotPresent line that is not equal to the passed entry. If we wanted to add > this assertion, we'd have to do pretty broad testing that this assertion > doesn't get triggered with any of the mainline protocols; The average gem5 > user shouldn't be responsible for dealing with this assertion. I don't feel > we should be any more aggressive with what this change tries to enforce. > > As a compromise, would you be satisfied if I added a warn_once that the > protocol contains an entry handling bug if the entry delete code is executed?
Joel, if a protocol is written incorrectly, that's that protocol's problem. I would not about protocols failing because of the assert. You don't want to add the assert, that's ok with me. Add a warn_once. But I am not ok with deleting the entries that the protocol should be deleting. - 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
