Eric Dumazet wrote: > Evgeniy Polyakov a écrit : >> Hi. >> >> On Tue, Apr 21, 2009 at 02:52:30PM +0800, Lai Jiangshan (laijs@xxxxxxxxxxxxxx) wrote: >>>> +void xt_info_rdlock_bh(void) >>>>> +{ >>>>> + struct xt_info_lock *lock; >>>>> + >>>>> + preempt_disable(); >>>>> + lock = &__get_cpu_var(xt_info_locks); >>>>> + if (likely(++lock->depth == 0)) >>> So what happen when xt_info_rdlock_bh() called recursively here? >>> >>>>> + spin_lock_bh(&lock->lock); >>>>> + preempt_enable_no_resched(); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(xt_info_rdlock_bh); >>>>> + >>> ---------- >>> Is this OK? (Now I suppose we can enter the read-side critical region >>> in irq context) >>> >>> void xt_info_rdlock_bh(void) >>> { >>> unsigned long flags; >>> struct xt_info_lock *lock; >>> >>> local_irq_save(flags); >>> lock = &__get_cpu_var(xt_info_locks); >>> if (likely(++lock->depth == 0)) >>> spin_lock_bh(&lock->lock); >>> local_irq_restore(flags); >>> } >> Netfilter as long as other generic network pathes are never accessed >> from interrupt context, but your analysis looks right for the softirq >> case. >> >> Stephen, should preempt_disable() be replaced with local_bh_disable() to >> prevent softirq to race on the same cpu for the lock's depth field? Or >> can it be made atomic? >> > > > Maybe just dont care about calling several time local_bh_disable() > (since we were doing this in previous kernels anyway, we used to call read_lock_bh()) > > This shortens fastpath, is faster than local_irq_save()/local_irq_restore(), > and looks better. > > void xt_info_rdlock_bh(void) > { > struct xt_info_lock *lock; > > local_bh_disable(); > lock = &__get_cpu_var(xt_info_locks); > if (likely(++lock->depth == 0)) > spin_lock(&lock->lock); > } This two functions is OK. But... > > void xt_info_rdunlock_bh(void) > { > struct xt_info_lock *lock = &__get_cpu_var(xt_info_locks); > > BUG_ON(lock->depth < 0); > if (likely(--lock->depth < 0)) > spin_unlock(&lock->lock); > local_bh_enable(); > } > > David said: Netfilter itself, is nesting. When using bridging netfilter, iptables can be entered twice in the same call chain. And Stephen said: In this version, I was trying to use/preserve the optimizations that are done in spin_unlock_bh(). So: void xt_info_rdlock_bh(void) { struct xt_info_lock *lock; preempt_disable(); lock = &__get_cpu_var(xt_info_locks); if (likely(lock->depth < 0)) spin_lock_bh(&lock->lock); /* softirq is disabled now */ ++lock->depth; preempt_enable_no_resched(); } xt_info_rdunlock_bh() is the same as v11. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html