Re: [gem5-dev] [m5-dev] Review Request: Ruby: Correctly set access permissions for directory entries
On Fri, 6 May 2011, Beckmann, Brad wrote: Hi Nilay, Yeah, pulling the State into the Machine makes sense to me. If I recall, my previous patch made it necessary that each machine included a state_declaration (previously the state enum). More tightly integrating the state to the machine seems to be a natural progression on that path. I understand moving the permission settings back to setState is the easiest way to make this work. However, it would be great if we could combine the setting of state and the setting of permission into one function call from the sm file. Thus we don't have to worry about the situation where one sets the state, but forgets to set the permission. That could lead to some random functional access failing and a very painful debug. Brad I was trying to recall why I had suggested pulling State in to the Machine. It seems the reasoning was that then the calls to the function *_State_to_Permission() can be shortened to State_to_Permission(). The problem with combining the setting state and the permission it seems would be that cache and directory entries are treated differently. Cache entries get supplied to set state as pointers, where as directory entries are sought within the function it self. I am currently in favor of going with what I submitted earlier so that functional access patch can get out of the way soon as possible. -- Nilay ___ gem5-dev mailing list gem5-dev@m5sim.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [m5-dev] Review Request: Ruby: Correctly set access permissions for directory entries
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/684/#review1232 --- Ship it! Hi Nilay, I see no reason why this would not work with our protocols. Thanks for checking. - Lisa On 2011-05-06 15:52:00, Nilay Vaish wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/684/ > --- > > (Updated 2011-05-06 15:52:00) > > > Review request for Default. > > > Summary > --- > > Ruby: Correctly set access permissions for directory entries > The access permissions for the directory entries are not being set correctly. > This is because pointers are not used for handling directory entries. > function. The setState() function will once again set the permissions as well. > But it would make use of the State_to_permission() function, instead of doing > the analysis it used to do earlier. The changePermission() function provided > by the AbstractEntry and AbstractCacheEntry classes has been exposed to SLICC > code once again. The set_permission() functionality has been removed. > > I have done this only for the MESI protocol so far. Once we build a consensus > one the changes, I will make changes to other protocols as well. > > As far as testing is concerned, the protocol compiles and clears 1 loads. > I did not test any more than that. > > A point that I wanted to raise for discussion: I think we should pull State > enum and the accompanying functions into the Machine it self. Brad, what do > you think? > > > Diffs > - > > src/mem/protocol/MESI_CMP_directory-L1cache.sm 3c628a51f6e1 > src/mem/protocol/MESI_CMP_directory-L2cache.sm 3c628a51f6e1 > src/mem/protocol/MESI_CMP_directory-dir.sm 3c628a51f6e1 > src/mem/protocol/RubySlicc_Types.sm 3c628a51f6e1 > src/mem/slicc/ast/MethodCallExprAST.py 3c628a51f6e1 > src/mem/slicc/symbols/StateMachine.py 3c628a51f6e1 > > Diff: http://reviews.m5sim.org/r/684/diff > > > Testing > --- > > > Thanks, > > Nilay > > ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: Ruby: Correctly set access permissions for directory entries
The stats and regression tester should not need to be updated with this patch. This is purely a Ruby/SLICC mechanism change. Brad > -Original Message- > From: m5-dev-boun...@m5sim.org [mailto:m5-dev-boun...@m5sim.org] On > Behalf Of Nilay Vaish > Sent: Saturday, May 07, 2011 6:17 AM > To: M5 Developer List > Subject: Re: [m5-dev] Review Request: Ruby: Correctly set access > permissions for directory entries > > Korey, I don't think there will be any change in the simulation > performance. I am not sure about stats. > > Brad, were the stats updated after you made the change? > > -- > Nilay > > > > On Fri, 6 May 2011, Korey Sewell wrote: > > > Nilay, > > can you explain the impact of that bug in terms of simulation > performance? > > Are benchmarks running slower because of this change? Will > regressions need > > to be updated? > > > > On Fri, May 6, 2011 at 8:13 PM, Beckmann, Brad > wrote: > > > >> Hi Nilay, > >> > >> Yeah, pulling the State into the Machine makes sense to me. If I > recall, > >> my previous patch made it necessary that each machine included a > >> state_declaration (previously the state enum). More tightly > integrating the > >> state to the machine seems to be a natural progression on that path. > >> > >> I understand moving the permission settings back to setState is the > easiest > >> way to make this work. However, it would be great if we could > combine the > >> setting of state and the setting of permission into one function > call from > >> the sm file. Thus we don't have to worry about the situation where > one sets > >> the state, but forgets to set the permission. That could lead to > some > >> random functional access failing and a very painful debug. > >> > >> Brad > >> > >> > ___ > m5-dev mailing list > m5-dev@m5sim.org > http://m5sim.org/mailman/listinfo/m5-dev ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: Ruby: Correctly set access permissions for directory entries
Korey, I don't think there will be any change in the simulation performance. I am not sure about stats. Brad, were the stats updated after you made the change? -- Nilay On Fri, 6 May 2011, Korey Sewell wrote: Nilay, can you explain the impact of that bug in terms of simulation performance? Are benchmarks running slower because of this change? Will regressions need to be updated? On Fri, May 6, 2011 at 8:13 PM, Beckmann, Brad wrote: Hi Nilay, Yeah, pulling the State into the Machine makes sense to me. If I recall, my previous patch made it necessary that each machine included a state_declaration (previously the state enum). More tightly integrating the state to the machine seems to be a natural progression on that path. I understand moving the permission settings back to setState is the easiest way to make this work. However, it would be great if we could combine the setting of state and the setting of permission into one function call from the sm file. Thus we don't have to worry about the situation where one sets the state, but forgets to set the permission. That could lead to some random functional access failing and a very painful debug. Brad ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: Ruby: Correctly set access permissions for directory entries
Nilay, can you explain the impact of that bug in terms of simulation performance? Are benchmarks running slower because of this change? Will regressions need to be updated? On Fri, May 6, 2011 at 8:13 PM, Beckmann, Brad wrote: > Hi Nilay, > > Yeah, pulling the State into the Machine makes sense to me. If I recall, > my previous patch made it necessary that each machine included a > state_declaration (previously the state enum). More tightly integrating the > state to the machine seems to be a natural progression on that path. > > I understand moving the permission settings back to setState is the easiest > way to make this work. However, it would be great if we could combine the > setting of state and the setting of permission into one function call from > the sm file. Thus we don't have to worry about the situation where one sets > the state, but forgets to set the permission. That could lead to some > random functional access failing and a very painful debug. > > Brad > > > > -Original Message- > > From: m5-dev-boun...@m5sim.org [mailto:m5-dev-boun...@m5sim.org] > > On Behalf Of Nilay Vaish > > Sent: Friday, May 06, 2011 3:52 PM > > To: Nilay Vaish; Default > > Subject: [m5-dev] Review Request: Ruby: Correctly set access permissions > > for directory entries > > > > > > --- > > This is an automatically generated e-mail. To reply, visit: > > http://reviews.m5sim.org/r/684/ > > --- > > > > Review request for Default. > > > > > > Summary > > --- > > > > Ruby: Correctly set access permissions for directory entries The access > > permissions for the directory entries are not being set correctly. > > This is because pointers are not used for handling directory entries. > > function. The setState() function will once again set the permissions as > well. > > But it would make use of the State_to_permission() function, instead of > > doing the analysis it used to do earlier. The changePermission() function > > provided by the AbstractEntry and AbstractCacheEntry classes has been > > exposed to SLICC code once again. The set_permission() functionality has > > been removed. > > > > I have done this only for the MESI protocol so far. Once we build a > consensus > > one the changes, I will make changes to other protocols as well. > > > > As far as testing is concerned, the protocol compiles and clears 1 > loads. > > I did not test any more than that. > > > > A point that I wanted to raise for discussion: I think we should pull > State > > enum and the accompanying functions into the Machine it self. Brad, what > > do you think? > > > > > > Diffs > > - > > > > src/mem/protocol/MESI_CMP_directory-L1cache.sm 3c628a51f6e1 > > src/mem/protocol/MESI_CMP_directory-L2cache.sm 3c628a51f6e1 > > src/mem/protocol/MESI_CMP_directory-dir.sm 3c628a51f6e1 > > src/mem/protocol/RubySlicc_Types.sm 3c628a51f6e1 > > src/mem/slicc/ast/MethodCallExprAST.py 3c628a51f6e1 > > src/mem/slicc/symbols/StateMachine.py 3c628a51f6e1 > > > > Diff: http://reviews.m5sim.org/r/684/diff > > > > > > Testing > > --- > > > > > > Thanks, > > > > Nilay > > > > ___ > > m5-dev mailing list > > m5-dev@m5sim.org > > http://m5sim.org/mailman/listinfo/m5-dev > > > ___ > m5-dev mailing list > m5-dev@m5sim.org > http://m5sim.org/mailman/listinfo/m5-dev > -- - Korey ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: Ruby: Correctly set access permissions for directory entries
Hi Nilay, Yeah, pulling the State into the Machine makes sense to me. If I recall, my previous patch made it necessary that each machine included a state_declaration (previously the state enum). More tightly integrating the state to the machine seems to be a natural progression on that path. I understand moving the permission settings back to setState is the easiest way to make this work. However, it would be great if we could combine the setting of state and the setting of permission into one function call from the sm file. Thus we don't have to worry about the situation where one sets the state, but forgets to set the permission. That could lead to some random functional access failing and a very painful debug. Brad > -Original Message- > From: m5-dev-boun...@m5sim.org [mailto:m5-dev-boun...@m5sim.org] > On Behalf Of Nilay Vaish > Sent: Friday, May 06, 2011 3:52 PM > To: Nilay Vaish; Default > Subject: [m5-dev] Review Request: Ruby: Correctly set access permissions > for directory entries > > > --- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/684/ > --- > > Review request for Default. > > > Summary > --- > > Ruby: Correctly set access permissions for directory entries The access > permissions for the directory entries are not being set correctly. > This is because pointers are not used for handling directory entries. > function. The setState() function will once again set the permissions as well. > But it would make use of the State_to_permission() function, instead of > doing the analysis it used to do earlier. The changePermission() function > provided by the AbstractEntry and AbstractCacheEntry classes has been > exposed to SLICC code once again. The set_permission() functionality has > been removed. > > I have done this only for the MESI protocol so far. Once we build a consensus > one the changes, I will make changes to other protocols as well. > > As far as testing is concerned, the protocol compiles and clears 1 loads. > I did not test any more than that. > > A point that I wanted to raise for discussion: I think we should pull State > enum and the accompanying functions into the Machine it self. Brad, what > do you think? > > > Diffs > - > > src/mem/protocol/MESI_CMP_directory-L1cache.sm 3c628a51f6e1 > src/mem/protocol/MESI_CMP_directory-L2cache.sm 3c628a51f6e1 > src/mem/protocol/MESI_CMP_directory-dir.sm 3c628a51f6e1 > src/mem/protocol/RubySlicc_Types.sm 3c628a51f6e1 > src/mem/slicc/ast/MethodCallExprAST.py 3c628a51f6e1 > src/mem/slicc/symbols/StateMachine.py 3c628a51f6e1 > > Diff: http://reviews.m5sim.org/r/684/diff > > > Testing > --- > > > Thanks, > > Nilay > > ___ > m5-dev mailing list > m5-dev@m5sim.org > http://m5sim.org/mailman/listinfo/m5-dev ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
[m5-dev] Review Request: Ruby: Correctly set access permissions for directory entries
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/684/ --- Review request for Default. Summary --- Ruby: Correctly set access permissions for directory entries The access permissions for the directory entries are not being set correctly. This is because pointers are not used for handling directory entries. function. The setState() function will once again set the permissions as well. But it would make use of the State_to_permission() function, instead of doing the analysis it used to do earlier. The changePermission() function provided by the AbstractEntry and AbstractCacheEntry classes has been exposed to SLICC code once again. The set_permission() functionality has been removed. I have done this only for the MESI protocol so far. Once we build a consensus one the changes, I will make changes to other protocols as well. As far as testing is concerned, the protocol compiles and clears 1 loads. I did not test any more than that. A point that I wanted to raise for discussion: I think we should pull State enum and the accompanying functions into the Machine it self. Brad, what do you think? Diffs - src/mem/protocol/MESI_CMP_directory-L1cache.sm 3c628a51f6e1 src/mem/protocol/MESI_CMP_directory-L2cache.sm 3c628a51f6e1 src/mem/protocol/MESI_CMP_directory-dir.sm 3c628a51f6e1 src/mem/protocol/RubySlicc_Types.sm 3c628a51f6e1 src/mem/slicc/ast/MethodCallExprAST.py 3c628a51f6e1 src/mem/slicc/symbols/StateMachine.py 3c628a51f6e1 Diff: http://reviews.m5sim.org/r/684/diff Testing --- Thanks, Nilay ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
