On 1/13/23 1:15 AM, Tonghao Zhang wrote:
On Jan 13, 2023, at 9:53 AM, Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
On 1/11/23 1:29 AM, tong@xxxxxxxxxxxxx wrote:
+ /*
+ * The lock may be taken in both NMI and non-NMI contexts.
+ * There is a false lockdep warning (inconsistent lock state),
+ * if lockdep enabled. The potential deadlock happens when the
+ * lock is contended from the same cpu. map_locked rejects
+ * concurrent access to the same bucket from the same CPU.
+ * When the lock is contended from a remote cpu, we would
+ * like the remote cpu to spin and wait, instead of giving
+ * up immediately. As this gives better throughput. So replacing
+ * the current raw_spin_lock_irqsave() with trylock sacrifices
+ * this performance gain. atomic map_locked is necessary.
+ * lockdep_off is invoked temporarily to fix the false warning.
+ */
+ lockdep_off();
raw_spin_lock_irqsave(&b->raw_lock, flags);
- *pflags = flags;
+ lockdep_on();
I am not very sure about the lockdep_off/on. Other than the false warning when using the very same htab map by both NMI and non-NMI context, I think the lockdep will still be useful to catch other potential issues. The commit c50eb518e262 ("bpf: Use separate lockdep class for each hashtab") has already solved this false alarm when NMI happens on one map and non-NMI happens on another map.
Alexei, what do you think? May be only land the patch 1 fix for now.
Hi Martin
Patch 2 is used for patch 1 to test whether there is a deadlock. We should apply this two patches.
It is too noisy for test_progs that developers routinely run. Lets continue to
explore other ways (or a different test) without this false positive splat.
Patch 1 was applied as already mentioned in the earlier reply.
