Re: [PATCH 1/8] Introduce simple hazard pointers

On Wed, Jun 25, 2025 at 11:52:04AM -0400, Waiman Long wrote:
[...]
> > +/*
> > + * Acquire a hazptr slot and begin the hazard pointer critical section.
> > + *
> > + * Must be called with preemption disabled, and preemption must remain disabled
> > + * until shazptr_clear().
> > + */
> > +static inline struct shazptr_guard shazptr_acquire(void *ptr)
> > +{
> > +	struct shazptr_guard guard = {
> > +		/* Preemption is disabled. */
> > +		.slot = this_cpu_ptr(&shazptr_slots),
> > +		.use_wildcard = false,
> > +	};
> > +
> > +	if (likely(!READ_ONCE(*guard.slot))) {
> > +		WRITE_ONCE(*guard.slot, ptr);
> > +	} else {
> > +		guard.use_wildcard = true;
> > +		WRITE_ONCE(*guard.slot, SHAZPTR_WILDCARD);
> > +	}
> Is it correct to assume that shazptr cannot be used in a mixed context
> environment on the same CPU like a task context and an interrupt context
> trying to acquire it simultaneously because the current check isn't atomic
> with respect to that?

I think the current implementation actually support mixed context usage,
let see (assuming we start in a task context):

	if (likely(!READ_ONCE(*guard.slot))) {

if an interrupt happens here, it's fine because the slot is still empty,
as long as the interrupt will eventually clear the slot.

		WRITE_ONCE(*guard.slot, ptr);

if an interrupt happens here, it's fine because the interrupt would
notice that the slot is already occupied, hence the interrupt will use a
wildcard, and because it uses a wild, it won't clear the slot after it
returns. However the task context's shazptr_clear() will eventually
clear the slot because its guard's .use_wildcard is false.

	} else {

if an interrupt happens here, it's fine because of the same: interrupt
will use wildcard, and it will not clear the slot, and some
shazptr_clear() in the task context will eventually clear it.

		guard.use_wildcard = true;
		WRITE_ONCE(*guard.slot, SHAZPTR_WILDCARD);

if an interrupt happens here, it's fine because of the same.

	}


It's similar to why rcu_read_lock() can be just a non-atomic inc.

> > +
> > +	smp_mb(); /* Synchronize with smp_mb() at synchronize_shazptr(). */
> > +
> > +	return guard;
> > +}
> > +
> > +static inline void shazptr_clear(struct shazptr_guard guard)
> > +{
> > +	/* Only clear the slot when the outermost guard is released */
> > +	if (likely(!guard.use_wildcard))
> > +		smp_store_release(guard.slot, NULL); /* Pair with ACQUIRE at synchronize_shazptr() */
> > +}
> 
> Is it better to name it shazptr_release() to be conformant with our current
> locking convention?
> 

Maybe, but I will need to think about slot reusing between
shazptr_acquire() and shazptr_release(), in the general hazptr API,
you can hazptr_alloc() a slot, use it and hazptr_clear() and then
use it again, eventually hazptr_free(). I would like to keep both hazptr
APIs consistent as well. Thanks!

Regards,
Boqun

> Cheers,
> Longman
>