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

Add hashmaps #215

Merged
merged 4 commits into from
Nov 11, 2021
Merged

Add hashmaps #215

merged 4 commits into from
Nov 11, 2021

Conversation

rhdxmr
Copy link
Collaborator

@rhdxmr rhdxmr commented Nov 9, 2021

Hello @rsdy
Can you review this PR?

1

Add hash table maps for both BPF programs and userspace programs.

  • PerCpuHashMap
  • LruHashMap
  • LruPerCpuHashMap

All structures provide similar methods.
For BPF programs, get, get_mut, set, delete, and get_val are implemented.
For userspace programs new, set, get, delete and iter methods are implemented.

2

Check Map is valid BPF map type (BPF_MAP_TYPE_HASH or BPF_MAP_TYPE_PERF_EVENT_ARRAY) when HashMap::new function is called.

3

Add derive Clone, Debug attribute to PerCpuValues.

4

Add hashmaps example program.

Signed-off-by: Junyeong Jeong <rhdxmr@gmail.com>
Signed-off-by: Junyeong Jeong <rhdxmr@gmail.com>
Signed-off-by: Junyeong Jeong <rhdxmr@gmail.com>
Signed-off-by: Junyeong Jeong <rhdxmr@gmail.com>
Copy link
Collaborator

@rsdy rsdy left a comment

Choose a reason for hiding this comment

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

I like this, and it looks thorough.

However, it seems like struct alignment is a contentious issue. Is there a way we could document a recommendation on using a particular alignment using the #[repr(align(8))] proc macro for instance?

It seems like it will be pretty cumbersome to use hashtables, as unaligned structs can be arbitrary in how they work with the verifier.

@rhdxmr
Copy link
Collaborator Author

rhdxmr commented Nov 10, 2021

#[repr(align(8))] does not affect the item if the alignment of the item is greater than 8 bytes.
And it's okay that items have smaller alignments 1, 2, or 4 because 8 bytes alignment always fits to those alignments.
So using that macro is not helpful.

I wrote a comment about the alignment at #150. I hope we have some documentation that states this limit clearly.

@rhdxmr
Copy link
Collaborator Author

rhdxmr commented Nov 10, 2021

I also considered raising compilation error if the alignment of the value is greater than 8 bytes.
But, isn't it too harsh?
I am not sure this behavior is desirable..

@rhdxmr
Copy link
Collaborator Author

rhdxmr commented Nov 11, 2021

@rsdy
I am going to merge this. Thanks

@rhdxmr rhdxmr merged commit afb8b8e into main Nov 11, 2021
@rhdxmr rhdxmr deleted the add-hashmaps branch November 11, 2021 09:45
@rsdy
Copy link
Collaborator

rsdy commented Nov 11, 2021

I also considered raising compilation error if the alignment of the value is greater than 8 bytes.

I'd like to see what kind of false positives this might trigger. I think if we are certain that this locks in correct behaviour, I'd be inclined to make it a hard error with a good error message than propagating failure to the verifier.

@rhdxmr
Copy link
Collaborator Author

rhdxmr commented Nov 11, 2021

@rsdy
I agree with you.

So I am going to add a feature of raising compilation error if the value has
a big alignment over 8 bytes.

This is the basis why the item whose alignment exceeds 8 bytes are banned.

  1. In official Rust documents, it is stated that misaligned pointers or references
    lead to undefined behavior.
  2. In BPF programs, the pointer returned by bpf_map_lookup_elem is misaligned
    because the value the pointer points to is merely 8 bytes aligned data.
  3. Hence, creating reference of the value, or dereferencing the pointer incurs
    undefined behavior.

@rsdy
Copy link
Collaborator

rsdy commented Nov 11, 2021

@rhdxmr that sounds promising, and quite clear cut.

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.

2 participants