Ring buffer code cleanups

[npitre]

• ring_buffer: delete redundant code
• ring_buffer: optimize the partial buffer loop
• ring_buffer: factorize almost identical code

[andyross]

Looks clean. Another advantage to new work being encouraged to reuse existing tools is that the existing tools get a round of review and audit too.

[andyross]

Style, since the test is now at the top of the loop, you could refactor this into a while via e.g.:

while((partial_size = ring_buf_get_claim(buf, &src, size)) != 0) {
   ...
}

IIRC someone (MISRA?) doesn't want us using assignment in while expressions though, so it might need a wrapper that takes the size by reference and returns a boolean? Or not, I don't remember what the rules are supposed to be anymore.

[npitre]

I'm sure MISRA is against assignments within condition tests. And the loop has 2 exit conditions: one at the top and one at the bottom.

[npitre]

The CI failure baffles me. Ideas anyone?

[peter-mitsis]

Regarding the CI failure ...

1. It is reproducible.
2. Locally, I see the failure arising after applying the 4th commit in the patch set.
3. I added a printk statement to where tracing_api_not_found is set to true. It suggests that the string 'sys_trace_k_mutex_unlock_exit' is not being found in the data.
4. More curiously, that same debug string is showing up even when the test passes--but after it reports a successful run. Something is definitely off.

[npitre]

I reduced the CI failure to the following patch applied to the otherwise
unmodified main branch:

diff --git a/lib/utils/ring_buffer.c b/lib/utils/ring_buffer.c
index 39e6e257e92..f5121f817c2 100644
--- a/lib/utils/ring_buffer.c
+++ b/lib/utils/ring_buffer.c
@@ -16,14 +16,25 @@ uint32_t ring_buf_put_claim(struct ring_buf *buf, uint8_t **data, uint32_t size)
 
 	base = buf->put_base;
 	wrap_size = buf->put_head - base;
+
+	/*
+	 * Same as the line after the if, however here this causes
+	 * "west build -p -b qemu_x86/atom tests/subsys/tracing/tracing_api -T tracing.transport.uart.async.test"
+	 * to fail when uncommented.
+	 */
+	free_space = buf->size - (buf->put_head - buf->get_tail);
+
 	if (unlikely(wrap_size >= buf->size)) {
 		/* put_base is not yet adjusted */
 		wrap_size -= buf->size;
 		base += buf->size;
 	}
+
+	/* duplicate of the line above */
+	free_space = buf->size - (buf->put_head - buf->get_tail);
+
 	wrap_size = buf->size - wrap_size;
 
-	free_space = ring_buf_space_get(buf);
 	size = MIN(size, free_space);
 	size = MIN(size, wrap_size);
 

Makes no sense to me. Assembly output is different with and without the
offending line of course but my x86 assembly fu isn't up to the task.

[cfriedt]

Would it make sense to factor this change out to a separate PR?

[npitre]

WTF? This was supposed to be part of PR #83871 which shows as being merged. Yet the main branch does not have it.

[cfriedt]

Uh-oh

[peter-mitsis]

I am presently suspecting that the CI failure is due to a problem with the test itself. In a previous comment I noted that the same debug string still shows up when the test passes (without the proposed changes). This is making me suspect that there is something amiss not with the ring buffer code but with the test, and that the proposed changes have altered the timing just enough such that the test will detect a failure before it completes. FWIW, the tracing buffer has a default size of 2048 (CONFIG_TRACING_BUFFER_SIZE). Increase this value and the test will pass.