Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mem-ruby: Reduce handshaking between CorePair and dir #1117

Merged
merged 1 commit into from
May 30, 2024

Conversation

NSurawar
Copy link

@NSurawar NSurawar commented May 8, 2024

Currently when data is downgraded by MOESI_AMD_Base-CorePair (e.g. due to a replacement) this requires a 4-way handshake between the CorePair and the dir. Specifically, the CorePair send a message telling the dir it'd like to downgrade then, the dir sends an ACK back and then, the CorePair writes the data back, and finally, the dir ACKs the writeback.
This is very inefficient and not representative of how modern protocols downgrade a request. Accordingly, this commits updates the downgrade support such that the CorePair writes back the data immediately and then the dir ACKs it.
Thus, this approach requires only a 2-way handshake.

Change-Id: I7ebc85bb03e8ce46a8847e3240fc170120e9fcd6

@mattsinc mattsinc requested review from abmerop and mattsinc May 8, 2024 18:02
@mattsinc mattsinc added the mem-ruby Ruby caches, structures, and protocols label May 8, 2024
Copy link
Contributor

@mattsinc mattsinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the changes we discussed earlier, one more.

Otherwise this looks good -- nice job!

src/mem/ruby/protocol/MOESI_AMD_Base-dir.sm Outdated Show resolved Hide resolved
@abmerop
Copy link
Member

abmerop commented May 9, 2024

FYI, I may not get to this until next week

@mattsinc
Copy link
Contributor

mattsinc commented May 9, 2024

FYI, I may not get to this until next week

@NSurawar has changes to make from my suggestions already, so no worries.

@mattsinc
Copy link
Contributor

@NSurawar can you please wrap the changes up this week?

@NSurawar NSurawar force-pushed the Coherence_fix branch 2 times, most recently from d8c01e0 to 32c32b0 Compare May 16, 2024 21:12
mattsinc
mattsinc previously approved these changes May 16, 2024
Copy link
Contributor

@mattsinc mattsinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok from my end, @abmerop let us know

@mattsinc
Copy link
Contributor

@NSurawar : you'll also need to update this branch to the head of develop before this can be committed.

Copy link
Member

@abmerop abmerop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable but I'd recommending going through and removing all of the dead code introduced by this change (CPUData can go, and wd_data action).

Comment on lines 365 to 367
//Deprecated - WriteBack data (wb_data) is now sent with VicDirty itself
//Leaving this in for reference
assert(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just remove this. The error below should tell you which message type is missing

src/mem/ruby/protocol/MOESI_AMD_Base-dir.sm Outdated Show resolved Hide resolved
src/mem/ruby/protocol/MOESI_AMD_Base-dir.sm Outdated Show resolved Hide resolved
src/mem/ruby/protocol/MOESI_AMD_Base-dir.sm Outdated Show resolved Hide resolved
@mattsinc
Copy link
Contributor

@NSurawar are you able to make @abmerop 's requested changes this weekend?

@mattsinc
Copy link
Contributor

@NSurawar are you able to make @abmerop 's requested changes this weekend?

@NSurawar please let me know -- we are trying to release a new version of gem5 and I will make these changes if needed so this doesn't linger.

@NSurawar
Copy link
Author

I'm sorry for the delay. I was caught up in some other work.
I'm looking into it now.

Currently when data is downgraded by MOESI_AMD_Base-CorePair (e.g. due to a replacement)
this requires a 4-way handshake between the CorePair and the dir.
Specifically, the CorePair send a message telling the dir it'd like to downgrade then,
the dir sends an ACK back and then, the CorePair writes the data back, and finally,
the dir ACKs the writeback.
This is very inefficient and not representative of how modern protocols downgrade a request.
Accordingly, this commits updates the downgrade support such that the CorePair writes back
the data immediately and then the dir ACKs it.
Thus, this approach requires only a 2-way handshake.

Change-Id: I7ebc85bb03e8ce46a8847e3240fc170120e9fcd6
@NSurawar
Copy link
Author

I've removed the stale code now. Please let me know if any additional changes are required.

Copy link
Member

@BobbyRBruce BobbyRBruce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me but i'll wait for @mattsinc or @abmerop to confirm and squash-merge it.

@ivanaamit ivanaamit merged commit efbfdea into gem5:develop May 30, 2024
35 checks passed
@mattsinc
Copy link
Contributor

Thanks all for getting this across the line while I was away

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mem-ruby Ruby caches, structures, and protocols
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants