Review Board 2.0.15


ruby: Add ReadRespWithInvalidate support, fixes MESI consistency bug

Review Request #2842 - Created May 21, 2015 and updated

Information
Marco Elver
gem5
default
Reviewers
Default 
Changeset 10839:2978f9ed5e7a


ruby: Add ReadRespWithInvalidate support, fixes MESI consistency bug


A sunk Inv in the IS state needs to be propagated to the LQ eventually. Thus far, Ruby has no support for properly propagating an invalidate along with a ReadResp.


This patch adds support for ReadRespWithInvalidate upon a readCallback, so that the LQ can properly deal with such a request.


Note that we cannot just do a forward_eviction in the IS,Inv transition, as other outstanding loads in the LQ after the one for which we get the Inv, may be satisfied (with old values) inbetween the Inv and IS_I,Data transitions.


The scenarios are the same as for the problem fixed in revision 10575 (which addressed the LSQ side of things).
Tester no longer finds bug. Note this was done with an (older) version of gem5 that was still happy with Ruby+O3CPU. A quick run with ruby-tester yielded no obvious issues.






NOTE: I'm not entirely happy with the "Hack?" code. If someone can possibly suggest how we can add a nicer way to add support for ReadRespWithInvalidate to Ruby, then that'd probably be better than what this patch does.

Issue Summary

2 1 0 1
Description From Last Updated Status
Yes, hack! :-) Surely this should be done as part of the hitCallback rather? Andreas Hansson May 31, 2015, 3:33 a.m. Open
Andreas Hansson
Posted (May 31, 2015, 3:33 a.m.)



  

   

   
    
    
   


  




   







    

     
     

      

       
        

         
          src/mem/ruby/system/Sequencer.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
Yes, hack! :-)


Surely this should be done as part of the hitCallback rather?


     





 

  
  
  
  
  
  Show all issues
 




     




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Seems like it should be in RubyPort::M5Port::hitCallback(PacketPtr pkt), which does other conditional packet transformations like this. I'm not sure of the best way to get the withInvalidate flag to that function, however.
    
    
    
   
    2. 

   


  


    


   



  

 


  

 

















 



 

  


 


  



   

   
Brad Beckmann

   
Posted  (June 1, 2015, 10:14 a.m.)


  


  


  

   
Question below:



  

   

   
    
    
   


  




   







    

     
     
    

    


     
Why introduce a new action here?  It seems like the simplier way to handle this situation is insert the forward_eviction_to_cpu action after the hx_load_hit callback rather than introduce a new action and additional complexity in the sequencer.


     





 

  
  
  
  
  
  Show all issues
 




     




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Calling hx_load_hit and then forward_eviction_to_cpu would always have to be seen by the LSQ in that order. To guarantee this is never an issue, both should be sent as one. The main reason being that if the ReadResp arrives after the InvalidateReq in the LSQ, the ReExec fault is lost in the LSQ. See comment in LSQ::recvTimingResp.
    

    From what I can see, this order is currently not guaranteed (and I'm not sure what the cost of guaranteeing it would be). In RubyPort, the InvalidateReq is not sent with a schedTimingResp, yet any response via hitCallback is. This means, that regardless of the calling (action) order, the InvalidateReq always arrives before the ReadResp, and the fault is always lost in the LSQ.
    

    There are likely other potential fixes (fix LSQ not to depend on order, guarantee order from Ruby side), but this one seems the least intrusive and most robust.
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Ah, I see.  Thanks for the explanation!
    

    Eventually we should remove all delay in the RubyPort interface.  The complexity of this patch is good justification why we should check in this other patch: http://reviews.gem5.org/r/2784 as a step of removing unecessary delay between the core and the l1 cache.
    

    This patch looks good to me then.
    
    
    
   
    3.