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

Allows user space to update/lookup map elements under bpf_spin_lock #473

Merged
merged 1 commit into from
Nov 10, 2021
Merged

Allows user space to update/lookup map elements under bpf_spin_lock #473

merged 1 commit into from
Nov 10, 2021

Conversation

mehrdadrad
Copy link
Contributor

@mehrdadrad mehrdadrad commented Nov 3, 2021

  • Added FLock constant (BPF_F_LOCK)
  • Added three new map methods to support lookup with FLock:
    • LookupWithFlags
    • LookupBytesWithFlags
    • LookupAndDeleteWithFlags
  • Added tests with test data.

Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

This adds a lot of API for a pretty niche use case. Could you instead detect whether a map contains a spinlock at creation time and infer the need for BPF_F_LOCK? This will be somewhat complicated by LoadPinnedMap which should also work with spinlock maps.

Alternatively we should investigate a single new method LookupOpts(key, valueOut, opts). Opts could be:

struct Opts {
    Delete bool
    Lock bool
}

That would avoid so much new API as well.

@mehrdadrad
Copy link
Contributor Author

mehrdadrad commented Nov 4, 2021

@lmb re: detect whether a map contains a spinlock
I was thinking about it but why BPF_F_LOCK is an option which it MUST pass from user space through BPF command? why doesn't the kernel handle it by itself?
I just tried bpftool and looks we can detect a map with bpf_spin_lock through BTF:

[9] STRUCT 'hash_elem' size=8 vlen=2
        'cnt' type_id=2 bits_offset=0
        'lock' type_id=10 bits_offset=32
[10] STRUCT 'bpf_spin_lock' size=4 vlen=1
        'val' type_id=6 bits_offset=0

 
In meanwhile the libbpf added more APIs as below if we follow them:

  • int bpf_map_lookup_and_delete_elem_flags(int fd, const void *key, void *value, __u64 flags)
  • int bpf_map_lookup_elem_flags(int fd, const void *key, void *value, __u64 flags)

Your alternative option can be like below:
LookupOpts should be support:

  • BPF_MAP_LOOKUP_AND_DELETE_ELEM
  • BPF_MAP_LOOKUP_ELEM
LookupOptions struct { 
   Flags MapUpdateFlags 
   Delete bool
}
LookupOpts(key, valueOut interface{}, opts LookupOptions) error

The last thing, we can add at least two of APIs which I already did same as libbpf.

Any thoughts?

@lmb
Copy link
Collaborator

lmb commented Nov 4, 2021

You're right, since it's possible to do a lookup without BPF_F_LOCK always adding it doesn't make sense. Let's scrap the BTF idea.

The best thing we can do (imo):

  • LookupWithFlags
  • LookupAndDeleteWithFlags

There should be a new type MapLookupFlags uint64 though, with a constant LookupLock.

cc @ti-mo

map.go Outdated Show resolved Hide resolved
map.go Outdated Show resolved Hide resolved
map.go Outdated Show resolved Hide resolved
@mehrdadrad mehrdadrad requested a review from lmb November 5, 2021 05:15
@lmb
Copy link
Collaborator

lmb commented Nov 5, 2021

Can you squash your changes please?

@lmb
Copy link
Collaborator

lmb commented Nov 10, 2021

Thanks!

@lmb lmb merged commit eced84a into cilium:master Nov 10, 2021
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