> 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.
@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 ----------------------------------------------------------- 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
