Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <c5c39d2a-a841-4a27-b072-7b190e6838cb@efficios.com>
Date:   Sun, 10 Dec 2023 12:28:32 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] tracing: Allow for max buffer data size trace_marker
 writes
Content-Language: en-US
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Linux Trace Kernel <linux-trace-kernel@vger.kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>
References: <20231209175003.63db40ab@gandalf.local.home>
 <2683467e-cadb-4bb8-8c50-87ef052edacb@efficios.com>
 <20231210103009.29010d00@gandalf.local.home>
 <a684a5f8-9a60-4e16-93f5-747117d08371@efficios.com>
 <20231210113829.780c7097@gandalf.local.home>
From:   Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
In-Reply-To: <20231210113829.780c7097@gandalf.local.home>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2023-12-10 11:38, Steven Rostedt wrote:
> On Sun, 10 Dec 2023 11:07:22 -0500
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
>>> It just allows more to be written in one go.
>>>
>>> I don't see why the tests need to cover this or detect this change.
>>
>> If the purpose of this change is to ensure that the entire
>> trace marker payload is shown within a single event, then
>> there should be a test which exercises this, and which
>> validates that the end result is that the entire payload
>> is indeed shown within a single event record.
> 
> No, the purpose of the change is not to do that, because there can always
> be a bigger trace marker write than a single event can hold. This is the
> way it has always worked. This is an optimization or "enhancement". The 1KB
> restriction was actually because of a previous implementation years ago
> (before selftests even existed) that wrote into a temp buffer before
> copying into the ring buffer. But since we now can copy directly into the
> ring buffer, there's no reason not to use the maximum that the ring buffer
> can accept.

My point is that the difference between the new "enhanced" behavior
and the previous behavior is not tested for.

> 
>>
>> Otherwise there is no permanent validation that this change
>> indeed does what it is intended to do, so it can regress
>> at any time without any test noticing it.
> 
> What regress? The amount of a trace_marker write that can make it into a
> the buffer in one go? Now, I agree that we should have a test to make sure
> that all of the trace marker write gets into the buffer.

Yes. This is pretty much my point.


> But it's always
> been allowed to break up that write however it wanted to.

And the enhanced behavior extends the amount of data that can get
written into a single sub-buffer, and this is not tested.

> 
> Note, because different architectures have different page sizes, how much
> that can make it in one go is architecture dependent. So you can have a
> "regression" by simply running your application on a different architecture.

Which is why in the following patches you have expressing the subbuffer
size as bytes rather than pages is important at the ABI level. It
facilitates portability of tests, and decreases documentation / user
burden.

> Again, it's not a requirement, it's just an enhancement.

How does this have anything to do with dispensing from testing the
new behavior ? If the new behavior has a bug that causes it to
silently truncate the trace marker payloads, how do you catch it
with the current tests ?

Thanks,

Mathieu

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