> On March 4, 2016, 5:13 p.m., Steve Reinhardt wrote:
> > Overall I think this specialization is good. I think the tuple return
> > thing is very clunky though. It looks like getNextQueueEntry() is only
> > called from getTimingPacket(), which in turn is only called from
> > sendDeferredPacket(). Also a lot of getTimingPacket() is basically
> >
> > if (mshr) {
> > // do a bunch of stuff
> > } else {
> > assert(wq_entry);
> > // do a bunch of other stuff
> > }
> >
> > So I think this whole call stack structure really only made sense because
> > of the unification of the write buffer and MSHR objects, and now it no
> > longer makes sense, and the tuple return values are a symptom of that.
> >
> > I think if we're going to bother to do this specialization, we really need
> > to refactor this piece of code avoid this pointless merging and forking of
> > the MSHR vs write buffer code paths that leads to these tuple return
> > values. In cases where the code paths are merged and we still want to do
> > something different between the two cases, we should try and do that by
> > defining appropriate virtual functions on QueueEntry, and perhaps (if
> > necessary) creating a common base class for the Queue<> classes and
> > defining some virtual functions on that.
>
> Andreas Hansson wrote:
> I am also not a huge fan of the tuple approach, but the only sensible
> solution would be to avoid calling the method, and instead merging these
> chains.
>
> I really do not like the idea of making the queue entry clever. I'd
> prefer if both the write queue entry and MSHR was as close to a struct as
> possible. The code is already challenging to follow, and splitting
> functionality into cache, MSHR and write queue entry is a recipee for
> disaster imho.
>
> Any other ideas of how to tidy this up?
>
> Andreas Hansson wrote:
> I am keen to get this patch finished up and pushed as there are follow-on
> patches depending on it, including a number of simplifications.
>
> Steve, are you happy with it as is?
>
> Steve Reinhardt wrote:
> Sorry for the delayed response... I've been meaning to take a closer look
> and provide a more concrete suggestion but I was traveling and didn't have a
> chance until just now.
>
> On closer inspection, the current structure really is pointless.
> getTimingPacket() is really two different functions now split by the if
> (mshr) { ... } else { assert(wq_entry); ... }. I'm sure there are multiple
> ways this could be cleaned up, but here's one that comes to mind:
>
> - Take what's inside the 'if (mshr) { ... }' part of getTimingPacket and
> make a new function out of it:
> PacketPtr makePacketFromMSHR(MSHR *) { ... }
> - Take what's inside the 'else { assert(wq_entry) ... }' part of
> getTimingPacket and make a new function out of that:
> PacketPtr makePacketFromWriteBuffer(WriteQueueEntry *) { ... }
> - Change the signature of getNextQueueEntry to return a PacketPtr (might
> even want to rename it getNextPacket())
> - On every return statement in getNextQueueEntry/getNextPacket:
> - if it's of the form 'return std::make_tuple(mshr, nullptr)' change it
> to 'return makePacketFromMSHR(mshr)'
> - if it's of the form 'return std::make_tuple(nullptr, wq_entry)'
> change it to 'return makePacketFromWriteBuffer(wq_entry)'
> - Back in sendDeferredPacket(), replace the old line
> MSHR *mshr = dynamic_cast<MSHR*>(pkt->senderState);
> with
> QueueEntry *q_entry = dynamic_cast<QueueEntry*>(pkt->senderState);
> - Define a virtual function on QueueEntry so you can do
> bool delete_pkt = q_entry->deletePkt();
> - Define another virtual function on QueueEntry so further down you can do
> q_entry->markInService(cache, pending_modified_resp);
>
> I expect you may not like the last three items, but I don't see the
> virtual functions as making the queue entries "clever", it's really just
> using virtual function dispatch to more cleanly implement switching based on
> a type. I hope you'll agree that the first four items constitute an
> unequivocal improvement though.
>
> Andreas Hansson wrote:
> I can play around with the functions and how they interlink. That said,
> I'd rather deal with the tuple than the dynamic_cast. Also, the MSHR and
> WriteQueueEntry already have a markInService that is called by the
> markInService on the MSHRQueue and WriteQueue, so things would get very
> convoluted if we add another function on the entry, calling the queue,
> calling the entry. I'll have a think around this, but I really don't want to
> confuse things unecessarily.
>
> Steve Reinhardt wrote:
> I'd be fine with having getNextQueueEntry() return a tuple<PacketPtr,
> QueueEntry*>... the clarity and the avoidance of the dynamic_cast outweighs
> the redundancy of explicitly returning something that's already available via
> pkt->senderState().
>
> Basically the entry and the queue both have markInService() code that
> needs to be executed, but I'm not sure that it matters which one calls which.
> Currently the queue function calls the entry function, but it could go the
> other way around.
>
> Andreas Hansson wrote:
> So this is proving more challenging than what I hoped. markInService
> ultimately needs to be called on the cache, and it has different signature
> and behaviour for MSHRs and write queue entries. Even if the entry could call
> the queue, the queue has no sensible way of calling the cache. Also the flow
> from entry to queue to cache seems very contrived to me.
>
> I am afraid I do not really see a sensible way of avoiding the current
> structure.
>
> Andreas Hansson wrote:
> Btw, I am also not too happy with the tuples, but I don't want to replace
> one "ugly" construct with another one, especially as I find that the current
> ugliness at least is easy to understand and follow.
>
> Steve Reinhardt wrote:
> Sorry for the delay again... I was traveling and then caught a cold on
> the trip. Don't mean to string you along like this.
>
> So one thing I wonder is whether we're now just trying to hard to force
> the new reality into the historical framework... yea, there are a lot of
> parallels in what happens whether you're sending out a miss from the MSHRs or
> a write from the write buffer, but when you come down to it, we're forcing
> the two different cases down the same code path just so they can share:
> 1. The snoop response queue check
> 2. The call to masterPort.sendTimingReq()
> 3. The event resched if the send is successful and there are more packets
> to go
>
> Maybe instead of trying to reconverge the execution before sending the
> packet, we should take the initial steps I outlined above, only instead of
> calling makePacketFromMSHR()/makePacketFromWriteBuffer(), we should replace
> those functions with sendPacketFromMSHR()/sendPacketFromWriteBuffer(), which
> incorporate some or all of the code after the call to getTimingPacket() in
> sendDeferredPacket() as well. (And we'd probably want to rename
> getTimingPacket() to sendTimingPacket().) We could put the snoop response
> queue check (item 1 above) in a function that gets called from both places to
> reduce redundancy. Item 2 is just one line so we could live with replicating
> it. Item 3 might be something we could leave in sendDeferredPacket(),
> assuming we return the success/waitingOnRetry flag from the new
> sendTimingPacket().
>
> Does that make sense? There's some high-level loss of elegance in that
> conceptually similar things are now being done in different functions, but
> there's no need to force-fit different things into the same code path at all.
Have a look. I'm keen to move on to the follow-on patches, so the sooner we can
converge on this the better.
- Andreas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3346/#review8054
-----------------------------------------------------------
On March 14, 2016, 12:46 a.m., Andreas Hansson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3346/
> -----------------------------------------------------------
>
> (Updated March 14, 2016, 12:46 a.m.)
>
>
> Review request for Default.
>
>
> Repository: gem5
>
>
> Description
> -------
>
> Changeset 11369:ebe8d4868008
> ---------------------------
> mem: Create a separate class for the cache write buffer
>
> This patch breaks out the cache write buffer into a separate class,
> without affecting any stats. The goal of the patch is to avoid
> encumbering the much-simpler write queue with the complex MSHR
> handling. In a follow on patch this simplification allows us to
> implement write combining.
>
> The WriteQueue gets its own class, but shares a common ancestor, the
> generic Queue, with the MSHRQueue. To distinguish between
> WriteQueueEntries and MSHRs in the cache, a number of functions are
> returning C++11 tuples as opposed to a single value.
>
>
> Diffs
> -----
>
> src/mem/cache/SConscript 2fd64ea0a7cb
> src/mem/cache/base.hh 2fd64ea0a7cb
> src/mem/cache/base.cc 2fd64ea0a7cb
> src/mem/cache/cache.hh 2fd64ea0a7cb
> src/mem/cache/cache.cc 2fd64ea0a7cb
> src/mem/cache/mshr.hh 2fd64ea0a7cb
> src/mem/cache/mshr.cc 2fd64ea0a7cb
> src/mem/cache/mshr_queue.hh 2fd64ea0a7cb
> src/mem/cache/mshr_queue.cc 2fd64ea0a7cb
> src/mem/cache/queue.hh PRE-CREATION
> src/mem/cache/queue_entry.hh PRE-CREATION
> src/mem/cache/write_queue.hh PRE-CREATION
> src/mem/cache/write_queue.cc PRE-CREATION
> src/mem/cache/write_queue_entry.hh PRE-CREATION
> src/mem/cache/write_queue_entry.cc PRE-CREATION
>
> Diff: http://reviews.gem5.org/r/3346/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andreas Hansson
>
>
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
