Re: [PATCH 2/2] hazptr: Introduce CONFIG_HAZPTR_DEBUG misuse detection

[Boqun Feng <boqun@kernel.org>]

On Thu, Jul 09, 2026 at 07:22:18PM -0400, Mathieu Desnoyers wrote:
> On 2026-07-09 19:05, Paul E. McKenney wrote:
> > On Thu, Jul 09, 2026 at 05:47:00PM -0400, Mathieu Desnoyers wrote:
> [...]
> > > > + */
> > > >    static inline
> > > >    void hazptr_detach_from_task(struct hazptr_ctx *ctx)
> > > >    {
> > > > @@ -160,17 +178,29 @@ void hazptr_note_context_switch(void)
> > > >    	}
> > > >    }
> > > > -/*
> > > > - * hazptr_acquire: Load pointer at address and protect with hazard pointer.
> > > > +/**
> > > > + * hazptr_acquire - Load pointer at address and protect with hazard pointer.
> > > > + *
> > > > + * @ctx: The hazard-pointer context to be passed to hazptr_release().
> > > > + * @addr_p: Pointer to the pointer that is to be hazard-pointer protected.
> > > >     *
> > > >     * Load @addr_p, and protect the loaded pointer with hazard pointer.
> > > > - * When using hazptr_acquire from interrupt handlers, the acquired slots
> > > > - * need to be released before returning from the interrupt handler.
> > > 
> > > I see that you removed wording of a major constraint here which allowed
> > > use of hazptr locally in a interrupt handler: the need to pair the
> > > acquire/release within the handler.
> > 
> > I did indeed remove that wording.  You could do something like this:
> > 
> > 	Task Context			IRQ Handler Interrupts Task
> > 	------------			---------------------------
> > 	preempt_disable();
> > 	ihp = __this_cpu_read(irq_hc);	ihp = __this_cpu_read(irq_hc);
> > 					p = hazptr_acquire(ihp, &gp);
> > 	lp = xchg(p, NULL);
> > 	if (lp) {
> > 		do_something(lp);
> > 		hazptr_release(ihp, lp);
> > 	}
> > 	preempt_enable();
> > 
> > If I understand the rules correctly (ha!), this is perfectly legal
> > and does not require a hazptr_detach_from_task().
> > 
> I am concerned about it, because I knowingly just used preempt disable
> to protect hazptr_acquire from the scheduler, but not from interrupt
> handlers, because it's faster than irqoff.
> 
> I am concerned that this use of hazptr_acquire could confuse the
> thread-level hazptr_acquire, let's dig:
> 
> void *hazptr_acquire(struct hazptr_ctx *ctx, void * const *addr_p)
> {
>         struct hazptr_percpu_slots *percpu_slots;
>         struct hazptr_slot_item *slot_item;
>         struct hazptr_slot *slot;
>         void *addr;
> 
>         guard(preempt)();
>         percpu_slots = this_cpu_ptr(&hazptr_percpu_slots);
>         slot_item = &percpu_slots->items[0];
>         slot = &slot_item->slot;
> [...]
> 
>   from here --------------------
>         if (unlikely(slot->addr))
>                 return __hazptr_acquire(ctx, addr_p);
>         WRITE_ONCE(slot->addr, HAZPTR_WILDCARD);        /* Store B */
> 
>         /* Memory ordering: Store B before Load A. */
>         smp_mb();
> 
>         /*
>          * Load @addr_p after storing wildcard to the hazard pointer slot.
>          */
>         addr = READ_ONCE(*addr_p);      /* Load A */
> 
>         /*
>          * We don't care about ordering of Store C. It will simply
>          * replace the wildcard by a more specific address. If addr is
>          * NULL, we simply store NULL into the slot.
>          */
>         WRITE_ONCE(slot->addr, addr);   /* Store C */
>    to here ----------------------------
> 
> I designed hazptr_acquire so a _nested_ interrupt which _brings back_
> the slot addr to its original state will work. Now let's see if
> that's still OK if the interrupt handler leaves the slot->addr
> populated with an address.
> 
> And no. If the interrupt happens right before "Store B", its
> reserved slot address is overwritten by the thread. That's incorrect.
> 
> So as it is today, the irq handler needs to vacate the slot before it
> returns, either through a hazptr release or a detach.
> 
> That being said, this is the code as it is today. If someone finds a
> clever way to support this use-case and keep it fast and not too complex,
> I'm all ears! :)
> 

I don't find this use-case is very useful, but I guess a different set
of percpu slot for interrupts can resolve this issue?

Regards,
Boqun

> One possible way to make it work would be to install the HAZPTR_WILDCARD
> with a local-cmpxchg expecting a NULL slot->addr. This would close this
> race window. But it comes at a non-null overhead price. Another alternative
> on x86 would be to use a lock prefixed cmpxchg, which has an implied smp_mb
> on success, which may be in the same ballpark as the sequence of WRITE_ONCE+
> explicit smp_mb().
> 
> Thanks,
> 
> Mathieu
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> https://www.efficios.com