Re: [PATCH net 1/1] batman-adv: stop caching unowned originator pointers in BAT IV

From: 'Simon Horman' <[email protected]>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/

I do have concerns that this review needs to be addressed in
order for this patch to progress.

OTOH, there is another AI-generated review of this patch available at
https://sashiko.dev. In that case, I believe the review relates to
a pre-existing condition, and addressing that (or not) can be treated
as a follow-up.

---
batman-adv: stop caching unowned originator pointers in BAT IV

This patch removes the orig_node back-pointer cached inside each BAT IV
neigh_node and resolves the originator on demand via batadv_orig_hash_find()
from the stored neighbor address. Tagged Fixes: c6c8fea29769 and
Cc: [email protected].

> diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
> index f28e9cbf8ad5..168b413dd18b 100644
> --- a/net/batman-adv/bat_iv_ogm.c
> +++ b/net/batman-adv/bat_iv_ogm.c

[ ... ]

> @@ -1372,8 +1380,8 @@ batadv_iv_ogm_process_per_outif(const struct sk_buff *skb, int ogm_offset,
>  
>  	router = batadv_orig_router_get(orig_node, if_outgoing);
>  	if (router) {
> -		router_router = batadv_orig_router_get(router->orig_node,
> -						       if_outgoing);
> +		router_router = batadv_orig_to_router(bat_priv, router->addr,
> +						      if_outgoing);
>  		router_ifinfo = batadv_neigh_ifinfo_get(router, if_outgoing);
>  	}

Is this replacement semantically equivalent to the original call?

The previous code used batadv_orig_router_get() on the cached orig_node to
fetch router_router. The new code calls batadv_orig_to_router(), which
internally runs batadv_find_router():

    net/batman-adv/originator.c:batadv_orig_to_router() {
        orig_node = batadv_orig_hash_find(bat_priv, orig_addr);
        ...
        neigh_node = batadv_find_router(bat_priv, orig_node, if_outgoing);
        ...
    }

batadv_find_router() has a bonding path that is taken when
recv_if == BATADV_IF_DEFAULT and bonding is enabled:

    net/batman-adv/routing.c:batadv_find_router() {
        ...
        if (!(recv_if == BATADV_IF_DEFAULT &&
              atomic_read(&bat_priv->bonding)))
            return router;

        /* bonding: loop through the list of possible routers ... */
        ...
        if (next_candidate) {
            ...
            batadv_last_bonding_replace(orig_node, next_candidate);
        } else if (first_candidate) {
            ...
            batadv_last_bonding_replace(orig_node, first_candidate);
        } else {
            batadv_last_bonding_replace(orig_node, NULL);
        }
        ...
    }

batadv_iv_ogm_process() invokes batadv_iv_ogm_process_per_outif() once with
BATADV_IF_DEFAULT per received OGM:

    net/batman-adv/bat_iv_ogm.c:batadv_iv_ogm_process() {
        ...
        batadv_iv_ogm_process_per_outif(skb, ogm_offset, orig_node,
                                        if_incoming, BATADV_IF_DEFAULT);
        ...
    }

With bonding enabled, does every received OGM now rotate
orig_node->last_bonding_candidate on the last-hop neighbor's originator via
batadv_last_bonding_replace(), where previously that state was advanced only
from forwarding paths?

And can the bonding candidate selection return a different router_router than
batadv_orig_router_get() would have, which then feeds the subsequent
loop-avoidance check?

    if (router && router_router &&
        (batadv_compare_eth(router->addr, prev_sender)) &&
        !(batadv_compare_eth(ogm_packet->orig, prev_sender)) &&
        (batadv_compare_eth(router->addr, router_router->addr))) {
        ...
        goto out;
    }

The sister helper added by this same patch, batadv_iv_ogm_neigh_ifinfo_sum(),
uses batadv_orig_hash_find() followed by batadv_iv_orig_ifinfo_sum() rather
than batadv_orig_to_router():

> +static u8 batadv_iv_ogm_neigh_ifinfo_sum(struct batadv_priv *bat_priv,
> +					 const struct batadv_neigh_node *neigh_node)
> +{
> +	struct batadv_orig_node *orig_neigh;
> +	u8 sum;
> +
> +	orig_neigh = batadv_orig_hash_find(bat_priv, neigh_node->addr);
> +	if (!orig_neigh)
> +		return 0;
> +
> +	sum = batadv_iv_orig_ifinfo_sum(orig_neigh, neigh_node->if_incoming);
> +	batadv_orig_node_put(orig_neigh);
> +
> +	return sum;
> +}

Would a similar pattern work here, something like:

    orig = batadv_orig_hash_find(bat_priv, router->addr);
    router_router = batadv_orig_router_get(orig, if_outgoing);
    batadv_orig_node_put(orig);

so that the lookup avoids invoking the bonding candidate machinery from the
OGM processing path?

The commit message describes only removing an unowned cached originator
pointer and does not mention a routing or bonding behavior change. Is the
change in router_router resolution intentional, and if so could the commit
message call it out?