Perf event array map kernel-side implementation #4144

mikeagun · 2025-01-16T18:42:17Z

Description

Implements the kernel side of perf event array maps.

This includes the core and kernel changes for #658, with part of the user-side API skeleton.

Testing

Includes platform and execution context tests.

Documentation

Documented in code and PerfEventArray.md.

Installation

N/A

include/bpf_helper_defs.h

include/ebpf_api.h

include/ebpf_core_structs.h

include/ebpf_extension.h

libs/api/ebpf_api.cpp

libs/execution_context/ebpf_core.c

libs/runtime/unit/platform_unit_test.cpp

libs/runtime/ebpf_perf_event_array.c

libs/execution_context/ebpf_link.c

mikeagun · 2025-02-12T21:53:28Z

Moving to draft until the ring buffer refactoring is merged in #4204

mikeagun · 2025-02-25T02:24:40Z

This is currently blocked on #4204, as it uses the new ring buffer for wait-free reserve (at dispatch) in the per-cpu rings. Most of the refactoring to use the new ring buffer has been done, but after the new ring buffer implementation is merged I'll update+publish this again.

mikeagun · 2025-03-07T16:33:51Z

I split the context header support requirement out into #4267. After that passes all CICD tests I'll rebase this PR split into two commits - context header support then perf event array impl.

mikeagun · 2025-03-11T19:20:44Z

libs/execution_context/unit/execution_context_unit_test.cpp

+            ebpf_map_create(&map_name, &map_definition, (uintptr_t)ebpf_handle_invalid, &local_map) == EBPF_SUCCESS);
+        map.reset(local_map);
+    }
+    // TODO (before merge): implementme


Comment to track porting the necessary platform unit tests over to execution context unit tests using the new ebpf_maps based definition (vs. the old ebpf_perf_event_array based implementation).

From lines 1608 and 1610, it looks like this PR should still be in Draft?

include/ebpf_structs.h

shankarseal · 2025-03-19T02:59:42Z

tests/sample/bindmonitor_perf_event_array.c

+    case BIND_OPERATION_BIND:
+        if (ctx->app_id_end > ctx->app_id_start) {
+            (void)bpf_perf_event_output(
+                ctx, &process_map, flags, ctx->app_id_start, ctx->app_id_end - ctx->app_id_start);


bind context already has app_id_start and app_id_end as data begin and data end, which bpf_perf_event_output should prepend (assuming thats what the flags does). Why not add another random event payload?

It now passes the ctxlen flag to copy the app id and outputs a random value for the data argument.

libs/runtime/ebpf_ring_buffer.h

shankarseal · 2025-03-19T03:05:19Z

libs/runtime/ebpf_ring_buffer.h

+ */
+_Must_inspect_result_ ebpf_result_t
+ebpf_ring_buffer_reserve_exclusive(
+    _Inout_ ebpf_ring_buffer_t* ring_buffer, _Outptr_result_bytebuffer_(length) uint8_t** data, size_t length);


Are these rings "pinned" to a specific core?

Yes.

There is one ring per core and the whole reserve_exclusive+memcpy+submit is done at dispatch to the ring for the current cpu -- so there can only be one producer writing to a single ring at a time.

I followed the linux flag semantics - below dispatch you must specify the EBPF_MAP_FLAG_CURRENT_CPU flag, and at dispatch you can either use auto-cpu or manually specify the cpu you are currently running on.

I would suggest updating the routine description to reflect that - what the word "exclusive" means.

libs/runtime/ebpf_ring_buffer.c

shankarseal · 2025-03-19T03:19:11Z

libs/execution_context/ebpf_maps.c

+    ebpf_list_entry_t contexts;
+} ebpf_core_map_async_contexts_t;
+
+typedef struct _ebpf_core_ring_buffer_map


Please consider:

Suggested change

typedef struct _ebpf_core_ring_buffer_map

typedef struct _ebpf_core_perf_ring

{

ebpf_ring_buffer_t ring;

ebpf_core_map_async_contexts_t async;

} ebpf_core_ring_t;

