queue.h: Reorder STAILQ_INSERT_TAIL

[des]

Summary

The current implementation briefly violates the tail invariant. This
is not usually an issue, but if an insert is in flight when a panic
occurs, we may then trip the invariant while dumping core.

MFC after: 1 week
Sponsored by: Klara, Inc.
Sponsored by: NetApp, Inc.

[jhb]

In particular, is the invariant you are tripping over QCMD_STAILQ_CHECK_TAIL?

[olce]

In D55819#1277146, @jhb wrote:

In particular, is the invariant you are tripping over QCMD_STAILQ_CHECK_TAIL?

If it is, then that change should fix it. In any case, the change is correct in the sense that all that has been working up to now will continue to work with it (excluding racy executions).

sys/sys/queue.h

502

You may as well dodge the (small) bullet in full by being completely explicit about what stqh_last is.

[olce]

Oh, and proposed commit's message herald line: "queue.h: Make STAILQ_INSERT_TAIL() maintain the tail invariant at all times".

[des]

In D55819#1277146, @jhb wrote:

In particular, is the invariant you are tripping over QCMD_STAILQ_CHECK_TAIL?

Yes. We weren't able to get to the bottom of the panic we saw (it's hard to reproduce and happened in a CI environment where live gdb is not available) but we assume that the queue was being updated just as the panic occurred so the current tail's next pointer had been updated to point to the new element but stqh_last had not. We then tripped QCMD_STAILQ_CHECK_TAIL while dumping core.

This, of course, assumes that neither the compiler nor the CPU reordered the writes...

[olce]

In D55819#1277553, @des wrote:

This, of course, assumes that neither the compiler nor the CPU reordered the writes...

The compiler could in theory, but I doubt it would in practice, as it sees that (head)->stqh_last is changed before prevlast is used. But maybe they have become smart enough. In any case, adding a memory barrier for such an infrequent case may pessimize existing code. An acceptable way out could be to test for panic and just bail out right in QMD_ASSERT() (introducing a new QMD_PANICKED()).

As for CPUs reordering the writes, given the synchronization that happens at panic, the CPU that continues to run after the panic cannot observe these reorderings and will just observe all writes as performed.