> On Jan. 5, 2017, 8:32 a.m., Tony Gutierrez wrote:
> > Is there anything holding this up from being shipped?
>
> Andreas Hansson wrote:
> In my view the patch needs two things:
>
> 1) Some thought around the design. I am still hoping there is a less
> invasive way of accommodating the functionality, possibly with some changes
> to the existing functions of the cache.
>
> 2) A way to test it. This should preferably include both synthetic and
> real use-cases. The memtester and memchecker may be a good starting point for
> the synthetic part.
>
> It would be great if someone could dig into these issues.
It's been quite a while since I've looked at this... in fact I was going to say
that there might be a memory leak, but looking back at the patch history I see
I fixed that already in May 2015 :).
It's pretty easy to come up with a simple test case, just using C++11 atomic
ops or pthread mutexes. Unfortunately the ones I had were on my AMD work system
that I had to give back, and it looks like I missed them when I went to copy
off useful non-proprietary stuff before I left. I know I didn't spend much time
writing tests though, so they should be easy to recreate.
As far as being less invasive, I think my reply to Andreas's comment of May 7,
2015 is still relevant: given the current structure of the cache code, I think
the existing solution is really not that invasive. There isn't really that much
code being added here; most of it is comments. Generating a synthetic request
to allow allocating an MSHR so that we can accumulate the deferred operations
that happen while the lock is held is a little strange, but it allows us to
reuse the bulk of the code in recvTimingResp() with just a few changes.
If the cache code were heavily restructured, we could probably layer it in such
a way that the fake packets would not be necessary, and thus the invasiveness
of the locked RMW support could be reduced. However, I started down this path a
long time ago (shortly after his comment), and after putting a fair amount of
work into the restructuring, I still didn't even have the code working at all,
so I gave up. Thus I think the net invasiveness of restructuring the cache to
reduce the amount of change required specifically for this feature is much much
larger than the invasiveness of this patch by itself. The end result would
probably be better code, but it's a huge amount of work that I don't see anyone
signing up for right now, so I think it's better to put this change in as is
for now, and put "restructuring the cache" on the to-do list.
- Steve
---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/2691/#review9228
---
On April 14, 2016, 10:42 p.m., Steve Reinhardt wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/2691/
> ---
>
> (Updated April 14, 2016, 10:42 p.m.)
>
>
> Review request for Default.
>
>
> Repository: gem5
>
>
> Description
> ---
>
> Changeset 11444:8a1419dbbfa6
> ---
> mem: implement x86 locked accesses in timing-mode classic cache
>
> Add LockedRMW(Read|Write)(Req|Resp) commands. In timing mode,
> use a combination of clearing permission bits and leaving
> an MSHR in place to prevent accesses & snoops from touching
> a locked block between the read and write parts of an locked
> RMW sequence.
>
>
> Diffs
> -
>
> src/mem/cache/cache.cc df24b9af42c72606f1fa8e5aa0502b53e81ea176
> src/mem/cache/mshr.hh df24b9af42c72606f1fa8e5aa0502b53e81ea176
> src/mem/packet.hh df24b9af42c72606f1fa8e5aa0502b53e81ea176
> src/mem/packet.cc df24b9af42c72606f1fa8e5aa0502b53e81ea176
>
> Diff: http://reviews.gem5.org/r/2691/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Steve Reinhardt
>
>
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev
