Review Board 2.0.15


MessagePort: implemented virtual recvTiming avoiding double delete

Review Request #382 - Created Jan. 6, 2011 and submitted

Information
Brad Beckmann
gem5
Reviewers
Default
ali, gblack, nate, stever 
MessagePort: implemented virtual recvTiming avoiding double delete

Double packet delete problem is due to an interrupt device deleting a packet
that the SimpleTimingPort also deletes. Since MessagePort descends from
SimpleTimingPort, simply reimplement the failing code from SimpleTimingPort:
recvTiming.

   

  


 


 

  

  









  
 


  

 
















 



 

  


 


  



   

   
Gabe Black

   
Posted  (Jan. 6, 2011, 8:21 p.m.)


  


  


  

   
I think there are two problems with this patch. First, if at all possible we should avoid the code duplication we'd now have for the recvTiming function. Second, while this probably does fix the legitimate problem of deleting packets twice, I think it creates a memory leak in the process. I suspect if you leave your other changes in place but get rid of your custom recvTiming function, things will still work. The packet won't be deleted by the device, won't be deleted after being received as a request in either atomic or timing mode, but will be deleted in both modes after being received as a response. The "virtual" you added in tport.hh could almost certainly go away then too.



  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Joel is the one who actually wrote this patch, so hopefully he can elaborate on the possible the memory leak.  I'll hold off on this patch until he can respond.
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Actually, the double delete problem still exists if we removed the (almost) replicated recvTiming code.  This is because pkt->needsResponse() returns false when the message type is MemCmd::MessageResp, which causes execution of the needsResponse else clause in SimpleTimingPort::recvTiming.  It would be freed there, as well as in recvAtomic.

I think when I tested this with Valgrind, I didn't see the memory leak (doesn't mean it doesn't exist).  However, I don't think I was able to justify to myself why it didn't occur.

I remember that I spent a while trying to figure out how to make this work nicely, but the inheritance SimpleTimingPort -> MessagePort -> IntPort, and the overloading that that implies makes this quite difficult to analyze.  For instance, I'm still not clear why the new MemCmd, MessageReq/Resp, needed to be defined for this.
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    It's been a while, but I think the idea was that MessageReq and MessageResp mean that no overlap between messages should happen. If you write 0x10 bytes to 0x0 and 0x10 bytes to 0x8, those writes will interact. If you write a 0x10 message to address 0x0 and a 0x10 message to 0x8, then since those are messages written to ports essentially, no interaction should happen. I don't know if it actually works that way in practice, but I kind of remember that being my thinking.

As far as the over/under deleting, I think the problem is that you're not supposed to add deletes to recvAtomic, aka leave that like it was and recvTiming like it was. I wasn't aware of this before, but digging through other bits of code it looks like recvAtomic doesn't delete packets that are being received. The idea is likely that since the call is being done in place, the calling code can easily clean up after itself. For recvTiming the source may no longer even exist when a packet is being handled.

So continue to get rid of the deletes in recvResponse, leave the recvAtomic and recvTiming alone, and everything should be straightened out I think. You'll also need to delete the packet built in the function that originally called sendAtomic, which appears to be sendMessage in IntPort.
    
    
    
   
    4. 

   


  




   







    

     
     

      

       
        

         
          src/mem/mport.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
I really don't like how much code duplication there is now between these two classes.




  

   

   
    
    
   


  


    


    

     
     

      

       
        

         
          src/mem/tport.hh
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
Marking this as explicitly virtual shouldn't really be necessary. Is there a reason you want to?




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    I think I had trouble compiling since MessagePort overloads recvTiming.  In this patch, MessagePort would become the first (only) descendant class of SimpleTimingPort that overloads recvTiming.
    
    
    
   
    2.