aya-bpf: Add docs for the main lib and few map types #244

vadorovsky · 2022-04-12T10:27:53Z

This change adds a summary of the aya-bpf lib and docs for:

hash map
lpm trie
perf event arrays

Signed-off-by: Michal Rostecki [email protected]

alessandrod

Thanks so much for getting this started!

See my comments. I think the reference docs are generally ok, but I think we need better examples.

I wouldn't worry about the module level documentation too much for now. I think we'll have to be very thorough when we get to that, and possibly cross reference examples with the user space API. I think that's going to be a lot of work so we can probably do it separately/at the end once everything else is in place.

alessandrod · 2022-04-17T10:48:38Z

bpf/aya-bpf/src/maps/hash_map.rs

+/// # use aya_bpf::programs::LsmContext;
+///
+/// #[map]
+/// static mut MY_MAP: HashMap<u32, u32> = HashMap::with_max_entries(1024, 0);


I think it would be better to have concrete examples. They can still be just 2-3 lines, but with actual (simple) use cases.

Perhaps given the limits on everyone's time, and the fact that this PR ended up going stale would suggest that follow-up items for expanding this documentation would be good: e.g. we could accept this as-is for now as "better than nothing" and then follow-up issues to extend the documentation further could be created, tagged as good-first-issue so as to provide some smaller, simple places new contributors could jump in and contribute?

alessandrod · 2022-04-17T10:49:06Z

bpf/aya-bpf/src/maps/hash_map.rs

+/// #[map]
+/// static mut MY_MAP: HashMap<u32, u32> = HashMap::with_max_entries(1024, 0);
+///
+/// # unsafe fn try_test(ctx: &LsmContext) -> Result<i32, i32> {


In all the examples, I think we should explicitly show unsafe usage, so the wrapper funcs shouldn't be unsafe

+1 if I'm understanding the original intention here it was to try and "scare" the caller of the code into noticing that what they're doing is unsafe, and I get that. However in Rust it's more idiomatic to "wrap" the unsafe functionality with code that carefully considers and accounts for all the potential safety issues and then provide a safe interface to that functionality.