typedef struct _ebpf_core_ring_buffer_map

{

ebpf_core_map_t core_map;

ebpf_core_ring_t ring;

} ebpf_core_ring_buffer_map_t;

I did try that out, but it doesn't currently save any lines of code, just adds a few lines/makes lines longer.

With some higher level restructuring it could maybe save a few lines, but the common functionality between perf array and ringbuf already uses shared code so the extra nesting doesn't simplify things.

Its just that it will look more symmetrical. I suggest you create an issue with these nit-pickish comments to be resolved a later time.

libs/execution_context/ebpf_maps.c

saxena-anurag · 2025-03-19T05:59:29Z

include/ebpf_api.h

+     * @retval EBPF_NO_MEMORY Out of memory.
+     */
+    _Must_inspect_result_ ebpf_result_t
+    ebpf_perf_event_array_map_write(


Can user mode write into perf_event_array map?

We don't support this yet via the libbpf interface (for ringbuf or perf array).

We already had the core functionality though for ringbuf, so I added the corresponding code for perf array.

saxena-anurag · 2025-03-19T06:17:40Z

libs/execution_context/ebpf_core.c


 Exit:
-    EBPF_OBJECT_RELEASE_REFERENCE((ebpf_core_object_t*)map);
+    if (reference_taken) {


nit: EBPF_OBJECT_RELEASE_REFERENCE already handles nullptr, do we need this check in that case?

No. I saw that some of the existing code used that pattern -- so I added it for the other updated ioctls. If there wasn't a perf/analyze reason for that though I can remove the reference_taken bool and checks.

The recommended pattern is to not add if checks here, to reduce the number of code paths. I.e., reduce code complexity and improve code coverage.

libs/execution_context/ebpf_maps.c

mikeagun · 2025-03-19T22:32:56Z

libs/execution_context/ebpf_maps.c

-static _Requires_lock_held_(ring_buffer_map->lock) void _ebpf_ring_buffer_map_signal_async_query_complete(
-    _Inout_ ebpf_core_ring_buffer_map_t* ring_buffer_map)
+typedef void
+map_async_query_complete_t(_In_ const ebpf_core_map_t* map, uint64_t index);


Note: Try again to update SAL to cover locking.

shankarseal · 2025-03-20T03:09:31Z

libs/execution_context/ebpf_maps.c

+_ebpf_ring_buffer_map_cancel_async_query(_In_ _Frees_ptr_ void* cancel_context)
+{
+    EBPF_LOG_ENTRY();
+    ebpf_core_map_async_query_context_t* context = (ebpf_core_map_async_query_context_t*)cancel_context;


since the cancel context is cast to the same type ebpf_core_map_async_query_context_t* both here and in line 2273, why not change the parameter type to ebpf_core_map_async_query_context_t* ?

shankarseal · 2025-03-21T01:57:18Z

libs/runtime/ebpf_ring_buffer.h

@@ -8,7 +8,17 @@

 CXPLAT_EXTERN_C_BEGIN

-typedef struct _ebpf_ring_buffer ebpf_ring_buffer_t;
+typedef struct _ebpf_ring_buffer


non blocking: consider creating a issue to make this struct opaque with "getter" functions. You can resolve this comment.

shankarseal · 2025-03-21T02:17:05Z

libs/execution_context/ebpf_maps.c

+ebpf_perf_event_array_map_output(
+    _In_ void* ctx, _Inout_ ebpf_map_t* map, uint64_t flags, _In_reads_bytes_(length) uint8_t* data, size_t length)
+{
+


Consider ebpf_perf_event_array_map_output calling ebpf_perf_event_array_map_output_simple.

shankarseal · 2025-03-21T02:20:23Z

libs/execution_context/ebpf_maps.h

+    /**
+     * @brief Write out a variable sized record to the perf event array.
+     *
+     * Writes a simple record to the ring for the current CPU.


Consider not using the word simple, and if you must, the routing description must explain what this means.

shankarseal · 2025-03-21T02:21:08Z

libs/execution_context/ebpf_maps.h

+     */
+    EBPF_INLINE_HINT
+    _Must_inspect_result_ ebpf_result_t
+    ebpf_perf_event_array_map_output(


Suggested change

ebpf_perf_event_array_map_output(

ebpf_perf_event_array_map_output_with_capture(

shankarseal · 2025-03-21T02:21:18Z

libs/execution_context/ebpf_maps.h

+     * @retval EBPF_OUT_OF_SPACE Unable to output to ring buffer due to inadequate space.
+     */
+    _Must_inspect_result_ ebpf_result_t
+    ebpf_perf_event_array_map_output_simple(


Suggested change

ebpf_perf_event_array_map_output_simple(

ebpf_perf_event_array_map_output(

dthaler · 2025-02-07T17:40:30Z

docs/eBpfExtensions.md

-passed to the eBPF program. To support this feature, the extension can use the macro `EBPF_CONTEXT_HEADER` to include
-the context header at the start of the program context structure. Even when the context header is added, the pointer
-passed to the eBPF program is after the context header.
+This is required for all extensions to support so the core can store runtime state needed by helpers.


If it's required, why do we need a flag? When would the flag be false?

dthaler · 2025-03-21T02:59:40Z

docs/PerfEventArray.md

@@ -1,12 +1,12 @@
 # Perf Event Array

-This document proposes support for a simplified version of the linux bpf map type BPF_MAP_TYPE_PERF_EVENT_ARRAY.
+This document describes the in-progress support for the bpf map type BPF_MAP_TYPE_PERF_EVENT_ARRAY.


if it's only "in progress" then can you reference an issue number here that tracks the completion of it?
Want to make sure this line gets updated by the time it's complete.

dthaler · 2025-03-21T03:01:49Z

include/ebpf_structs.h

+#define EBPF_MAP_FLAG_INDEX_MASK 0xffffffffULL
+#define EBPF_MAP_FLAG_INDEX_SHIFT 0
+#define EBPF_MAP_FLAG_CURRENT_CPU EBPF_MAP_FLAG_INDEX_MASK
+/* BPF_FUNC_perf_event_output flags for program types with data pointer in context */


Suggested change

/* BPF_FUNC_perf_event_output flags for program types with data pointer in context */

/* BPF_FUNC_perf_event_output flags for program types with data pointer in context. */

nit: For consistency with line 417

dthaler · 2025-03-21T03:03:39Z

libs/execution_context/ebpf_core.c


 Exit:
-    EBPF_OBJECT_RELEASE_REFERENCE((ebpf_core_object_t*)map);
+    if (reference_taken) {


The recommended pattern is to not add if checks here, to reduce the number of code paths. I.e., reduce code complexity and improve code coverage.

dthaler · 2025-03-21T03:05:39Z

libs/execution_context/ebpf_maps.h

+     *
+     * Flags is used to select the CPU to write to and the context data to copy.
+     * - EBPF_MAP_FLAG_INDEX_MASK - Mask for specifying cpu.
+     *   - EBPF_MAP_FLAG_CURRENT_CPU - Select current cpu.


Suggested change

* - EBPF_MAP_FLAG_CURRENT_CPU - Select current cpu.

* - EBPF_MAP_FLAG_CURRENT_CPU - Select current cpu.

dthaler · 2025-03-21T03:07:17Z

libs/execution_context/ebpf_maps.h

+     * Flags is used to select the CPU to write to and the context data to copy.
+     * - EBPF_MAP_FLAG_INDEX_MASK - Mask for specifying cpu.
+     *   - EBPF_MAP_FLAG_CURRENT_CPU - Select current cpu.
+     * - EBPF_MAP_FLAG_CTXLEN_MASK - Mask for specifying context length.


Why is "LEN" abbreviated when (say) "length" is spelled out as an argument?
The coding style guidelines in this project say to avoid abbreviations (unless it's for parity with Linux or to match abbreviations in the language or a library being called).

dthaler · 2025-03-21T03:07:55Z

libs/execution_context/ebpf_program.h

+    /**
+     * @brief Get the data start and end pointers from the program context.
+     *
+     *  @note Extension must support context headers.


Suggested change

* @note Extension must support context headers.

* @note Extension must support context headers.

Make doxygen line up. Also in line 495 and perhaps elsewhere.

dthaler · 2025-03-21T03:10:13Z

libs/execution_context/unit/execution_context_unit_test.cpp

+            ebpf_map_create(&map_name, &map_definition, (uintptr_t)ebpf_handle_invalid, &local_map) == EBPF_SUCCESS);
+        map.reset(local_map);
+    }
+    // TODO (before merge): implementme


From lines 1608 and 1610, it looks like this PR should still be in Draft?

mikeagun force-pushed the perfbuf-impl branch from 7623a4f to 10d4f86 Compare January 18, 2025 02:23

mikeagun mentioned this pull request Jan 27, 2025

Support direct mapped memory consumers for ring buffer and perf event array maps #4163

Open

mikeagun force-pushed the perfbuf-impl branch from d5f2cea to a5f9711 Compare January 29, 2025 17:41

mikeagun marked this pull request as ready for review February 4, 2025 19:58

mikeagun requested review from dthaler, poornagmsft, Alan-Jowett, saxena-anurag, shankarseal, dv-msft, matthewige and mtfriesen as code owners February 4, 2025 19:58

mikeagun changed the title ~~DRAFT Perf event array map implementation.~~ Perf event array map kernel-side implementation. Feb 4, 2025

Alan-Jowett reviewed Feb 5, 2025

View reviewed changes

include/bpf_helper_defs.h Outdated Show resolved Hide resolved

dthaler reviewed Feb 5, 2025

View reviewed changes

shankarseal reviewed Feb 6, 2025

View reviewed changes

libs/runtime/ebpf_perf_event_array.c Outdated Show resolved Hide resolved

shankarseal reviewed Feb 6, 2025

View reviewed changes

libs/runtime/ebpf_perf_event_array.c Outdated Show resolved Hide resolved

mikeagun commented Feb 7, 2025

View reviewed changes

libs/execution_context/ebpf_link.c Outdated Show resolved Hide resolved

mikeagun marked this pull request as draft February 12, 2025 21:54

mikeagun mentioned this pull request Feb 26, 2025

Design proposal for new ring buffer API #3848

Draft

mikeagun force-pushed the perfbuf-impl branch from 6513f71 to 547a6e1 Compare February 28, 2025 17:30

mikeagun marked this pull request as ready for review February 28, 2025 17:38

mikeagun requested review from LakshK98 and vpidatala94 as code owners February 28, 2025 17:38

mikeagun mentioned this pull request Mar 7, 2025

Make context header support mandatory #4267

Merged

mikeagun force-pushed the perfbuf-impl branch from dc44759 to 10b8ee7 Compare March 7, 2025 18:15

mikeagun changed the title ~~Perf event array map kernel-side implementation.~~ Perf event array map kernel-side implementation Mar 7, 2025

mikeagun commented Mar 11, 2025

View reviewed changes

Michael Agun added 5 commits March 17, 2025 11:20

Kernel-side changes for perf event array.

49035f3

Remove redundant tests.

fc9ef7e

Inline ebpf_perf_event_array into ebpf_maps.

d58146d

feedback

a8369b0

Consolidate perf array and ringbuf ioctls.

302a62b

mikeagun force-pushed the perfbuf-impl branch from 2b7985c to 302a62b Compare March 18, 2025 00:18

Michael Agun added 2 commits March 18, 2025 13:12

Refactor to consolidate ring and perf ioctls.

d2698e5

Analyze build fixes.

2dce71d

shankarseal reviewed Mar 19, 2025

View reviewed changes

include/ebpf_structs.h Outdated Show resolved Hide resolved

shankarseal reviewed Mar 19, 2025

View reviewed changes

libs/runtime/ebpf_ring_buffer.h Show resolved Hide resolved

shankarseal reviewed Mar 19, 2025

View reviewed changes

libs/runtime/ebpf_ring_buffer.c Outdated Show resolved Hide resolved

shankarseal reviewed Mar 19, 2025

View reviewed changes

libs/runtime/ebpf_ring_buffer.c Outdated Show resolved Hide resolved

shankarseal reviewed Mar 19, 2025

View reviewed changes

libs/execution_context/ebpf_maps.c Outdated Show resolved Hide resolved

saxena-anurag reviewed Mar 19, 2025

View reviewed changes

Feedback and test updates.

3206503

shankarseal reviewed Mar 19, 2025

View reviewed changes

libs/execution_context/ebpf_maps.c Show resolved Hide resolved

mikeagun commented Mar 19, 2025

View reviewed changes

shankarseal reviewed Mar 20, 2025

View reviewed changes

shankarseal reviewed Mar 21, 2025

View reviewed changes

shankarseal approved these changes Mar 21, 2025

View reviewed changes

dthaler reviewed Mar 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perf event array map kernel-side implementation #4144

Perf event array map kernel-side implementation #4144

mikeagun commented Jan 16, 2025 •

edited

Loading

mikeagun commented Feb 12, 2025

mikeagun commented Feb 25, 2025

mikeagun commented Mar 7, 2025

mikeagun Mar 11, 2025

dthaler Mar 21, 2025

shankarseal Mar 19, 2025

mikeagun Mar 19, 2025

shankarseal Mar 19, 2025

mikeagun Mar 19, 2025

shankarseal Mar 21, 2025

shankarseal Mar 19, 2025 •

edited

Loading

mikeagun Mar 19, 2025

shankarseal Mar 21, 2025

saxena-anurag Mar 19, 2025

mikeagun Mar 19, 2025

saxena-anurag Mar 19, 2025

mikeagun Mar 19, 2025

dthaler Mar 21, 2025

mikeagun Mar 19, 2025

shankarseal Mar 20, 2025

shankarseal Mar 21, 2025

shankarseal Mar 21, 2025

shankarseal Mar 21, 2025

shankarseal Mar 21, 2025

shankarseal Mar 21, 2025

dthaler Feb 7, 2025

dthaler Mar 21, 2025

dthaler Mar 21, 2025

dthaler Mar 21, 2025

dthaler Mar 21, 2025

dthaler Mar 21, 2025

dthaler Mar 21, 2025

dthaler Mar 21, 2025

-typedef struct _ebpf_core_ring_buffer_map
+typedef struct _ebpf_core_perf_ring
+{
+    ebpf_ring_buffer_t ring;
+    ebpf_core_map_async_contexts_t async;
+} ebpf_core_ring_t;
+typedef struct _ebpf_core_ring_buffer_map
+{
+    ebpf_core_map_t core_map;
+    ebpf_core_ring_t ring;
+} ebpf_core_ring_buffer_map_t;

	ebpf_perf_event_array_map_output(
	ebpf_perf_event_array_map_output_with_capture(

	ebpf_perf_event_array_map_output_simple(
	ebpf_perf_event_array_map_output(

	/* BPF_FUNC_perf_event_output flags for program types with data pointer in context */
	/* BPF_FUNC_perf_event_output flags for program types with data pointer in context. */

	* - EBPF_MAP_FLAG_CURRENT_CPU - Select current cpu.
	* - EBPF_MAP_FLAG_CURRENT_CPU - Select current cpu.

	* @note Extension must support context headers.
	* @note Extension must support context headers.

Perf event array map kernel-side implementation #4144

Are you sure you want to change the base?

Perf event array map kernel-side implementation #4144

Conversation

mikeagun commented Jan 16, 2025 • edited Loading

Description

Testing

Documentation

Installation

mikeagun commented Feb 12, 2025

mikeagun commented Feb 25, 2025

mikeagun commented Mar 7, 2025

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

shankarseal Mar 19, 2025 • edited Loading

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

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

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

mikeagun commented Jan 16, 2025 •

edited

Loading

shankarseal Mar 19, 2025 •

edited

Loading