Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

fix(arc): Per-CID locking. Sync Map to CID lock #68

Closed
wants to merge 1 commit into from

Conversation

frrist
Copy link
Member

@frrist frrist commented Apr 6, 2021

No description provided.

@gammazero
Copy link

gammazero commented Apr 7, 2021

Having per-cid locking may be slightly more complex, and there could be some additional overhead with allocating locks and growing a map. For your consideration, here is a very simple alternative that hashes keys over a fixed set of locks:
https://github.com/kubernetes/utils/tree/master/keymutex

If you do prefer per-cid locking, allow me to suggest that you do not need a separate mutex for each cid. The presence of a map entry (a struct{}{}) is sufficient to determine if a resource is locked. If the cid is already locked (map entry present), Then you can use sync.Cond.Wait() to wait for a signal to check again to see if the resource is unlocked (map entry gone). Lock the resource by creating the map entry, unlock by removing it and signaling the sync.Cond.
Example here: https://github.com/gammazero/kmutex/blob/master/kmutex.go
That example uses interface{} as its key type, but there was no performance difference when benchmarked against ints and strings.

The things here are not meant to prescribe a specific implementation, but to be helpful examples if considering different implementations.

@frrist frrist force-pushed the frrist/arc-cache-bench branch 2 times, most recently from 11498f4 to 80e1a84 Compare April 7, 2021 18:22
@frrist frrist requested a review from aschmahmann April 7, 2021 19:19
@frrist frrist force-pushed the frrist/arc-cache-syncmap branch 2 times, most recently from a019fdb to 7926442 Compare April 8, 2021 18:44
@frrist frrist self-assigned this Apr 8, 2021
@frrist frrist requested a review from Stebalien April 8, 2021 19:13
@frrist frrist mentioned this pull request Apr 26, 2021
2 tasks
@frrist frrist force-pushed the frrist/arc-cache-syncmap branch from 7926442 to 7e416b8 Compare May 3, 2021 20:00
@frrist frrist changed the base branch from frrist/arc-cache-bench to master May 3, 2021 20:00
@Stebalien
Copy link
Member

Closing based on #64 (comment).

@Stebalien Stebalien closed this May 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants