> On Sept. 1, 2016, 3:33 p.m., Jason Lowe-Power wrote:
> > What testing did you perform to make sure all of the protocols were
> > modified correctly?
> >
> > Most of these changes seem reasonable to me, but I know from experience
> > that even when the SLICC changes seem like they are right, if they aren't
> > tested carefully there's almost always bugs.
>
> Michael LeBeane wrote:
> The sequencer changes have been tested pretty thoroughly in a custom
> protocol; the public SLICC files only with the regression tester. I'm not
> sure how much coverage that provides for DMA other than checking if it
> compiles.
>
> Jason Lowe-Power wrote:
> I'm not sure what to do about this. Maybe others in the community will
> speak up ;).
>
> I don't feel comfortable pushing a patch with code that we know hasn't
> been tested at all. Specifically with Ruby/SLICC, this has bitten me before.
> I've updated gem5 and all of sudden my simluations are broken because someone
> changed a protocol without testing it. IMO, code needs to be tested at least
> somewhat before it's pushed into the mainline.
>
> However, I also understand that there isn't any testing infrastructure
> for most of the protocols, and we can't ask you to solve that problem before
> pushing the patch in.
>
> Do others have thoughts on this (reoccurring) problem?
>
> For this specific patch, can you run your workload that use DMA with
> protocols other than your internal protocol?
>
> Michael LeBeane wrote:
> It would take some effort, but I do have some DMA intensive networking
> benchmarks that should be able to run over the public protocols. I wouldn't
> be able to share these or add them to the regression tester (they are
> proprietary).
>
> I know that doesn't help at all with the more general problem of poor
> tester coverage, but would that be an acceptable solution for this patch?
>
> Jason Lowe-Power wrote:
> I would appreciate it if you ran those tests. It doesn't bother me nearly
> as much that they are proprietary tests. At least we'll know that something
> executed correctly with the other protocols! Improving the coverage of the
> regression tests is hugely outside of the scope for this patch.
>
> I'd still like to hear what others think about this problem in the longer
> term.
>
> Andreas Hansson wrote:
> Another potential issue: The queue of requests that has been created,
> does it officially need to be visible to any functional access? In other
> words, has it "happened" yet? For the queued ports we have functions to query
> the queued packets on a functional access, but that does not seem to be
> present in the patch.
>
> Brad Beckmann wrote:
> I think Michael is a bit unsure how to move this patch forward in a
> reasonable way. Here are a couple things to consider:
>
> - Jason, another way Michael could have approached this patch is simply
> provided the changes to the DMASequencer and not touched the public
> protocols. Limiting these DMA controllers to only a single outstanding
> request is obviously a big bottleneck, but we don't use these protocols at
> AMD. Michael really went beyond what was necessary here. If he tests the
> basically functionality of the public protocol changes, I think that is
> enough. We should minimize the burden for performance fixes to the public
> protocols.
> - Andreas, in many of your recent Ruby code reviews, you've mentioned
> concerns with functional accesses. Perhaps we should arrange a separate
> thread on the topic and clarify the current state of Ruby functional
> accesses, as well as discuss your goals. I'm pretty sure this patch is
> orthogonal to functional accesses. It is simply allowing multiple DMA
> requests to be processed by the public DMA controllers.
>
> Jason Lowe-Power wrote:
> I think this change is great, I would like to see it pushed in. I agree
> that these changes are needed to get reasonable DMA performance.
>
> I agree that's enough to just test the basic functionality with the
> public protocols. As long as these protocols have all been compiled, and any
> workload that exercises DMA is run on the protocol, I'm happy.
>
> In the past, there have been changes to Ruby protocols that weren't
> tested at all. Then, sometimes months later, users (i.e., people down the
> hall from me) have noticed that the protocols were broken, in some cases they
> didn't even compile! Digging back through hundreds of changes to find out
> what broke the protocol is incredibly time consuming, even with hg bisect.
> IMO, we should strucure our code requirements to avoid this.
>
> For now, I'm perfectly happy with just asking the question "Did you test
> these changes?" and the reviewee replying "Yes". In the future we can work on
> the regression test coverage.
>
> I agree that functional access is out of the scope here (it's already
> broken in Ruby). It is something we probably should do at some point,
> though...
>
> Let me know if this still isn't clear. I really do want to see this get
> pushed in sooner rather than later!
>
> Andreas Hansson wrote:
> I don't mind the patch going in, I merely think it would be good to
> highlight that there is _yet_ another buffer that needs to be queried on
> functional accesses.