I mean that run is scope-bound, the thread is run for limited block of code, and then put back to suspension. It is not intended to be fully runnable state, e.g. an attempt to return to userspace would cause invariants check.

Like flashlight flash.

TD_IS_SUSPENDED should be enough, but ok.

You mean, 'sleepq_remove' as the possible name?

Issue there is that PT_COREDUMP might select a target thread that is suspended while it is already put on sleepqueue. This is the sleepq_catch_signals()->sig_ast_needsigchk()->issignal()->ptracestop() case of ptracestop() suspension. But sv_coredump() needs to sleep for misc. VFS resources like rangelocks/vnode/buffer locks etc. So I remove the thread from the sleepq if any (resulting in the usual spurious EINTR from ptrace(2) anyway).

No, I cannot select a better thread generally, this might be the only thread at all.

Nested there denotes the situation of sleep while we are on sleepqueue, if not done.

Also added missed wmsg

Well, this is the place which informs other threads to block.

I think this is stray/mismerge.

Yes this was the intent, initial version lack any writeability check on fd, and fget_write() is the same method as used e.g. by write(2).

Does not make sense to limit it to root, since, as you notes, PT_IO allows to read the same mappings anyway.

It is done by the victim thread, see the end of ptrace_coredump().

The
.Fa addr
argument...

Should we indicate what type(s) of file descriptors are permissible?

Can we clarify non-dumpable?
Do we need a comment in mmap(2) alongside MAP_NOCORE? I assume there are multiple reasons MAP_NOCORE may be used.

Maybe "must already be stopped"?

So if global compress_user_cores is not set PT_COREDUMP cores are also uncompressed?

I think convention is for non-arg flags to come first, all together? So [-fk] [-c core]?

Probably "Use the ptrace(2) PT_COREDUMP kernel facility to..."

Initial intent was to make a flag to adjust global settings. But ok, I now made PC_COMPRESS completely independent from the configuration for automatic core dump.

Next logical step would be to add an option to gcore to request compressed dump, but then it raises the question why this option is only implemented for -k.

So in some error cases this is not true. How does userspace know whether it needs to wait?

If it's completely independent, shouldn't the check be if (((flags & SVC_PT_COREDUMP) == 0 && compress_user_cores) || (flags & (SVC_PT_COREDUMP | SVC_NOCOMPRESS)) == SVC_PT_COREDUMP)?

Also we may use either zlib or zstd for compression, and this is controlled by a different global setting.

Why is the caller responsible for kicking the swapper? It seems reasonable to handle it here. I also don't see why the caller drops the proc lock to call kick_proc0().

Yes, sleepq_remove() seems like a better name for this. I don't have a strong feeling about it though.

Is there some fundamental reason we couldn't support dumping to a named pipe as well? It could be useful for doing compression in userspace, for instance. I am not suggesting to do it in this review.

Ok.

Shouldn't the wait happen only if PT_COREDUMP was successful?

I propose to not care about the difference, see updated man page and my other reply.

I think it is good enough for now to not provide a selection of compression method through flags.

IMO sleepq_remove() is too generic, this situation is specific. I want to leave the generic name unused.

I did not inspected code to claim that we can.

One easy to see reason is that we do not want a user process (reader) to be able to block the process in this state for indefinite time. This might cause cascaded debugger' hangs.

No, in all cases when the victim thread was unsuspended. As you noted in ptrace(2) comment, it is hard for userspace to know exactly when to wait, but also it is quite dirty for kernel either to try to suppress clearing of P_WAITED, or to wait internally.

I think the acceptable solution is to wait there with WNOHANG. It would eat stray notification if there is any, and waitpid(2) returns 0 when there was no process to report.

I admit that it is inelegant but it is the least evil as I see the problem.

I believe there is no race here if a thread was unsuspended: after ptrace(PT_COREDUMP) returns, the use of the proc lock in ptracestop() ensures that the new stop event is immediately visible to a waitpid(WNOHANG) caller. Is that correct? I am just concerned about waitpid(WNOHANG) returning a false negative, i.e., there is a new stop event to consume but waitpid(WNOHANG) did not consume it because the re-suspension happens asynchronously.

I'd be fine with simply not supporting compression at all through this interface, at least initially.

In fact I believe the interface is somewhat broken: if SVC_NOCOMPRESS is not set, we will try to use the value of compress_user_cores as the compression algorithm, and if it is not configured compressor_init() will return an error. I think it was correct before my earlier suggestion, sorry.

At the very least I think this knob, kern.compress_user_cores, should be referenced in the documentation.

Ok, but the debugger can decide who the reader is. One can imagine e.g., gcore that uses a named pipe to stream the core dump through a compression library, using a second thread to read from the pipe. You don't even need a named pipe for this.

The waitpid(WNOHANG) should be executed atfer ptrace(PT_COREDUMP) returned. The unsuspended thread suspends itself, which generates the event by thread_suspend_switch()->thread_stopped()->childproc_stopped() before wakeup.

So you are right that there is no race, but the main reason is that the event generation is in fact synchronous.

It still means that user SIGSTOPping the debugger leaves debuggee in unacceptable state. Lets postpone this till real consumers appear.

Hm, yes. I think I will not change anything, because indeed the process lock is held. But the sleep/wakeup there can be changed to p->p_pptr wchan, and then wakeup in ptrace_coredump() is not needed.

By 'precise' I mean that it match the kernel-generated dumps both now and in future, if e.g. some notes or new kinds of segments are added to the kernel dumper, which not yet copied to gcore.

Why not check td2->td_proc != p before calling proc_can_trace()?