Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test
On Wed, 29 Apr 2015, Beckmann, Brad wrote: My main objection to the change is that it is not worth the time. It is taking a sledgehammer to a bug that only requires a minor tweak. There is a lot of downstream code that will be impacted by a change that doesn't provide any real benefit. To do what you want the right way, would require making the CheckTable and RubyTesters separate SimObjects and then instantiating them appropriately. Why go through all that trouble? I am willing to go through all the trouble because I am confident that the right way is to have multiple testers and not one single tester. Anyways, there are two questions that come to my mind here. First, let's for an instant assume that we do not want to change ruby tester's c++ code. How do you propose we fix the current bug? Since you have not posted any code, I am discussing what I think would be done. We will have to make each protocol configuration file understand that it cannot assume that the number of cpu objects is same as options.num_cpus and do something different when the two quantities are unequal. Do we want to put that down as a policy? Don't you think this renders the option meaningless? Second, this question is more general and should be discussed more widely. When is a change deemed as 'impacting downstream code'? And what should be our policy for such changes? What separate code are you concerned about? The specific code to handle the tester in Ruby (C++) has nothing to do with whether there is one RubyTester or multiple RubyTesters. May be it has nothing to do with single or multiple testers. But you agree that we have code where we behave differently if a tester is running. And I do not want such code to proliferate. -- Nilay ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test
Replies below: -Original Message- From: gem5-dev [mailto:gem5-dev-boun...@gem5.org] On Behalf Of Nilay Vaish Sent: Thursday, April 30, 2015 8:20 AM To: gem5 Developer List Subject: Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test On Wed, 29 Apr 2015, Beckmann, Brad wrote: My main objection to the change is that it is not worth the time. It is taking a sledgehammer to a bug that only requires a minor tweak. There is a lot of downstream code that will be impacted by a change that doesn't provide any real benefit. To do what you want the right way, would require making the CheckTable and RubyTesters separate SimObjects and then instantiating them appropriately. Why go through all that trouble? I am willing to go through all the trouble because I am confident that the right way is to have multiple testers and not one single tester. Anyways, there are two questions that come to my mind here. First, let's for an instant assume that we do not want to change ruby tester's c++ code. How do you propose we fix the current bug? Since you have not posted any code, I am discussing what I think would be done. We will have to make each protocol configuration file understand that it cannot assume that the number of cpu objects is same as options.num_cpus and do something different when the two quantities are unequal. Do we want to put that down as a policy? Don't you think this renders the option meaningless? [BB] This is where I don't fully understand the bug you are encountering. The number of cpus is different than the number of testers. The number of cpus should impact the number of m5 ports provided by the tester, not the number of testers. That is how the tester has worked in the past. If the current configurations don't support that, then they need to be fixed. Second, this question is more general and should be discussed more widely. When is a change deemed as 'impacting downstream code'? And what should be our policy for such changes? [BB] When new features are added to the code, then one should expect that downstream code should be impacted. However, a simple problem in configuration should not require one to change how ruby testers are architected. More ruby testers exist than what is currently available in the public tree. What separate code are you concerned about? The specific code to handle the tester in Ruby (C++) has nothing to do with whether there is one RubyTester or multiple RubyTesters. May be it has nothing to do with single or multiple testers. But you agree that we have code where we behave differently if a tester is running. And I do not want such code to proliferate. [BB] I think the fundamental problem there is the too simplistic RubyPort interface. There are several problems that could be fixed if we allowed a richer set of information to be passed between Ruby and the packet generators (CPUs/testers/GPUs/etc.). -- Nilay ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test
On Tue, 28 Apr 2015, Beckmann, Brad wrote: The tester has always been a single object (since 1999!). The tester works in a coordinated fashion to instigate races. It does not operate as separate independent objects. Brad, even if it has been a single object for a long time, I still believe it is not the right way. If we have multiple objects, we should still be able to make them work in a coordinated fashion by letting them share some portion of memory. I did this by marking the CheckTable static, but I am willing to make it common to every tester object in a different fashion. Can you explain what may prevent us from coordinating multiple tester objects? Again, I do not want to create separate code for testing and for simulation. There are several places in ruby where we do this and I think we should do away with them, instead of further spreading this approach. -- Nilay ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test
My main objection to the change is that it is not worth the time. It is taking a sledgehammer to a bug that only requires a minor tweak. There is a lot of downstream code that will be impacted by a change that doesn't provide any real benefit. To do what you want the right way, would require making the CheckTable and RubyTesters separate SimObjects and then instantiating them appropriately. Why go through all that trouble? What separate code are you concerned about? The specific code to handle the tester in Ruby (C++) has nothing to do with whether there is one RubyTester or multiple RubyTesters. Brad -Original Message- From: gem5-dev [mailto:gem5-dev-boun...@gem5.org] On Behalf Of Nilay Vaish Sent: Wednesday, April 29, 2015 1:35 PM To: gem5 Developer List Subject: Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test On Tue, 28 Apr 2015, Beckmann, Brad wrote: The tester has always been a single object (since 1999!). The tester works in a coordinated fashion to instigate races. It does not operate as separate independent objects. Brad, even if it has been a single object for a long time, I still believe it is not the right way. If we have multiple objects, we should still be able to make them work in a coordinated fashion by letting them share some portion of memory. I did this by marking the CheckTable static, but I am willing to make it common to every tester object in a different fashion. Can you explain what may prevent us from coordinating multiple tester objects? Again, I do not want to create separate code for testing and for simulation. There are several places in ruby where we do this and I think we should do away with them, instead of further spreading this approach. -- Nilay ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test
On Mon, 27 Apr 2015, Brad Beckmann wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2749/#review6088 --- Responding to Nilay's comments over email: I think it would be best if you explained the initial bug that led to this patch. I think the explanation in the original message is clear enough. The coherence protocols expect there are multiple cpus when options.num_cpus 1. Currently a single tester is created even when options.num_cpus 1. This is the bug. Going forward, the tester will need to work with protocols that do not expect simply a number of CPUs. There will be controllers that only generate instruction fetches, and other controllers that support shared i-caches and separate d-caches. In these protocols, understanding the number of CPUs is not good enough. The tester will need to be more flexible handling ports. I don't follow what you want to say here. I think it would really good if you post your patch and we then decide which code is better. -- Nilay ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test
--- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2749/#review6091 --- Brad's comment aside, is there really a need for a Ruby-specific tester of this kind? Is it not generic enough to be used with any memory system? - Andreas Hansson On April 27, 2015, 7:23 p.m., Nilay Vaish wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2749/ --- (Updated April 27, 2015, 7:23 p.m.) Review request for Default. Repository: gem5 Description --- Changeset 10811:b826d3a54c0c --- cpu: testers: rubytest: fix the test The test was broken. When run with more than a cpu, it resulted in error while configuring the system. Coherence protocols assume that multiple cpus are present when options.num_cpus 1. But the test only creates one tester even when we were testing a multicpu system. Now, we will create individual testers for each cpu. Diffs - src/cpu/testers/rubytest/RubyTester.hh df2aa91dba5b src/cpu/testers/rubytest/RubyTester.cc df2aa91dba5b src/cpu/testers/rubytest/RubyTester.py df2aa91dba5b tests/configs/rubytest-ruby.py df2aa91dba5b configs/example/ruby_random_test.py df2aa91dba5b src/cpu/testers/rubytest/Check.hh df2aa91dba5b src/cpu/testers/rubytest/Check.cc df2aa91dba5b src/cpu/testers/rubytest/CheckTable.hh df2aa91dba5b src/cpu/testers/rubytest/CheckTable.cc df2aa91dba5b Diff: http://reviews.gem5.org/r/2749/diff/ Testing --- Thanks, Nilay Vaish ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test
On Mon, 27 Apr 2015, Steve Reinhardt wrote: I appreciate Nilay's desire to not have the tester configuration diverge from the simulation configuration. However, the general impression I get here is that we're making the C++ more complicated in order to avoid changes to the Python. Given that the point of putting the configuration in python was to enable additional flexibility, this direction (at least superficially) seems completely backwards. If we want to have common code that sets up Ruby configurations independent of whether it will be supporting various objects representing compute devices (CPUs/GPUs/whatever) or various ports on a single tester object, then IMO the right approach is to find a way to make the Python more modular, not to do unnatural things in the C++. Why have a single tester object? -- Nilay ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test
The tester has always been a single object (since 1999!). The tester works in a coordinated fashion to instigate races. It does not operate as separate independent objects. As for Andreas's question, one can certainly try to use the ruby tester with the classic memory model. It does not need to be Ruby specific, but it is quite powerful. Brad -Original Message- From: gem5-dev [mailto:gem5-dev-boun...@gem5.org] On Behalf Of Nilay Vaish Sent: Tuesday, April 28, 2015 11:50 AM To: gem5 Developer List Subject: Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test On Mon, 27 Apr 2015, Steve Reinhardt wrote: I appreciate Nilay's desire to not have the tester configuration diverge from the simulation configuration. However, the general impression I get here is that we're making the C++ more complicated in order to avoid changes to the Python. Given that the point of putting the configuration in python was to enable additional flexibility, this direction (at least superficially) seems completely backwards. If we want to have common code that sets up Ruby configurations independent of whether it will be supporting various objects representing compute devices (CPUs/GPUs/whatever) or various ports on a single tester object, then IMO the right approach is to find a way to make the Python more modular, not to do unnatural things in the C++. Why have a single tester object? -- Nilay ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test
On Mon, 27 Apr 2015, Brad Beckmann wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2749/#review6086 --- Why make such a large and ugly change to fix a seemingly simple problem in configuration? The Ruby Tester has always been a single tester, with multiple ports, rather than a set of testers. This patch makes a set of testers, but then uses an ugly combination of static variables and conditions to fall back on the single tester logic. I knowingly did not change the configuration files. If we create multiple cpus when we run actual simulations, why not create multiple testers? I do not want to add checks to python configuration files that change behavior of the code when a tester is being invoked. I think this difference in behavior is the reason why bugs creep into the testers concerned with ruby. As far as use of a static variable is concerned, since the original code always created a single tester variable, I do not see why a static variable is a problem. If you are still not convinced, I can convert the check table to a sim object and add a pointer to it to the tester python object. We have a patch in our patch queue that we will soon post that I believe fixes this problem in a much better way. I will not believe your claim unless I see the code myself. And if your patch puts in a check that the code should behave differently when a tester is invoked, in my opinion, that is not better at all. src/cpu/testers/rubytest/RubyTester.hh (line 140) http://reviews.gem5.org/r/2749/#comment5292 We should avoid static variables. At some point, we could be interested in running multiple instances of Ruby with the random tester. Since ruby_random_test.py anyway did not used to create more than one tester, I do not see why having a static variable is problem. There are several things that would need to change in ruby to run multiple instances. src/cpu/testers/rubytest/RubyTester.cc (line 96) http://reviews.gem5.org/r/2749/#comment5293 This is pretty ugly hack. The tester is not meant to be created per cpu. There is a single tester and check table that is used to coordinate the races. Again if we are creating multiple cpus when we run simulations, we should be creating multiple testers. I do not want to test in a fashion different from the fashion in which we run actual simulations. -- Nilay ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test
--- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2749/#review6086 --- Why make such a large and ugly change to fix a seemingly simple problem in configuration? The Ruby Tester has always been a single tester, with multiple ports, rather than a set of testers. This patch makes a set of testers, but then uses an ugly combination of static variables and conditions to fall back on the single tester logic. We have a patch in our patch queue that we will soon post that I believe fixes this problem in a much better way. src/cpu/testers/rubytest/RubyTester.hh (line 140) http://reviews.gem5.org/r/2749/#comment5292 We should avoid static variables. At some point, we could be interested in running multiple instances of Ruby with the random tester. src/cpu/testers/rubytest/RubyTester.cc (line 96) http://reviews.gem5.org/r/2749/#comment5293 This is pretty ugly hack. The tester is not meant to be created per cpu. There is a single tester and check table that is used to coordinate the races. - Brad Beckmann On April 27, 2015, 7:23 p.m., Nilay Vaish wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2749/ --- (Updated April 27, 2015, 7:23 p.m.) Review request for Default. Repository: gem5 Description --- Changeset 10811:b826d3a54c0c --- cpu: testers: rubytest: fix the test The test was broken. When run with more than a cpu, it resulted in error while configuring the system. Coherence protocols assume that multiple cpus are present when options.num_cpus 1. But the test only creates one tester even when we were testing a multicpu system. Now, we will create individual testers for each cpu. Diffs - src/cpu/testers/rubytest/RubyTester.hh df2aa91dba5b src/cpu/testers/rubytest/RubyTester.cc df2aa91dba5b src/cpu/testers/rubytest/RubyTester.py df2aa91dba5b tests/configs/rubytest-ruby.py df2aa91dba5b configs/example/ruby_random_test.py df2aa91dba5b src/cpu/testers/rubytest/Check.hh df2aa91dba5b src/cpu/testers/rubytest/Check.cc df2aa91dba5b src/cpu/testers/rubytest/CheckTable.hh df2aa91dba5b src/cpu/testers/rubytest/CheckTable.cc df2aa91dba5b Diff: http://reviews.gem5.org/r/2749/diff/ Testing --- Thanks, Nilay Vaish ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test
I appreciate Nilay's desire to not have the tester configuration diverge from the simulation configuration. However, the general impression I get here is that we're making the C++ more complicated in order to avoid changes to the Python. Given that the point of putting the configuration in python was to enable additional flexibility, this direction (at least superficially) seems completely backwards. If we want to have common code that sets up Ruby configurations independent of whether it will be supporting various objects representing compute devices (CPUs/GPUs/whatever) or various ports on a single tester object, then IMO the right approach is to find a way to make the Python more modular, not to do unnatural things in the C++. I haven't looked at the code in question, and in fact I'm not all that involved with the Ruby work here at AMD right now, so this is just a high-level comment based on reading this thread. Steve On Mon, Apr 27, 2015 at 3:26 PM, Brad Beckmann brad.beckm...@amd.com wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2749/#review6088 --- Responding to Nilay's comments over email: I think it would be best if you explained the initial bug that led to this patch. Going forward, the tester will need to work with protocols that do not expect simply a number of CPUs. There will be controllers that only generate instruction fetches, and other controllers that support shared i-caches and separate d-caches. In these protocols, understanding the number of CPUs is not good enough. The tester will need to be more flexible handling ports. - Brad Beckmann On April 27, 2015, 7:23 p.m., Nilay Vaish wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2749/ --- (Updated April 27, 2015, 7:23 p.m.) Review request for Default. Repository: gem5 Description --- Changeset 10811:b826d3a54c0c --- cpu: testers: rubytest: fix the test The test was broken. When run with more than a cpu, it resulted in error while configuring the system. Coherence protocols assume that multiple cpus are present when options.num_cpus 1. But the test only creates one tester even when we were testing a multicpu system. Now, we will create individual testers for each cpu. Diffs - src/cpu/testers/rubytest/RubyTester.hh df2aa91dba5b src/cpu/testers/rubytest/RubyTester.cc df2aa91dba5b src/cpu/testers/rubytest/RubyTester.py df2aa91dba5b tests/configs/rubytest-ruby.py df2aa91dba5b configs/example/ruby_random_test.py df2aa91dba5b src/cpu/testers/rubytest/Check.hh df2aa91dba5b src/cpu/testers/rubytest/Check.cc df2aa91dba5b src/cpu/testers/rubytest/CheckTable.hh df2aa91dba5b src/cpu/testers/rubytest/CheckTable.cc df2aa91dba5b Diff: http://reviews.gem5.org/r/2749/diff/ Testing --- Thanks, Nilay Vaish ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test
--- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2749/#review6088 --- Responding to Nilay's comments over email: I think it would be best if you explained the initial bug that led to this patch. Going forward, the tester will need to work with protocols that do not expect simply a number of CPUs. There will be controllers that only generate instruction fetches, and other controllers that support shared i-caches and separate d-caches. In these protocols, understanding the number of CPUs is not good enough. The tester will need to be more flexible handling ports. - Brad Beckmann On April 27, 2015, 7:23 p.m., Nilay Vaish wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2749/ --- (Updated April 27, 2015, 7:23 p.m.) Review request for Default. Repository: gem5 Description --- Changeset 10811:b826d3a54c0c --- cpu: testers: rubytest: fix the test The test was broken. When run with more than a cpu, it resulted in error while configuring the system. Coherence protocols assume that multiple cpus are present when options.num_cpus 1. But the test only creates one tester even when we were testing a multicpu system. Now, we will create individual testers for each cpu. Diffs - src/cpu/testers/rubytest/RubyTester.hh df2aa91dba5b src/cpu/testers/rubytest/RubyTester.cc df2aa91dba5b src/cpu/testers/rubytest/RubyTester.py df2aa91dba5b tests/configs/rubytest-ruby.py df2aa91dba5b configs/example/ruby_random_test.py df2aa91dba5b src/cpu/testers/rubytest/Check.hh df2aa91dba5b src/cpu/testers/rubytest/Check.cc df2aa91dba5b src/cpu/testers/rubytest/CheckTable.hh df2aa91dba5b src/cpu/testers/rubytest/CheckTable.cc df2aa91dba5b Diff: http://reviews.gem5.org/r/2749/diff/ Testing --- Thanks, Nilay Vaish ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
[gem5-dev] Review Request 2749: cpu: testers: rubytest: fix the test
--- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2749/ --- Review request for Default. Repository: gem5 Description --- Changeset 10811:b826d3a54c0c --- cpu: testers: rubytest: fix the test The test was broken. When run with more than a cpu, it resulted in error while configuring the system. Coherence protocols assume that multiple cpus are present when options.num_cpus 1. But the test only creates one tester even when we were testing a multicpu system. Now, we will create individual testers for each cpu. Diffs - src/cpu/testers/rubytest/RubyTester.hh df2aa91dba5b src/cpu/testers/rubytest/RubyTester.cc df2aa91dba5b src/cpu/testers/rubytest/RubyTester.py df2aa91dba5b tests/configs/rubytest-ruby.py df2aa91dba5b configs/example/ruby_random_test.py df2aa91dba5b src/cpu/testers/rubytest/Check.hh df2aa91dba5b src/cpu/testers/rubytest/Check.cc df2aa91dba5b src/cpu/testers/rubytest/CheckTable.hh df2aa91dba5b src/cpu/testers/rubytest/CheckTable.cc df2aa91dba5b Diff: http://reviews.gem5.org/r/2749/diff/ Testing --- Thanks, Nilay Vaish ___ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
