Received-SPF: pass (google.com: domain of linux-kernel+bounces-204684-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <d04042cd-0d17-48e7-ae9f-9a4a409408ab@efficios.com>
Date: Thu, 6 Jun 2024 12:17:20 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 0/3] Tracepoints and static branch/call in Rust
To: Alice Ryhl <aliceryhl@google.com>
Cc: Steven Rostedt <rostedt@goodmis.org>,
 Masami Hiramatsu <mhiramat@kernel.org>, Peter Zijlstra
 <peterz@infradead.org>, Josh Poimboeuf <jpoimboe@kernel.org>,
 Jason Baron <jbaron@akamai.com>, Ard Biesheuvel <ardb@kernel.org>,
 Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>,
 Wedson Almeida Filho <wedsonaf@gmail.com>, Boqun Feng
 <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>,
 =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>,
 Benno Lossin <benno.lossin@proton.me>,
 Andreas Hindborg <a.hindborg@samsung.com>,
 linux-trace-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240606-tracepoint-v1-0-6551627bf51b@google.com>
 <cd4a58d9-3e0a-49d1-8a74-bc9d53fc2dfd@efficios.com>
 <CAH5fLgi4zs5ehDCEgkxPzaamNKn_2cP5+qH8KTy4ujdf2_D-vA@mail.gmail.com>
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Content-Language: en-US
In-Reply-To: <CAH5fLgi4zs5ehDCEgkxPzaamNKn_2cP5+qH8KTy4ujdf2_D-vA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 2024-06-06 11:46, Alice Ryhl wrote:
> On Thu, Jun 6, 2024 at 5:25 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> On 2024-06-06 11:05, Alice Ryhl wrote:
>>> This implementation implements support for static keys in Rust so that
>>> the actual static branch will end up in the Rust object file. However,
>>> it would also be possible to just wrap the trace_##name generated by
>>> __DECLARE_TRACE in an extern C function and then call that from Rust.
>>> This will simplify the Rust code by removing the need for static
>>> branches and calls, but it places the static branch behind an external
>>> call, which has performance implications.
>>
>> The tracepoints try very hard to minimize overhead of dormant tracepoints
>> so it is not frowned-upon to have them built into production binaries.
>> This is needed to make sure distribution vendors keep those tracepoints
>> in the kernel binaries that reach end-users.
>>
>> Adding a function call before evaluation of the static branch goes against
>> this major goal.
>>
>>>
>>> A possible middle ground would be to place just the __DO_TRACE body in
>>> an extern C function and to implement the Rust wrapper by doing the
>>> static branch in Rust, and then calling into C the code that contains
>>> __DO_TRACE when the tracepoint is active. However, this would need some
>>> changes to include/linux/tracepoint.h to generate and export a function
>>> containing the body of __DO_TRACE when the tracepoint should be callable
>>> from Rust.
>>
>> This tradeoff is more acceptable than having a function call before
>> evaluation of the static branch, but I wonder what is the upside of
>> this tradeoff compared to inlining the whole __DO_TRACE in Rust ?
>>
>>> So in general, there is a tradeoff between placing parts of the
>>> tracepoint (which is perf sensitive) behind an external call, and having
>>> code duplicated in both C and Rust (which must be kept in sync when
>>> changes are made). This is an important point that I would like feedback
>>> on from the C maintainers.
>>
>> I don't see how the duplication happens there: __DO_TRACE is meant to be
>> inlined into each C tracepoint caller site, so the code is already meant
>> to be duplicated. Having an explicit function wrapping the tracepoint
>> for Rust would just create an extra instance of __DO_TRACE if it happens
>> to be also inlined into C code.
>>
>> Or do you meant you would like to prevent having to duplicate the
>> implementation of __DO_TRACE in both C and Rust ?
>>
>> I'm not sure if you mean to prevent source code duplication between
>> C and Rust or duplication of binary code (instructions).
> 
> It's a question of maintenance burden. If you change how __DO_TRACE is
> implemented, then those changes must also be reflected in the Rust
> version. There's no issue in the binary code.

As long as it is only __DO_TRACE that is duplicated between C and Rust,
I don't see it as a large burden.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com