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

[Nilay Shroff]

On 6/30/26 4:26 PM, Marco Elver wrote:
> On Tue, 30 Jun 2026 at 11:26, Nilay Shroff <nilay@xxxxxxxxxxxxx> wrote:
> > On 6/29/26 6:20 PM, Christoph Hellwig wrote:
> > > On Fri, Jun 26, 2026 at 08:52:36PM +0530, Nilay Shroff wrote:
> > > > That's because nvme_poll() and nvme_irq() both reuse nvme_poll_cq() to
> > > > process completions, but they rely on different synchronization mechanisms.
> > >
> > > Well, we could split the function easily, but it still uses the same
> > > data structure and thus annotations.
> > I see your point. The annotations describe only the polling synchronization model,
> > while the same state is also accessed under interrupt serialization for IRQ queues.
> > Since __guarded_by() cannot express alternative synchronization mechanisms today,
> > the annotations end up requiring __context_unsafe() for the IRQ path.
> >
> > It sounds like your preference would be to leave these fields unannotated rather
> > than partially annotate one synchronization model. Is that the direction you'd
> > recommend until the annotation infrastructure can express multiple valid
> > synchronization schemes?
> >
> >
> > Marco,
> > I know it's not currently supported, but are there any plan to extend the current
> > capability model to support multiple valid synchronization mechanisms for a single
> > object? For example, allowing a guarded field to be protected by either a lock or
> > another abstract capability (such as interrupt serialization), instead of requiring
> > a single __guarded_by() relationship." For instance,
> >
> > __guarded_by_any(&cq_poll_lock, IRQ_CONTEXT)
> >
> > Then irq handler would be annotated with __capability(irq_serialization).
>
> I had planned to support IRQ context locks, which would be abstract
> but not be backed by a real object (similar to how RCU is handled).
> Just haven't gotten around to it.
>
> > So now the compiler would know if the code executes in the interrupt/irq context
> > then access to the guarded variable doesn't require any lock otherwise it needs
> > ->cq_poll_lock when accessed. I don't know how easy it'd be to add such support
> > but it may be explored.
>
> Multi-arg __guarded_by() is supported by Clang, but perhaps not
> exactly the way you imagined above. This has a brief summary:
> https://patch.msgid.link/20260515124426.2227783-1-elver@xxxxxxxxxx
>
> In short, you can hold any lock to get reader access, and must hold
> all for writer access.

That's interesting, and I can see how that would be useful. However, I don't think
it quite addresses this particular use case.
Here we're not choosing between multiple locks protecting the same object. Instead,
the synchronization mechanism depends on the execution context: the polling path
protects the completion queue with ->cq_poll_lock, whereas the IRQ path relies on
interrupt serialization and deliberately does not acquire ->cq_poll_lock.

So the requirement is effectively "hold ->cq_poll_lock or execute in the IRQ
serialization context and thus avoid holding any lock". Unless I'm misunderstanding,
I don't think multi-argument __guarded_by() can express that today.

Thanks,
--Nilay