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

Shared memory bitmap allocator #443

Merged
merged 62 commits into from
Feb 25, 2022
Merged

Shared memory bitmap allocator #443

merged 62 commits into from
Feb 25, 2022

Conversation

evanstella
Copy link
Contributor

@evanstella evanstella commented Feb 14, 2022

Summary of this Pull Request (PR)

Client/Server Shared memory library shm_bm. Tested on x86_64.

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
  • ping_pong_shmem, sched_ping_pong, sched_crt_tests,

@evanstella evanstella changed the title Lib/shmem Shared memory bitmap allocator Feb 14, 2022
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.

OMG this is fantastic. A few notes, and a few requested changes, but I think it is good to go for now. I can pull now, or wait till you do a few tweaks (comments throughout). Up to you.

src/components/implementation/tests/unit_pingshmem/ping.c Outdated Show resolved Hide resolved
src/components/implementation/tests/unit_pingshmem/ping.c Outdated Show resolved Hide resolved
src/components/lib/shm_bm/shm_bm.h Outdated Show resolved Hide resolved
src/components/lib/shm_bm/shm_bm.h Outdated Show resolved Hide resolved

shmptr = entry->shmptr;
// obj has not been allocated
if (unlikely(*(SHM_BM_REFC(shmptr, nobj) + objid) == 0)) return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that this check actually has much utility. It is a subtle argument so we should chat about it at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this would be required since it is possible for a component to take a reference to an unallocated object otherwise. This might not be something we are worried about tho.

src/components/lib/shm_bm/shm_bm.h Outdated Show resolved Hide resolved
src/components/lib/shm_bm/shm_bm.h Outdated Show resolved Hide resolved
@@ -49,6 +49,18 @@ cos_faa(int *var, int value)
return value;
}

/* Byte sized fetch-and-add implementation on x86. It returns the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a note and/or empty function for this in arm that blows up?

@gparmer
Copy link
Collaborator

gparmer commented Feb 25, 2022

Great job! looks really clean.

@gparmer gparmer merged commit 5a0b323 into gwsystems:main Feb 25, 2022
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.

2 participants