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
