Page MenuHomeFreeBSD

dtrace: make 'ring' and 'fill' policies imply 'noswitch' flag
AbandonedPublic

Authored by avg on Dec 24 2021, 10:45 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 27, 10:09 AM
Unknown Object (File)
Thu, Apr 25, 11:32 PM
Unknown Object (File)
Thu, Apr 25, 6:46 PM
Unknown Object (File)
Thu, Apr 25, 2:50 PM
Unknown Object (File)
Mon, Apr 22, 3:40 AM
Unknown Object (File)
Sun, Apr 21, 6:27 PM
Unknown Object (File)
Dec 23 2023, 1:11 AM
Unknown Object (File)
May 23 2023, 3:47 PM

Details

Reviewers
markj
tsoome
Group Reviewers
DTrace
Summary

This should disable allocation of the second per-CPU principal buffer
which is never used. This will also enable additional asserts
for buffers that are never switched.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 43577
Build 40465: arc lint + arc unit

Event Timeline

avg requested review of this revision.Dec 24 2021, 10:45 AM

@avg This seems to introduce a kernel panic when -x bufpolicy=ring is used:

# dtrace -x bufpolicy=ring -n 'BEGIN { trace(basename((char *)rand())); }'

results in mstate->dtms_scratch_ptr being NULL in dtrace_dif_emulate(). Unsure why, but we should probably fix it. FWIW, there is a test in the DTrace test suite (common/safety/tst.basename.d) which catches this, and might be an argument to run the test suite in CI somewhere to catch such regressions a bit earlier.

CC @lwhsu?

Looking at the code a little bit further, it seems that

	/*
	 * For ring buffers and fill buffers, the scratch space is always
	 * the inactive buffer.
	 */
	mstate->dtms_scratch_base = (uintptr_t)buf->dtb_xamot;
	mstate->dtms_scratch_size = buf->dtb_size;
	mstate->dtms_scratch_ptr = mstate->dtms_scratch_base;

+

two places that have:

		if (flags & DTRACEBUF_NOSWITCH)
			continue;

		if ((buf->dtb_xamot = kmem_zalloc(size,
		    KM_NOSLEEP | KM_NORMALPRI)) == NULL)
			goto err;

are the cause. That is to say, buf->dtb_xamot likely does not get allocated, and therefore is NULL. However, when we have ringbuffers, we use that space as the scratch space. It seems like DTrace assumes that both buffers need to be allocated regardless of switching. I don't have time right now to test my understanding of the problems, but will have some more time tonight.

The comment at the end of dtrace_buffer_reserve() suggests that this commit is incorrect.

Looks like I assumed too much without paying attention to the scratch buffer.
I'll revert the change.

@avg It looks like this hit stable as well so will also need to be reverted before the release is cut: https://cgit.freebsd.org/src/commit/?h=stable/14&id=fb9c50f983ff6bdd6f33a22ae7d5b391435dd02a

Thanks!

All should be done. Thank you for reporting and diagnosing the problem.