On Wed, Nov 6, 2024 at 1:49 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: > > Hi, > > On 11/6/2024 4:45 PM, Sebastian Andrzej Siewior wrote: > > On 2024-11-06 14:35:39 [+0800], Hou Tao wrote: > >> From: Hou Tao <houtao1@xxxxxxxxxx> > >> > >> Hi, > > Hi Hou, > > > >> The patch set fixes a lockdep warning for htab of map. The > >> warning is found when running test_maps. The warning occurs when > >> htab_put_fd_value() attempts to acquire map_idr_lock to free the map id > >> of the inner map while already holding the bucket lock (raw_spinlock_t). > >> > >> The fix moves the invocation of free_htab_elem() after > >> htab_unlock_bucket() and adds a test case to verify the solution. Please > >> see the individual patches for details. Comments are always welcome. The fix makes sense. I manually resolved merge conflict and applied. > > Thank you. > > > > Acked-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> > > > > I've seen that you didn't move check_and_free_fields() out of the bucket > > locked section. Type BPF_TIMER does hrtimer_cancel() if the timer > > happens to run on a remote CPU. On PREEMPT_RT this will acquire a > > sleeping lock which is problematic due to the raw_spinlock_t. > > Would it be okay, to cleanup the timer unconditionally via the > > workqueue? > > Yes. The patch set still invokes check_and_free_fields() under the > bucket lock when updating an existing element in a pre-allocated htab. I > missed the hrtimer case. For the sleeping lock, you mean the > cpu_base->softirq_expiry_lock in hrtimer_cancel_waiting_running(), right > ? Instead of cancelling the timer in workqueue, maybe we could save the > old value temporarily in the bucket lock, and try to free it outside of > the bucket lock or disabling the extra_elems logic temporarily for the > case ? We definitely need to avoid spamming wq when cancelling timers. wq may not be able to handle the volume. Moving it outside of bucket lock is certainly better.
