On 25 September 2026 20:52:45 BST, Mathieu Desnoyers <[email protected]> wrote: >On 2026-09-25 15:49, Mathieu Desnoyers wrote: >> Boqun Feng pointed out that a detach happening concurrently with >> hazptr_synchronize can miss a slot. Indeed, scanning the per-CPU >> slots needs to be done *before* scanning the overflow lists. Fix the >> implementation accordingly. >> >> Signed-off-by: Mathieu Desnoyers <[email protected]> > >Self-NACK. I did not migrate the detach code to move it to the new >hazptr_overflow_list_phase. Sorry about the noise. I'll prepare an >updated version. > >Thanks, > >Mathieu
1: Didn't know you could self nak 2: I must've been dumb during review then, in respin I'll think heavier about it. > > >> Reported-by: Boqun Feng <[email protected]> >> Cc: Paul E. McKenney <[email protected]> >> Cc: Boqun Feng <[email protected]> >> Cc: Bradley Morgan <[email protected]> >> Cc: Gary Guo <[email protected]> >> Cc: <[email protected]> >> Cc: <[email protected]> >> --- >> kernel/hazptr.c | 43 ++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 36 insertions(+), 7 deletions(-) >> >> diff --git a/kernel/hazptr.c b/kernel/hazptr.c >> index d3d1050d92cf..ccc19843edbd 100644 >> --- a/kernel/hazptr.c >> +++ b/kernel/hazptr.c >> @@ -24,6 +24,9 @@ static DEFINE_MUTEX(hazptr_wildcard_lock); /* Protect the >> wildcard flip. */ >> void *hazptr_wildcard = (void *) 1UL; >> EXPORT_SYMBOL_GPL(hazptr_wildcard); >> +/* The current overflow list phase. */ >> +static unsigned int hazptr_overflow_list_phase; >> + >> struct hazptr_overflow_list { >> raw_spinlock_t lock; /* Lock protecting overflow list and >> list generation. */ >> struct hlist_head head; /* Overflow list head. */ >> @@ -53,6 +56,12 @@ void *flip_wildcard(void *wildcard) >> return ((unsigned long) wildcard == 1UL) ? (void *) 2UL : (void *) 1UL; >> } >> +static >> +unsigned int flip_list_phase(unsigned int phase) >> +{ >> + return 1 - phase; >> +} >> + >> static >> bool is_wildcard(void *addr) >> { >> @@ -178,15 +187,12 @@ void hazptr_synchronize_cpu_slots(int cpu, void >*addr, void *scan_wildcard) >> } >> static >> -void hazptr_scan_period(void *addr, void *scan_wildcard) >> +void hazptr_scan_cpu_slots_period(void *addr, void *scan_wildcard) >> { >> - unsigned int scan_idx = (unsigned long) scan_wildcard - 1; >> int cpu; >> /* Scan all CPUs slots. */ >> for_each_possible_cpu(cpu) { >> - struct hazptr_overflow_list_flip *overflow_list_flip = >> per_cpu_ptr(&percpu_overflow_list_flip, cpu); >> - >> /* >> * Scan CPU slots. >> * Forward progress against recurring wildcards is guaranteed >> @@ -199,6 +205,17 @@ void hazptr_scan_period(void *addr, void >*scan_wildcard) >> * to acquire that same hazard pointer value. >> */ >> hazptr_synchronize_cpu_slots(cpu, addr, scan_wildcard); >> + } >> +} >> + >> +static >> +void hazptr_scan_overflow_list_period(void *addr, unsigned int >scan_idx) >> +{ >> + int cpu; >> + >> + /* Scan all CPUs overflow lists. */ >> + for_each_possible_cpu(cpu) { >> + struct hazptr_overflow_list_flip *overflow_list_flip = >> per_cpu_ptr(&percpu_overflow_list_flip, cpu); >> /* >> * Scan backup slots in percpu overflow lists. >> @@ -218,6 +235,7 @@ void hazptr_scan_period(void *addr, void >*scan_wildcard) >> */ >> void hazptr_synchronize(void *addr) >> { >> + unsigned int scan_list_phase; >> void *scan_wildcard; >> /* >> @@ -236,10 +254,21 @@ void hazptr_synchronize(void *addr) >> smp_mb(); >> guard(mutex)(&hazptr_wildcard_lock); >> + >> + /* Scan per-CPU slots. */ >> scan_wildcard = flip_wildcard(hazptr_wildcard); >> - hazptr_scan_period(addr, scan_wildcard); >> - WRITE_ONCE(hazptr_wildcard, scan_wildcard); /* Flip the current >> wildcard. */ >> - hazptr_scan_period(addr, flip_wildcard(scan_wildcard)); >> + hazptr_scan_cpu_slots_period(addr, scan_wildcard); >> + WRITE_ONCE(hazptr_wildcard, scan_wildcard); /* Flip >> the current wildcard. */ >> + hazptr_scan_cpu_slots_period(addr, flip_wildcard(scan_wildcard)); >> + >> + /* >> + * Scan overflow lists *after* scanning per-CPU slots. See >> + * hazptr_promote_to_backup_slot() for scan ordering requirement. >> + */ >> + scan_list_phase = flip_list_phase(hazptr_overflow_list_phase); >> + hazptr_scan_overflow_list_period(addr, scan_list_phase); >> + WRITE_ONCE(hazptr_overflow_list_phase, scan_list_phase); /* Flip >> the current list phase. */ >> + hazptr_scan_overflow_list_period(addr, >> flip_list_phase(scan_list_phase)); >> } >> EXPORT_SYMBOL_GPL(hazptr_synchronize); >> > > > --- Thanks! https://lore.kernel.org/all/[email protected]/
