> 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!
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. - Andreas ----------------------------------------------------------- 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 [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
