Hi Ciro, Thank you for your reply. Looks like this patch does address the problem I mentioned along with some other ones. Thanks for sharing.
Best Regards, Shehab On Thu, Apr 9, 2020 at 1:45 PM Ciro Santilli <ciro.santi...@gmail.com> wrote: > Thanks for this Shehab, > > Could you compare your changes to this patchset: > https://gem5-review.googlesource.com/c/public/gem5/+/22022/1 > > On Thu, Apr 9, 2020 at 6:22 PM Shehab Elsayed <shehaby...@gmail.com> > wrote: > > > > Hello All, > > > > I was running some experiments and I ran into a problem with ruby where > a functional read was failing. After some investigation I found that the > reason was that the functional read was trying to read a line that was in a > MaybeStale state (no ReadOnly or ReadWrite versions). > > > > I implemented a fix which so far seems to be working fine but I am not > sure if there is a deeper problem that needs fixing or if my fix could > present future problems. > > > > I am running Full System simulations with ARM ISA and MESI_Three_Level. > > > > Here is my fix (I have marked new lines with //------NEW------//): > > Basically what this fix does is perform the functional read from the > controller that has the line in the MaybeStale state if no ReadOnly or > ReadWrite versions in any controller. > > > > bool > > RubySystem::functionalRead(PacketPtr pkt) > > { > > Addr address(pkt->getAddr()); > > Addr line_address = makeLineAddress(address); > > > > AccessPermission access_perm = AccessPermission_NotPresent; > > int num_controllers = m_abs_cntrl_vec.size(); > > > > DPRINTF(RubySystem, "Functional Read request for %#x\n", address); > > > > unsigned int num_ro = 0; > > unsigned int num_rw = 0; > > unsigned int num_busy = 0; > > unsigned int num_backing_store = 0; > > unsigned int num_invalid = 0; > > unsigned int num_maybe_stale = 0; //------NEW------// > > > > // In this loop we count the number of controllers that have the > given > > // address in read only, read write and busy states. > > for (unsigned int i = 0; i < num_controllers; ++i) { > > > > // Ignore ATD controllers for functional reads > > if (m_abs_cntrl_vec[i]->getType() == MachineType_ATD) { > > continue; > > } > > > > access_perm = m_abs_cntrl_vec[i]-> > getAccessPermission(line_address); > > if (access_perm == AccessPermission_Read_Only) > > num_ro++; > > else if (access_perm == AccessPermission_Read_Write) > > num_rw++; > > else if (access_perm == AccessPermission_Busy) > > num_busy++; > > else if (access_perm == AccessPermission_Backing_Store) > > // See RubySlicc_Exports.sm for details, but Backing_Store > is meant > > // to represent blocks in memory *for Broadcast/Snooping > protocols*, > > // where memory has no idea whether it has an exclusive copy > of data > > // or not. > > num_backing_store++; > > else if (access_perm == AccessPermission_Invalid || > > access_perm == AccessPermission_NotPresent) > > num_invalid++; > > else if (access_perm == AccessPermission_Maybe_Stale) > //------NEW------// > > num_maybe_stale++; > //------NEW------// > > } > > > > // This if case is meant to capture what happens in a Broadcast/Snoop > > // protocol where the block does not exist in the cache hierarchy. > You > > // only want to read from the Backing_Store memory if there is no > copy in > > // the cache hierarchy, otherwise you want to try to read the RO or > RW > > // copies existing in the cache hierarchy (covered by the else > statement). > > // The reason is because the Backing_Store memory could easily be > stale, if > > // there are copies floating around the cache hierarchy, so you want > to read > > // it only if it's not in the cache hierarchy at all. > > if (num_invalid == (num_controllers - 1) && num_backing_store == 1) { > > DPRINTF(RubySystem, "only copy in Backing_Store memory, read > from it\n"); > > for (unsigned int i = 0; i < num_controllers; ++i) { > > access_perm = > m_abs_cntrl_vec[i]->getAccessPermission(line_address); > > if (access_perm == AccessPermission_Backing_Store) { > > m_abs_cntrl_vec[i]->functionalRead(line_address, pkt); > > return true; > > } > > } > > } else if (num_ro > 0 || num_rw >= 1 || num_maybe_stale > 0) { > //------NEW------// > > if (num_rw > 1) { > > // We iterate over the vector of abstract controllers, and > return > > // the first copy found. If we have more than one cache with > block > > // in writable permission, the first one found would be > returned. > > warn("More than one Abstract Controller with RW permission > for " > > "addr: %#x on cacheline: %#x.", address, line_address); > > } > > // In Broadcast/Snoop protocols, this covers if you know the > block > > // exists somewhere in the caching hierarchy, then you want to > read any > > // valid RO or RW block. In directory protocols, same thing, > you want > > // to read any valid readable copy of the block. > > DPRINTF(RubySystem, "num_busy = %d, num_ro = %d, num_rw = %d\n", > > num_busy, num_ro, num_rw); > > > > // In this loop, we try to figure which controller has a read > only or > > // a read write copy of the given address. Any valid copy would > suffice > > // for a functional read. > > // Sometimes the functional read is to a line that has recently > > // transitioned to MaybeStale state and no other controller has > it in > > // a ReadOnly or ReadWrite stat yet. > > // For example in MESI_Three_Level a modified block in the L2 > goes into > > // a MaybeStale state when requested by the L1. If the > functional read > > // occurs at that specific time when the block in L2 became > MaybeStale > > // but still hasn't made it to ReadOnly on ReadWrite state in > the L1, > > // the request should then be fulfilled from the L2 controller. > > // (I think so) > > > > // In case no ReadOnly or ReadWrite controller was found, this > will > > // point to the MaybeStale controller > > AbstractController* func_read_cntrl = NULL; > //------NEW------// > > > > for (unsigned int i = 0;i < num_controllers;++i) { > > > > access_perm = m_abs_cntrl_vec[i]-> > > getAccessPermission(line_address); > > if (access_perm == AccessPermission_Read_Only || > > access_perm == AccessPermission_Read_Write) { > > m_abs_cntrl_vec[i]->functionalRead(line_address, pkt); > > return true; > > } > > > > // Record the first MaybeStale controller in case no > ReadOnly or > > // ReadWrite controller was found > > if (access_perm == AccessPermission_Maybe_Stale && > //------NEW------// > > func_read_cntrl == NULL) { > //------NEW------// > > func_read_cntrl = m_abs_cntrl_vec[i]; > //------NEW------// > > } > //------NEW------// > > } > > > > // At this point no RO or RW controller was found, read from the > > // MaybeStale one > > if (func_read_cntrl != NULL) { > //------NEW------// > > func_read_cntrl->functionalRead(line_address, pkt); > //------NEW------// > > return true; > //------NEW------// > > } > //------NEW------// > > } > > > > return false; > > } > > _______________________________________________ > > gem5-users mailing list > > gem5-users@gem5.org > > http://m5sim.org/cgi-bin/mailman/listinfo/gem5-users >
_______________________________________________ gem5-users mailing list gem5-users@gem5.org http://m5sim.org/cgi-bin/mailman/listinfo/gem5-users
