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

[Nilay Shroff <nilay@linux.ibm.com>]

On 6/30/26 4:26 PM, Marco Elver wrote:
> On Tue, 30 Jun 2026 at 11:26, Nilay Shroff <nilay@linux.ibm.com> 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@google.com
> 
> 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