Re: [PATCH net] batman-adv: reduce softirq drain in tt_purge loops

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 12044 bytes --]

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 <omermetekaya0@gmail.com>
> ---
>  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 <linux/sched.h>

> +			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 <omermetekaya0@gmail.com>

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 <omermetekaya0@gmail.com>
>     Signed-off-by: Sven Eckelmann <sven@narfation.org>

[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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]