Are all the concerns and questions that Andreas S raised addressed? Andreas
On 19/06/2013 11:32, "Nilay Vaish" <[email protected]> wrote: >Does anyone feel that more discussion is required on this patch? I am >planning to commit it on coming Monday. > >-- >Nilay > >On Thu, 13 Jun 2013, Nilay Vaish wrote: > >> At the very outset, let me state that there are things that I do not >>have a >> complete understanding of. So what ever I am stating my be how I feel >>things >> work after observing the code execution. >> >> 1. I think a processor in Idle state means that there is no PC >>associated >> with it. If you want to, I think you can assume the PC to be 0 in Idle >>state. >> >> 2. From what I observed, it seems that the processor that boots up >>first >> controls the booting of other processors. In the two processor system >>that I >> tested with, I observed that processor 1 executed 4-5 instructions from >>the >> microcode rom. Then it stopped with the micropc != 0 and did not >>execute >> anything for a considerable amount of time. Then, it started executing >> instructions at some point in time. The in between time when it was not >> executing instructions, the cpu type was switched at least 2-3 times. I >>had >> to make those changes to the code so to prevent the system from getting >>into >> a deadlock as micropc for processor 1 was != 0 and it was not executing >> anything. >> >> If we force that the processor 1 cannot stop with micropc != 0 and if >>it is >> true that processor 1 is dependent on processor 0, I think we can get >>into a >> deadlock while trying to drain the system. This is because processor 1 >>will >> not execute anything since it is waiting for processor 0 to do some >>thing, >> while processor 0 will drain it self and stop executing instructions. >> >> -- >> Nilay >> >> >> On Tue, 11 Jun 2013, Andreas Sandberg wrote: >> >>> >>> ----------------------------------------------------------- >>> This is an automatically generated e-mail. To reply, visit: >>> http://reviews.gem5.org/r/1904/#review4415 >>> ----------------------------------------------------------- >>> >>> >>> I get the feeling that this will completely break all the assumptions >>>we >>> make about a drained system not executing microcode. You need to >>>change >>> that before committing, otherwise all KVM-related work will break. >>> >>> >>> src/cpu/o3/commit_impl.hh >>> <http://reviews.gem5.org/r/1904/#comment4140> >>> >>> This breaks all the assumptions we make about a drained system. It >>> doesn't matter if it is Idle or not, as long as it is executing >>>microcode, >>> it isn't drained and will break things. >>> >>> >>> >>> src/cpu/simple/atomic.cc >>> <http://reviews.gem5.org/r/1904/#comment4137> >>> >>> I'm not sure if this is actually correct. Isn't it possible for a >>>CPU to >>> be Idle and in microcode or something else that will break the >>>assumptions >>> we have about drained CPUs? >>> >>> >>> >>> src/cpu/simple/atomic.cc >>> <http://reviews.gem5.org/r/1904/#comment4138> >>> >>> isDrained() should always be true, independent of the state, when >>>we >>> reach this code. Could you move it to a separate assert to make sure >>>we >>> better error messages if it explodes? >>> >>> >>> >>> src/cpu/simple/timing.cc >>> <http://reviews.gem5.org/r/1904/#comment4139> >>> >>> Same as for the atomic CPU. Should probably be something like this: >>> >>> assert(_statue == Running || _status == Idle); >>> assert(isDrained()); >>> >>> >>> >>> - Andreas Sandberg >>> >>> >>> On June 7, 2013, 3:48 p.m., Nilay Vaish wrote: >>>> >>>> ----------------------------------------------------------- >>>> This is an automatically generated e-mail. To reply, visit: >>>> http://reviews.gem5.org/r/1904/ >>>> ----------------------------------------------------------- >>>> >>>> (Updated June 7, 2013, 3:48 p.m.) >>>> >>>> >>>> Review request for Default. >>>> >>>> >>>> Description >>>> ------- >>>> >>>> Changeset 9748:5d7be2fc04c7 >>>> --------------------------- >>>> cpu: some changes to switching >>>> >>>> 1. The fetch stage, when it overtakes from another cpu, initializes >>>>its >>>> status variable to Running. When an o3 cpu is about to switch to >>>>another >>>> cpu, the fetch stage checks that its status should be Idle. Now >>>>suppose >>>> there are two processors in the system. The operating system has just >>>> started and it is running on cpu0. Then, cpu1 would not be actually >>>>doing >>>> anything. When trying to switch to another cpu, cpu1 gets stuck >>>>because >>>> there is nothing going on that will move it from Running to Idle. >>>> >>>> I think we should have the fetchStatus be initialized to Idle state. >>>> The o3 cpu will activate some context at point in future. When it does >>>> that, it calls the function wakeFromQuiesce() on the fetch stage. This >>>> function in turn changes the fetchStatus to Running. As of now, the >>>> function only does this for thread 0. I am proposing that we >>>> pass the thread id and do it for that particular thread. >>>> >>>> 2. The TimingSimpleCPU incorrectly tested its status when switching >>>>out. >>>> >>>> 3. The commit stage should check for the its status being Idle when >>>> testing whether it needs to drain itself. >>>> >>>> 4. The atomic cpu should test its status variable before checking any >>>> other variables for deciding on draining. >>>> >>>> >>>> Diffs >>>> ----- >>>> >>>> src/cpu/o3/commit_impl.hh ea26ba576891 >>>> src/cpu/o3/cpu.cc ea26ba576891 >>>> src/cpu/o3/fetch.hh ea26ba576891 >>>> src/cpu/o3/fetch_impl.hh ea26ba576891 >>>> src/cpu/simple/atomic.cc ea26ba576891 >>>> src/cpu/simple/timing.cc ea26ba576891 >>>> >>>> Diff: http://reviews.gem5.org/r/1904/diff/ >>>> >>>> >>>> Testing >>>> ------- >>>> >>>> >>>> Thanks, >>>> >>>> Nilay Vaish >>>> >>>> >>> >>> >> >_______________________________________________ >gem5-dev mailing list >[email protected] >http://m5sim.org/mailman/listinfo/gem5-dev > -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
