> 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.
I finally got around to testing the DMA part of the public protocols using some networking benchmarks. They seem to work fine. Any more issues that need to be addressed with this patch? - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3591/#review8698 ----------------------------------------------------------- On Aug. 30, 2016, 3:54 p.m., Michael LeBeane wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3591/ > ----------------------------------------------------------- > > (Updated Aug. 30, 2016, 3:54 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11557:5d6fcf14c77e > --------------------------- > ruby: Allow multiple outstanding DMA requests > DMA sequencers and protocols can currently only issue one DMA access at a > time. > This patch implements the necessary functionality to support multiple > outstanding DMA requests in Ruby. > > > Diffs > ----- > > src/mem/ruby/system/Sequencer.py 985d9b9a68bf20e22ba65f7398dde0193e6ca076 > src/mem/ruby/system/DMASequencer.hh > 985d9b9a68bf20e22ba65f7398dde0193e6ca076 > src/mem/ruby/system/DMASequencer.cc > 985d9b9a68bf20e22ba65f7398dde0193e6ca076 > src/mem/protocol/MOESI_hammer-dma.sm > 985d9b9a68bf20e22ba65f7398dde0193e6ca076 > src/mem/protocol/RubySlicc_Types.sm > 985d9b9a68bf20e22ba65f7398dde0193e6ca076 > src/mem/protocol/MOESI_CMP_directory-dma.sm > 985d9b9a68bf20e22ba65f7398dde0193e6ca076 > src/mem/protocol/MOESI_CMP_token-dma.sm > 985d9b9a68bf20e22ba65f7398dde0193e6ca076 > src/mem/protocol/MESI_Two_Level-dma.sm > 985d9b9a68bf20e22ba65f7398dde0193e6ca076 > src/mem/protocol/MI_example-dma.sm 985d9b9a68bf20e22ba65f7398dde0193e6ca076 > > Diff: http://reviews.gem5.org/r/3591/diff/ > > > Testing > ------- > > > Thanks, > > Michael LeBeane > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
