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

Add introduction to eBPF #76

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Add introduction to eBPF #76

merged 1 commit into from
Nov 13, 2024

Conversation

athos-ribeiro
Copy link
Contributor

This is a short introduction to eBPF, (hopefully) in a Ubuntu server user context. I was not sure whether I should expand on the examples and move them to a different section (e.g., How to) or if I should simply omit them and save them for a how-to guide in the future. Still, I kept them short and they are what they are supposed to be, just examples :)

@athos-ribeiro athos-ribeiro requested a review from s-makin November 7, 2024 19:15
@s-makin
Copy link
Collaborator

s-makin commented Nov 8, 2024

This is a short introduction to eBPF, (hopefully) in a Ubuntu server user context. I was not sure whether I should expand on the examples and move them to a different section (e.g., How to) or if I should simply omit them and save them for a how-to guide in the future. Still, I kept them short and they are what they are supposed to be, just examples :)

I think it's fine to have these examples here - they're being used to illustrate a point, not to provide a sequence of instructions :) I'd say leave them as they are, no need to remove.

Copy link
Collaborator

@s-makin s-makin left a comment

Choose a reason for hiding this comment

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

Really nice article - I've left some minor comments/suggestions and formatting nits

explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Show resolved Hide resolved
explanation/intro-to/ebpf.md Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
@athos-ribeiro
Copy link
Contributor Author

Thanks for the review, @s-makin

I addressed most of your points and commented on the ones I could not address :)

Copy link
Collaborator

@s-makin s-makin left a comment

Choose a reason for hiding this comment

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

I've done round 2, it's looking nice :D I've found a couple of tiny nits, and I've added comments to the discussions

explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
After the verification step, the bytecode is Just-In-Time (JIT) compiled into
machine-code to optimize the program's execution.

eBPF is a powerful tool for server and system administrators. It provides the
Copy link
Collaborator

Choose a reason for hiding this comment

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

The new title is good and suits the rest of the content, but I'd still prefer this first paragraph to be near the top (in the intro). It basically provides all the justification (the "why should I care?") that the reader is looking for to know whether it's relevant or interesting for them. Imo, once a reader is halfway down the page, they've already decided they're interested (and the "why" is a waste of their time at that point). If they don't get halfway down because we didn't provide a strong hook then they'll never see that this is relevant for them. This paragraph does a great job of explaining the "why", so I'd rather not bury it, but rather put it right in front of the reader's eyes :)

explanation/introduction-to.rst Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
@athos-ribeiro
Copy link
Contributor Author

Hi Sally! I addressed most of your comments again and replied your questions. Let me know how it looks now :)

Copy link
Contributor

@cpaelzer cpaelzer left a comment

Choose a reason for hiding this comment

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

I've left a few small and even fewer big comments and suggestions.
Please get in touch if the bigger changes need discussion or if you disagree to my suggestion.

explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
explanation/intro-to/ebpf.md Show resolved Hide resolved
explanation/intro-to/ebpf.md Outdated Show resolved Hide resolved
Signed-off-by: Athos Ribeiro <[email protected]>
@athos-ribeiro
Copy link
Contributor Author

Thanks, Christian!

I applied all your changes and pushed them here

Copy link
Contributor

@cpaelzer cpaelzer left a comment

Choose a reason for hiding this comment

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

Final check:

  • The "why one should care" is now at the start - was a request of @s-makin
  • The same section got my suggested extensions which makes it even more "you want to care"
  • The extended examples and their evolution into self-modifying has landed
  • The extended references have landed
  • All other smaller changes requested have been landed

So all content is good,
we have two language questions left open and my new content needs the eyes of @s-makin for style and words.
I've pinged trying to land this rather soon.

@cpaelzer
Copy link
Contributor

I've prepared a small fixup which is ready to land together with all of this here.
Merging yours and then adding mine right away.

@cpaelzer cpaelzer merged commit c3eeb5d into canonical:main Nov 13, 2024
1 check passed
cpaelzer added a commit that referenced this pull request Nov 13, 2024
Sally and I had an interactive session closing out the few open
questions and phrasing of PR #76. Combined with all the feedback Athos
already integrated to the current state of his branch we can merge it now.

Changes:
- update structure of the intro
- rebalance headers
- guard arguments and file paths with backticks
- regroup the "modify to your needs" section
- several "ensure readability" rewrite of sentences
- capitalize QEMU and OpenStack correctly
- make all mention of bpf, BPF, ... -> eBPF

Signed-off-by: Christian Ehrhardt <[email protected]>
@cpaelzer
Copy link
Contributor

FYI 6af3763 is the follow up that landed as well.

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.

3 participants