Re: [PATCHv2 12/17] nvme: add Clang context annotations for nvme_queue::cq_poll_lock

[Nilay Shroff]

On 6/26/26 12:18 PM, Christoph Hellwig wrote:
> On Sun, Jun 14, 2026 at 06:45:27PM +0530, Nilay Shroff wrote:
> > nvme_queue::cqes, nvme_queue::cq_head, and nvme_queue::cq_phase are
> > protected by nvme_queue::cq_poll_lock. Annotate these fields with
> > __guarded_by(&cq_poll_lock) and annotate helpers accessing them with
> > __must_hold(&cq_poll_lock) so that Clang's context analysis can
> > validate the locking requirements.
> >
> > IRQ-based queues do not use cq_poll_lock and instead rely on interrupt
> > serialization. Annotate nvme_irq() and nvme_irq_check() with
> > __context_unsafe() to suppress the corresponding context analysis
> > warnings.
> >
> > nvme_poll() invokes nvme_cqe_pending() as a lockless fast-path check
> > before acquiring cq_poll_lock. This check is intentionally kept outside
> > the lock because nvme_poll() may be called repeatedly in a tight polling
> > loop until completions are found. The result is only advisory, as the
> > completion queue is subsequently revalidated under cq_poll_lock by
> > nvme_poll_cq(). Suppress the corresponding context analysis warning by
> > annotating the lockless invocation of nvme_cqe_pending() with
> > context_unsafe().
>
> I'm not sure the annotation really buys us anything in this form,
> as it makes bold claims about a guard and then ignores it for the
> most common case.

That's because nvme_poll() and nvme_irq() both reuse nvme_poll_cq() to
process completions, but they rely on different synchronization mechanisms.

For the polling path, accesses to the completion queue are synchronized by
->cq_poll_lock, so it is still useful to annotate those accesses with
__must_hold(&cq_poll_lock) and let Clang verify that the polling path
acquires the lock correctly.

The IRQ path, on the other hand, relies on interrupt serialization rather
than ->cq_poll_lock, so those accesses are intentionally annotated with
context_unsafe() to suppress the corresponding warnings.

So, IMO, while the annotations don't apply to the IRQ path, they still provide
value by validating the locking requirements for the polling path, where
->cq_poll_lock is the intended synchronization primitive.

Thanks,
--Nilay