Skip to content

Multi sched read #55

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

Merged
merged 10 commits into from
Apr 19, 2025
Merged

Multi sched read #55

merged 10 commits into from
Apr 19, 2025

Conversation

marioskogias
Copy link
Collaborator

No description provided.

@marioskogias marioskogias force-pushed the multi-sched-read branch 4 times, most recently from c74300f to f8ae235 Compare November 11, 2024 13:49
@marioskogias marioskogias marked this pull request as ready for review November 11, 2024 13:55
@marioskogias marioskogias requested a review from mjp41 November 11, 2024 13:56
Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

This is looks good. Here are some minor comments as I tried to understand the code.

@@ -39,6 +39,99 @@ void test_body()
});
}

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should have some more racy tests. Where there are concurrent attempts to schedule, and hence concurrent potential wake and schedule, and sleep. We don't seem to cover those cases here.

{
Logging::cout() << " Previous slot is a writer or blocked reader cown "
<< *new_slot << Logging::endl;
<< *chain_first_slot << Logging::endl;
yield();
goto fn_out;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if direct returns in this code would be clearer.

Suggested change
goto fn_out;
return {0, 0};

I'm also not sure I get the usage of ex_count here. This is that all the reads in this chain have seen a READ_AVAILABLE, so they can all decrease there execution count by 1?

yield();
cown->next_writer = first_body;
}
}

// Process execution count
ec[first_body_index] += ex_count;

for (int i = 1; i < first_consecutive_readers_count; i++)
ec[first_body_index + i] += 1;
Copy link
Member

@mjp41 mjp41 Nov 20, 2024

Choose a reason for hiding this comment

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

Why is ex_count not used here? It feels like the whole chain should get to go if the read available was seen during scheduling, but otherwise it shouldn't? Doesn't this increase the count on the chain apart from the first entry, if read available wasn't seen?

Comment on lines 1001 to 1016
{
Logging::cout() << " Writer at head of queue and got the cown "
<< *curr_slot << Logging::endl;
<< *chain_first_slot << Logging::endl;
ex_count++;
yield();
}
Copy link
Member

Choose a reason for hiding this comment

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

Can ex_count be anything other than 0 in this branch?

Suggested change
{
Logging::cout() << " Writer at head of queue and got the cown "
<< *curr_slot << Logging::endl;
<< *chain_first_slot << Logging::endl;
ex_count++;
yield();
}
{
Logging::cout() << " Writer at head of queue and got the cown "
<< *chain_first_slot << Logging::endl;
ec[first_body_index] += 1;
yield();
continue;
}

Comment on lines 1015 to 1035

// Process execution count
ec[first_body_index] += ex_count;

for (int i = 1; i < first_consecutive_readers_count; i++)
ec[first_body_index + i] += 1;
}

Copy link
Member

Choose a reason for hiding this comment

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

If we can treat ex_count as a read chain that observed the read was available. Could we do this? Perhaps with a rename of ex_count?

Suggested change
// Process execution count
ec[first_body_index] += ex_count;
for (int i = 1; i < first_consecutive_readers_count; i++)
ec[first_body_index + i] += 1;
}
// Process execution count
if (ex_count != 0)
{
for (int i = 0; i < first_consecutive_readers_count; i++)
ec[first_body_index + i] += 1;
}
}

@mjp41
Copy link
Member

mjp41 commented Dec 2, 2024

@vishalgupta97 could you take a look at this? Thanks

@vishalgupta97
Copy link
Contributor

The changes looks good. Maybe a test case can be added to test longer changes with different combinations of reads and writes.

@mjp41 mjp41 enabled auto-merge (squash) April 19, 2025 14:49
@mjp41 mjp41 force-pushed the multi-sched-read branch from 3e230b6 to 4e6b40c Compare April 19, 2025 15:19
@mjp41 mjp41 merged commit 89f3d77 into microsoft:main Apr 19, 2025
3 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants