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

Patina PR #410

Open
wants to merge 13 commits into
base: loaderarm
Choose a base branch
from
Open

Patina PR #410

wants to merge 13 commits into from

Conversation

msdx321
Copy link
Member

@msdx321 msdx321 commented Feb 8, 2021

Summary of this Pull Request (PR)

Implement Patina library and benchkit

Intent for your PR

Choose one (Mandatory):

  • This PR is for a code-review and is intended to get feedback, but not to be pulled yet.
  • This PR is mature, and ready to be integrated into the repo.

Reviewers (Mandatory):

(Specify @<github.com username(s)> of the reviewers. Ex: @user1, @user2)

Code Quality

As part of this pull request, I've considered the following:

Style:

  • Comments adhere to the Style Guide (SG)
  • Spacing adhere's to the SG
  • Naming adhere's to the SG
  • All other aspects of the SG are adhered to, or exceptions are justified in this pull request
  • I have run the auto formatter on my code before submitting this PR (see doc/auto_formatter.md for instructions)

Code Craftsmanship:

  • I've made an attempt to remove all redundant code
  • I've considered ways in which my changes might impact existing code, and cleaned it up
  • I've formatted the code in an effort to make it easier to read (proper error handling, function use, etc...)
  • I've commented appropriately where code is tricky
  • I agree that there is no "throw-away" code, and that code in this PR is of high quality

Testing

I've tested the code using the following test programs (provide list here):

  • micro_booter
  • unit_pingpong
  • unit_schedtests
  • ...(add others here)

Signed-off-by: msdx321 <msdx321@gmail.com>
Signed-off-by: msdx321 <msdx321@gmail.com>
Signed-off-by: msdx321 <msdx321@gmail.com>
Signed-off-by: msdx321 <msdx321@gmail.com>
Signed-off-by: msdx321 <msdx321@gmail.com>
Signed-off-by: msdx321 <msdx321@gmail.com>
Signed-off-by: msdx321 <msdx321@gmail.com>
Signed-off-by: msdx321 <msdx321@gmail.com>
Signed-off-by: msdx321 <msdx321@gmail.com>
Signed-off-by: msdx321 <msdx321@gmail.com>
Signed-off-by: msdx321 <msdx321@gmail.com>
Signed-off-by: msdx321 <msdx321@gmail.com>
Signed-off-by: msdx321 <msdx321@gmail.com>
Copy link
Collaborator

@gparmer gparmer 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 really great, and very close to being done, as I read it.

The biggest change I'd like to see is 1. remove the rampant debug printouts from the previous PR (sorry to ask this of you!), and 2. make sure that the printouts well-explain what is being tested. When the tests are done, they should print out that they are done in a consistent manner. Imagine a python script parsing their output to make sure that they still spit out the correct output.

while (1) {
cycles_t end;

while (count < ITERATION) {
debug("h1,");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know you didn't add this code, but there is no reason to keep meaningless printouts in the code. Feel free to remove them.

@@ -0,0 +1,9 @@
## tests

This is the skeleton interface used by the `mkcomponent.sh` script to aid in the creation of a new component.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a brief note here about what it actually does, or remove this file. The file has no value as it stands. (I know it was automatically created).

}

/***
* The two threads reciprocally sends and receives.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"send" and "receive" should be singular.

{
/* Never stops running; writer controls how many iters to run. */
while (1) {
debug("r1,");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even worse, the debug statements make this hard to read! I know you didn't write them originally.

{
int i;

for (int i = 0; i < ITERATION + COLD_OFFSET; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

double definition of i. Remove the int on this line.


assert(!crt_sem_init(s, (unsigned long)init_value))

ss_sem_activate(s);
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like the spacing is uniformly off for this style of function.

assert(a & b);
assert(!(a->sec + b->sec < a->sec || a->usec + b->usec < a->usec))

a->sec = a->sec + b->sec;
Copy link
Collaborator

Choose a reason for hiding this comment

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

spacing

patina_time_add(struct time *a, struct time *b)
{
assert(a & b);
assert(!(a->sec + b->sec < a->sec || a->usec + b->usec < a->usec))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great job checking for the overflow.

assert(a & b);
assert(!(a->sec - b->sec > a->sec || a->usec - b->usec > a->usec))

a->sec = a->sec - b->sec;
Copy link
Collaborator

Choose a reason for hiding this comment

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

space etc...

@@ -229,7 +229,7 @@ perfdata_99ptile(struct perfdata *pd)
static void
perfdata_print(struct perfdata *pd)
{
printc("PD:%s -sz:%d,SD:%llu,Mean:%llu,99%%:%llu, Max: %llu\n",
printc("#PD:%s -sz:%d,SD:%llu,Mean:%llu,99%%:%llu, Max: %llu\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

smart.

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.

None yet

2 participants