Skip to content

Fix moveRegularItemOnEviction function #36

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 12 commits into from
Closed

Fix moveRegularItemOnEviction function #36

wants to merge 12 commits into from

Conversation

vinser52
Copy link
Collaborator

@vinser52 vinser52 commented Feb 3, 2022 •
edited by igchor
Loading

@igchor Please review

This change is Reviewable

@vinser52 vinser52 requested a review from igchor February 3, 2022 10:58
@vinser52
Copy link
Collaborator Author

vinser52 commented Feb 3, 2022

In this PR I have fixed the moveRegularItemOnEviction function. The fix contains the following:

  1. Fixed the comment of the function which describes the return value of moveRegularItemOnEviction function. The function tries to move the content of the oldItem to the newItemHdl in order to recycle the oldItem. If the function succeeds it returns the handle to the oldItem, so that it can be reused for the new stuff. Otherwise, it returns an empty handle.
  2. If replaceInMMContainer(oldItemPtr, *newItemHdl) or newItemHdl->isAccessible() return false it means some concurrent thread removed this key or called inserOrReplace, but we still can recycle the oldItem.
  3. Removed code block related to chainedItem from moveRegularItemOnEviction because this function intended to work only with regular Items and we have no logic for chainedItem yet.

igchor
igchor reviewed Feb 3, 2022
View reviewed changes
Copy link

@igchor igchor left a comment

Choose a reason for hiding this comment

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

LGTM but it would be good to find out why similar logic is not applied in moveRegularItem.

Reviewable status: 0 of 2 files reviewed, all discussions resolved

@victoria-mcgrath victoria-mcgrath mentioned this pull request May 5, 2022
Closed
@vinser52
Merge pull request pmem#15 from vinser52/fix_deprecation_warn
f0c5c54 
Fix deprecation warn
@vinser52
Update CI to use intel/CacheLib repo (pmem#17)
f1dfc60
@vinser52
Clean-up test's blacklist (pmem#18)
63c9029
@vinser52
Merge pull request pmem#13 from vinser52/numa_binding
a4c4ab3 
Initial support of NUMA bindings
@vinser52
Fix ReaperSkippingSlabTraversalWhileSlabReleasing test
187bbf4 
The issue was caused by incorrect behaviour of the
CacheAllocator<CacheTrait>::tryEvictToNextMemoryTier method in case the
evicted item is expired. We cannot simply return a handle to it, but we need
to remove it from the access container and MM container.
@vinser52
Merge pull request pmem#19 from vinser52/fix_allocator_test
829a434 
Fix ReaperSkippingSlabTraversalWhileSlabReleasing test
@vinser52 vinser52 force-pushed the fix_async_movement branch 2 times, most recently from 777ffbd to 9c10d59 Compare October 10, 2022 14:36
@igchor igchor closed this Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igchor igchor igchor left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vinser52 @igchor