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

Set required_irql in sockops and sockaddr program information #3159

Merged
merged 2 commits into from
Jan 5, 2024

Conversation

Alan-Jowett
Copy link
Member

Description

The sockops and sockaddr program information are missing the required_irql fields. This causes bpf_prog_test_run_opts to invoke them at IRQL == PASSIVE_LEVEL instead of IRQL == DISPATCH_LEVEL.

Testing

CI/CD

Documentation

No.

Installation

No.

mtfriesen
mtfriesen previously approved these changes Jan 5, 2024
dv-msft
dv-msft previously approved these changes Jan 5, 2024
@@ -73,7 +73,7 @@ functions that override the global helper functions provided by the eBPF runtime
structure from provided data and context buffers.
* `context_destroy`: Pointer to `ebpf_program_context_destroy_t` function that destroys a program type specific
context structure and populates the returned data and context buffers.
* `required_irql`: IRQL that the eBPF program runs at.
* `required_irql`: IRQL that the eBPF program runs at when being invoked via bpf_prog_test_run_opts.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* `required_irql`: IRQL that the eBPF program runs at when being invoked via bpf_prog_test_run_opts.
* `required_irql`: IRQL at which the eBPF program is invoked by bpf_prog_test_run_opts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So does this mean the IRQL used by bpf_prog_test_run_opts, and the IRQL from a real invocation can be different? If so, the label "required_irql' seems wrong to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Longer term there is a need to have the bpf_prog_test_run_opts specify what IRQL to run the BPF program at and for the extension to be able to validate this, which will be addressed by #3157, but that is a larger change. The goal of this is to unblock the BPF performance testing so we can detect regressions.

Before #3157 can be implemented, there needs to be a decision on:

  1. Do we need to update the version of the interface for pre-release eBPF core or is a breaking changer acceptable.
  2. How can this be passed this via the libbpf API in a way that maintains compatability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed that the label is wrong? If so, why not change it now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I take it changing the label would be a breaking change, yes?

@Alan-Jowett Alan-Jowett added this pull request to the merge queue Jan 5, 2024
Merged via the queue into microsoft:main with commit 3c9c72a Jan 5, 2024
76 checks passed
@Alan-Jowett Alan-Jowett deleted the set_required_irql branch January 5, 2024 22:02
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.

4 participants