> 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? > > Nilay Vaish wrote: > 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. > > Joel Hestness wrote: > I see. Are you worried that a protocol that is mishandling entries might > also be maintaining other pointers to the entry in the cache, so deleting > them may cause protocol memory bugs? I can buy that. If so, I'll just add the > warn_once instead of deletion and update the review request.
You got it right. I should have been more clear that the responsibility of deletion lies with the protocol since it is doing the allocation. We cannot predict when is the right time to carry out deletions. No need to repost. - 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
