On Friday, 30 October 2026 00:59:51 CET Ömer Mete Kaya wrote: > Replace per-bucket spin_lock_bh()/spin_unlock_bh() with outer > local_bh_disable()/enable() and periodic drain every 64 buckets > to avoid excessive softirq processing during TT purge. I would prefer if there is paragraph explaining what the actual problem is instead of "avoid excessive softirq processing during TT purge." - I would not know what is going on or if it is a critical problem just from these couple of words. > Add may_sleep parameter to batadv_tt_local_purge() since it is > called both from workqueue context (where cond_resched is safe) > and under commit_lock spinlock in batadv_tt_local_resize_to_mtu() > (where sleeping is not allowed). But what was the intention to have the potential sleeps in the first place. It is most likely good to have them but it is not explained here at all. And if there is a good reason to have it - would there be any bigger problems when it runs under batadv_tt_local_purge() without it? > Fixes: c6c8fea29769 ("net: Add batman-adv meshing protocol") > Reported-by: syzbot+61b202fdc410840f0198@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=61b202fdc410840f0198 > Signed-off-by: Ömer Mete Kaya > --- > net/batman-adv/translation-table.c | 30 +++++++++++++++++++++++------- > 1 file changed, 23 insertions(+), 7 deletions(-) This is not going through net (or net-next). So please don't mark it like it. The target would be the batadv tree. Is there a good reason to have this sent through a non-next tree and get it backported to various stable trees? For me, it looks just like the system has a loop ("bridge0: received packet on bridge_slave_1 with own address as source address") and therefore the system is under heavy load. Due to this problem, some other tasks will take longer - like the workqueue task from batadv. And you are now trying to remove the kernel splat "BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=-20 stuck for 32s!" For the moment, I don't understand why this has to be handled like some critical fix in batman-adv. It is just a bystander which suffers from the current system state. If you think that you can optimize the situation for batman-adv - fine. But this is then something for batadv-next and not a fix which needs to enter net + needs to be backported. But maybe I am misunderstanding the problem here - please describe it better, so I can also understand what the problem+idea is behind your change. And I am not talking about just putting in a lot of text like these LLMs are generating nowadays. I am talking about a simple to follow explanation which result in me thinking "hey, this is good to have in following tree/branch". Btw. it doesn't apply in the batadv tree (shown here): $ ./scripts/get_maintainer.pl --scm ./20261030000222.286935-1-omermetekaya0@gmail.com.mbx [...] git https://git.open-mesh.org/batadv.git [...] There is a conflict in batadv_tt_local_resize_to_mtu() due to the commit 48d05a984683 ("batman-adv: tt: limit local table to single TVLV response size"). It is only a small context conflict: @@ net/batman-adv/translation-table.c: static void batadv_tt_purge(struct work_stru batadv_tt_req_purge(bat_priv); batadv_tt_roam_purge(bat_priv); @@ net/batman-adv/translation-table.c: void batadv_tt_local_resize_to_mtu(struct net_device *mesh_iface) - if (packet_size_max >= table_size) + if (size_max >= table_size) break; - batadv_tt_local_purge(bat_priv, timeout); > diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c > index 68f79853032e..915f5589e056 100644 > --- a/net/batman-adv/translation-table.c > +++ b/net/batman-adv/translation-table.c > @@ -1386,21 +1386,29 @@ static void batadv_tt_local_purge_list(struct batadv_priv *bat_priv, > * inactive or not > */ > static void batadv_tt_local_purge(struct batadv_priv *bat_priv, > - int timeout) > + int timeout, bool may_sleep) New parameter is not documented. > { > spinlock_t *list_lock; /* protects write access to the hash lists */ > struct batadv_hashtable *hash = bat_priv->tt.local_hash; > struct hlist_head *head; > u32 i; > > + local_bh_disable(); > for (i = 0; i < hash->size; i++) { > head = &hash->table[i]; > list_lock = &hash->list_locks[i]; > > - spin_lock_bh(list_lock); > + spin_lock(list_lock); > batadv_tt_local_purge_list(bat_priv, head, timeout); > - spin_unlock_bh(list_lock); > + spin_unlock(list_lock); > + > + if (may_sleep && (i & 63) == 63) { > + local_bh_enable(); > + cond_resched(); You are missing the include for this function: #include > + local_bh_disable(); > + } > } > + local_bh_enable(); > } This looks like a workaround for a problem which isn't described very well. Are there any precedents in the Linux kernel? I know of things like ctnetlink_dump_table() with the large local_bh_disable() and inner spinlocks. But I know of nothing with the cond_resched. But since I don't know why it was added in the first place, I would a better explanation of the problem and the reasoning why it was tried to be fixed like you did. For me, it looks like you intended to cut the worker->softirq switches from 1024 to 16. But it is unclear why this would help here because there seems to be quite some heavy (pending) load on the system which will then still run when local_bh_enable() is called (on this CONFIG_PREEMPT kernel). > static void batadv_tt_local_purge(struct batadv_priv *bat_priv, > - int timeout) > + int timeout, bool may_sleep) [...] > @@ -2363,11 +2371,12 @@ static void batadv_tt_global_purge(struct batadv_priv *bat_priv) I see changes in batadv_tt_local_purge + batadv_tt_global_purge but where is the change for batadv_tt_local_purge_pending_clients, batadv_tt_global_del_orig, batadv_tt_local_transition_new? I think batadv_tt_global_table_free and batadv_tt_local_table_free are not running in the right context, so let us ignore them for now. And to finish the list, here is the Sashiko output. Please deal with them directly - I hate to write rebuttals for these things when sending it to the next maintainer(s). Findings: Critical: 0 High: 0 Medium: 1 Low: 2 --- Patch [1] batman-adv: reduce softirq drain in tt_purge loops --- [Low] batman-adv: batadv_tt_local_purge() kernel-doc missing new @may_sleep param [Low] batman-adv: tt purge loops hold BH disabled across 64 buckets instead of 1 [Medium] batman-adv: commit message claims softirq reduction; patch only defers it Inline Review: commit ae1b9a789b6f70e3fdb116868b75058b32b7b764 Author: Ömer Mete Kaya batman-adv: reduce softirq drain in tt_purge loops This commit replaces the per-bucket spin_lock_bh()/spin_unlock_bh() calls in batadv_tt_local_purge() and batadv_tt_global_purge() with an outer local_bh_disable() section. That section is briefly re-enabled, with a cond_resched(), every 64 buckets. A may_sleep parameter is added so that batadv_tt_local_resize_to_mtu(), which holds commit_lock, skips the rescheduling point. Closes: https://syzkaller.appspot.com/bug?extid=61b202fdc410840f0198 > batman-adv: reduce softirq drain in tt_purge loops > > Replace per-bucket spin_lock_bh()/spin_unlock_bh() with outer > local_bh_disable()/enable() and periodic drain every 64 buckets > to avoid excessive softirq processing during TT purge. > > Add may_sleep parameter to batadv_tt_local_purge() since it is > called both from workqueue context (where cond_resched is safe) > and under commit_lock spinlock in batadv_tt_local_resize_to_mtu() > (where sleeping is not allowed). > > Fixes: c6c8fea29769 ("net: Add batman-adv meshing protocol") > Reported-by: syzbot+61b202fdc410840f0198@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=61b202fdc410840f0198 > Signed-off-by: Ömer Mete Kaya > Signed-off-by: Sven Eckelmann [Severity: Medium] Does this patch reduce the softirq work, or only defer it? Every 64 buckets the loop does: local_bh_enable(); cond_resched(); local_bh_disable(); local_bh_enable() still runs all pending softirqs, so the total softirq work looks coalesced rather than reduced. The new behaviour seems to be the cond_resched() in workqueue context. The message doesn't present that as the fix, and it doesn't explain how it addresses the syzbot report. The resize path is also unchanged in practice. batadv_tt_local_resize_to_mtu() holds spin_lock_bh(&bat_priv->tt.commit_lock) and passes may_sleep=false: batadv_tt_local_purge(bat_priv, timeout, false); batadv_tt_local_purge_pending_clients(bat_priv); batadv_tt_local_purge_pending_clients() is called right after, in the same loop. It also walks every bucket of the local hash, and it still uses per-bucket spin_lock_bh()/spin_unlock_bh(). Should it be converted too, so that the tt_purge loops are handled consistently? The Fixes: tag points at c6c8fea29769, the initial batman-adv import. Is there a more specific commit that introduced the problem syzbot reported? > diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c > index 46304069b3b75..f64f31e25c6b0 100644 > --- a/net/batman-adv/translation-table.c > +++ b/net/batman-adv/translation-table.c > @@ -1832,21 +1832,29 @@ static void batadv_tt_local_purge_list(struct batadv_priv *bat_priv, > * inactive or not > */ > static void batadv_tt_local_purge(struct batadv_priv *bat_priv, > - int timeout) > + int timeout, bool may_sleep) [Severity: Low] The kernel-doc block above batadv_tt_local_purge() still documents only @bat_priv and @timeout. Should an @may_sleep line be added? scripts/kernel-doc will warn that may_sleep is not described. The new parameter also has a contract worth documenting: it must be false when the caller holds a spinlock such as tt.commit_lock. > { > spinlock_t *list_lock; /* protects write access to the hash lists */ > struct batadv_hashtable *hash = bat_priv->tt.local_hash; > struct hlist_head *head; > u32 i; > > + local_bh_disable(); > for (i = 0; i < hash->size; i++) { > head = &hash->table[i]; > list_lock = &hash->list_locks[i]; > > - spin_lock_bh(list_lock); > + spin_lock(list_lock); > batadv_tt_local_purge_list(bat_priv, head, timeout); > - spin_unlock_bh(list_lock); > + spin_unlock(list_lock); > + > + if (may_sleep && (i & 63) == 63) { > + local_bh_enable(); > + cond_resched(); > + local_bh_disable(); > + } [Severity: Low] With the outer local_bh_disable(), bottom halves now stay disabled for up to 64 buckets of the hash instead of one. Before this patch, each spin_unlock_bh() let pending softirqs such as NET_RX_SOFTIRQ, NET_TX_SOFTIRQ and timers run between buckets. On preemptible kernels it also allowed the worker to be preempted there. Could this increase softirq latency on the CPU running batadv_tt_purge()? On PREEMPT_RT, local_bh_disable() also holds the per-CPU softirq lock for the whole 64-bucket window. batadv_tt_global_purge() gets the same wider window, and there the re-enable runs unconditionally. Would it work to keep the per-bucket spin_lock_bh() and only add the cond_resched() every 64 buckets? That would give the same scheduling point without the longer windows or the new may_sleep parameter. Regards, Sven