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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

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 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?

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
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

@@ -2427,19 +2426,16 @@ machine(MachineType:CorePair, "CP-like Core Coherence")
}

transition(MO_I, NB_AckWB, I) {L2TagArrayWrite} {
wb_data;
Copy link
Member

Choose a reason for hiding this comment

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

Remove the wb_data action since these are the only 4 cases it is used

p_popRequestQueue;
}

transition(BL, {VicDirty, VicClean}) {
zz_recycleRequestQueue;
}

//CPUData is now deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Should remove the CPUData event, in_port triggers, and all CPUData transitions if that is the case

@@ -1397,22 +1400,33 @@ machine(MachineType:Directory, "AMD Baseline protocol")
p_popRequestQueue;
}

transition(U, VicDirty, BL) {L3TagArrayRead} {
transition(U, VicDirty, U) {L3TagArrayRead, L3DataArrayWrite} {
Copy link
Member

Choose a reason for hiding this comment

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

The 3rd transition , U is not necessary here (it will remain in the same state)

p_popRequestQueue;
}

transition(U, VicClean, BL) {L3TagArrayRead} {
transition(U, VicClean, U) {L3TagArrayRead, L3DataArrayWrite} {
Copy link
Member

Choose a reason for hiding this comment

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

Also do not need 3rd parameter here

@mattsinc
Copy link
Contributor

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

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

3 participants