Given my impression that the intention here was meant to alert the reader of the safety issues, I think a good compromise here would be to place the unsafe around the exact thing it needs to be around but make the function itself safe as @alessandrod suggests, but THEN add comments to that code which explains why we must currently do that as unsafe (extra credit if there's an open issue we can link to which tracks making that code safe in the future).

alessandrod · 2022-04-17T10:52:50Z

bpf/aya-bpf/src/maps/hash_map.rs

@@ -19,6 +41,16 @@ pub struct HashMap<K, V> {
 }

 impl<K, V> HashMap<K, V> {
+    /// Creates a `HashMap` with the maximum number of elements.


Creates an empty `HashMap<K, V>` with the specified maximum number of elements.

Tip: for inspiration on how to phrase the docs for the userspace API, I took inspiration from similar stdlib docs, eg Vec::with_capacity() is a good analogous of ::with_max_entries().

alessandrod · 2022-04-17T10:54:45Z

bpf/aya-bpf/src/maps/hash_map.rs

@@ -27,6 +59,17 @@ impl<K, V> HashMap<K, V> {
        }
    }

+    /// Creates a `HashMap` pinned in the BPFFS filesystem, with the maximum


Creates an empty `HashMap<K, V>` with the specified maximum number of elements, and pins it to the BPF file system (bpffs).

alessandrod · 2022-04-17T10:56:35Z

bpf/aya-bpf/src/maps/hash_map.rs

+    ///
+    /// # unsafe fn try_test(ctx: &LsmContext) -> Result<i32, i32> {
+    /// let key: u32 = 13;
+    /// if let Some(mut value) = MY_MAP.get_mut(&key) {


Especially for mutable methods, I think it's important to show examples that make sense and aren't racy. And the documentation should point out that all the mutable methods are potentially racy if the keys aren't carefully chosen.

+1 we should be careful about examples that might not have clear implications to code readers, as example code often unfortunately finds it's way into production.

netlify · 2022-05-20T12:10:00Z

❌ Deploy Preview for aya-rs-docs failed.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`61dbd64`
🔍 Latest deploy log	https://app.netlify.com/sites/aya-rs-docs/deploys/6401c61ae9b44d0008b26c2e

shaneutt

Overall Looking good: this significantly improves documentation.

I think for docs that could simply use more elaboration it may be fair to create follow-up issues as good-first-issues for newcomers to get a chance to make a small change. However I think there's a couple examples here that need more explanation, or perhaps some re-working before we merge.

bpf/aya-bpf/src/lib.rs

shaneutt · 2022-12-19T22:14:10Z

bpf/aya-bpf/src/maps/hash_map.rs

+/// # use aya_bpf::programs::LsmContext;
+///
+/// #[map]
+/// static mut MY_MAP: HashMap<u32, u32> = HashMap::with_max_entries(1024, 0);


Perhaps given the limits on everyone's time, and the fact that this PR ended up going stale would suggest that follow-up items for expanding this documentation would be good: e.g. we could accept this as-is for now as "better than nothing" and then follow-up issues to extend the documentation further could be created, tagged as good-first-issue so as to provide some smaller, simple places new contributors could jump in and contribute?

shaneutt · 2022-12-19T22:29:32Z

bpf/aya-bpf/src/maps/hash_map.rs

+/// #[map]
+/// static mut MY_MAP: HashMap<u32, u32> = HashMap::with_max_entries(1024, 0);
+///
+/// # unsafe fn try_test(ctx: &LsmContext) -> Result<i32, i32> {


+1 if I'm understanding the original intention here it was to try and "scare" the caller of the code into noticing that what they're doing is unsafe, and I get that. However in Rust it's more idiomatic to "wrap" the unsafe functionality with code that carefully considers and accounts for all the potential safety issues and then provide a safe interface to that functionality.

Given my impression that the intention here was meant to alert the reader of the safety issues, I think a good compromise here would be to place the unsafe around the exact thing it needs to be around but make the function itself safe as @alessandrod suggests, but THEN add comments to that code which explains why we must currently do that as unsafe (extra credit if there's an open issue we can link to which tracks making that code safe in the future).

shaneutt · 2022-12-19T22:32:12Z

bpf/aya-bpf/src/maps/hash_map.rs

+    ///
+    /// # unsafe fn try_test(ctx: &LsmContext) -> Result<i32, i32> {
+    /// let key: u32 = 13;
+    /// if let Some(mut value) = MY_MAP.get_mut(&key) {


+1 we should be careful about examples that might not have clear implications to code readers, as example code often unfortunately finds it's way into production.

Signed-off-by: Michal Rostecki <[email protected]>

vadorovsky · 2023-03-06T11:23:47Z

Closing it. I will submit smaller PRs per map type every day Attempting to document everything at once isn't really working.

vadorovsky force-pushed the aya-bpf-docs-1 branch 2 times, most recently from be662d2 to fee288a Compare April 13, 2022 08:43

vadorovsky changed the title ~~aya-bpf: Add docs for the main lib annd few map types~~ aya-bpf: Add docs for the main lib and few map types Apr 13, 2022

vadorovsky force-pushed the aya-bpf-docs-1 branch 2 times, most recently from 82ef66d to d0263e5 Compare April 17, 2022 10:09

alessandrod requested changes Apr 17, 2022

View reviewed changes

dave-tucker added documentation Improvements or additions to documentation aya-bpf This is about aya-bpf (kernel) labels Apr 28, 2022

vadorovsky force-pushed the aya-bpf-docs-1 branch from d0263e5 to 36b9cae Compare May 20, 2022 12:09

shaneutt reviewed Dec 19, 2022

View reviewed changes

dave-tucker added this to the March Release 🗓️ milestone Feb 23, 2023

vadorovsky force-pushed the aya-bpf-docs-1 branch 2 times, most recently from 5de6a05 to e0f2aed Compare February 27, 2023 15:43

vadorovsky mentioned this pull request Feb 27, 2023

Implement XDP map types #527

Merged

3 tasks

aya-bpf: Add docs to map types and methods

61dbd64

Signed-off-by: Michal Rostecki <[email protected]>

vadorovsky force-pushed the aya-bpf-docs-1 branch from e0f2aed to 61dbd64 Compare March 3, 2023 10:04

vadorovsky closed this Mar 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aya-bpf: Add docs for the main lib and few map types #244

aya-bpf: Add docs for the main lib and few map types #244

vadorovsky commented Apr 12, 2022

alessandrod left a comment

alessandrod Apr 17, 2022

shaneutt Dec 19, 2022

alessandrod Apr 17, 2022

shaneutt Dec 19, 2022

alessandrod Apr 17, 2022

alessandrod Apr 17, 2022

alessandrod Apr 17, 2022

shaneutt Dec 19, 2022

netlify bot commented May 20, 2022 •

edited

Loading

shaneutt left a comment

shaneutt Dec 19, 2022

shaneutt Dec 19, 2022

shaneutt Dec 19, 2022

vadorovsky commented Mar 6, 2023 •

edited

Loading

aya-bpf: Add docs for the main lib and few map types #244

aya-bpf: Add docs for the main lib and few map types #244

Conversation

vadorovsky commented Apr 12, 2022

alessandrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented May 20, 2022 • edited Loading

❌ Deploy Preview for aya-rs-docs failed.

shaneutt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadorovsky commented Mar 6, 2023 • edited Loading

netlify bot commented May 20, 2022 •

edited

Loading

vadorovsky commented Mar 6, 2023 •

edited

Loading