Can we add something like

#define td_ast_pending(td, tda) ((td->td_ast & TDAI(tda)) != 0)

and use it instead?

Indeed, this appears to be unlocked. PMC_SOFT_CALL and PMC_SOFT_CALL_TF merely disable interrupts.

The comment above is quite cryptic now.

It looks like the kclear mechanism is only used by SU. At the same time, it seems weird that SU cannot specifically request its own handler, i.e., ast_kclear() will run all pending handlers flagged with ASTR_KCLEAR. What if another ASTR_KCLEAR handler is pending but not prepared to run in this context?

Can we pass a mask of desired handlers to ast_kclear() instead, so it looks like ast_kclear(TDAI(TDA_UFS))? Then the caller can control the set of handlers that run, and the purpose of the ast_kclear() is clear to someone reading the code. It might be simpler too, e.g., ASTR_KCLEAR is not needed.

Hmm, now I see that GEOM uses it too, but I think ast_kclear(TDAI2(TDA_UFS, TDA_GEOM)) is still clearer.

Why is "atomic" quoted? Seems like this read-and-clear is indeed atomic.

Declare at the beginning of the scope?

I decided to go with kclear() approach, because explicit bit set requires unrealted callers, in this case exit1(), to be aware of all existing ASTF_KCLEAR handlers.

I reformulated the comment.

Maybe it makes sense to support both. For exit1(), it makes some sense to handle all kclear ASTs, but not so much in NFS.

I do not see why NFS needs an exception. The geom AST is predicated on TDP_GEOM, both in HEAD and in the new code. Then practically, NFS server threads should not trigger TDP_GEOM at the first place, but if they did, I see no reason why they should not wait for geom events to finish propagation.

Extra newline.

The racct_enable check is unneeded: the handler is registered if and only if racct_enable is set, and it's never cleared. The check should go after the assertion.

Or if (!td_ast_pending(td, TDA_SIG) && !td_ast_pending(td, TDA_SUSPEND)), I expect the compiler would fuse the checks.

What's the purpose of the fence?

I would note here that deregistration does not drain threads executing the handler.

Practically, this is a designator for the place where something should be done if we are serious about making registration and deregistration fully async correct. Initially it was rel for ae_f assignment there and in ast_deregister(), and acq in ast_handler() load of f. But then I decided that the overhead is not worth it, esp. because this does not solve the issue completely.

Still I find it somewhat useful in the sense of place-keeper.

Fix referencing curthread twice?

struct thread *td = curthread;

Could hide the prefix inside the macro:

(TDA_##tda)

--HPS

curthread (actually the __curthread() inline function, the curthread macro is expanded to) is designated as pure2. This is an instruction for significantly smart compiler that the second call is not needed, it can reuse the value.

Anyway, I did as you suggested.

IMO it would be too obscure, I like to see the namespace of the identifier explicitly.

See https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=commit;h=f66589606ecfb0180ae7c0045feac6a6669bbd6b

I think yes, other platforms need this as well, but I worked around it in the very start of ast_handler():

	if (framep != NULL) {
		kmsan_mark(framep, sizeof(*framep), KMSAN_STATE_INITED);
		td->td_frame = framep;
	}

Why did this change to take an unused td argument?

Extra blank line.

As a followup commit I believe you could switch this one to ASTR_FREQUIRED or whatever the flag is to require TDA_KTRACE instead of doing it unconditionally. When it was inline it made sense to depend on the existing nested check in KTRUSERRET() rather than adding a new td_flags flag. However, since we now have a TDA_KTRACE flag anyway, we might as well check it and avoid the call. I haven't checked any other ASTR_UNCOND to see if they are in similar shape, but this one at least I think is safe to change. I agree with preserving existing behavior initially though and doing the change as a followup change.

Can this use td_ast_pending()?

This is bug, I forgot to change the curthread instance below.

Done.

Other ASTR_UNCOND are signals and hwpmc_soft. For signals, I am sure that it is required to do ASTR_UNCOND due to possible rescheduling of the process-globally delivered signals among the threads. We check for p->pendingcnt > 0 and !SIGISEMPTY(p->p_siglist) as one of the conditions.

For hwpmc_soft perhaps it is possible to remove ASTR_UNCOND, but this is definitely outside scope of testing for this change.

Should this be (curthread->td_owepreempt || td_ast_pending(curthread, TDA_SCHED))?