Re: [PATCH net-next] ipv6: sit: Add ipip6_tunnel_dst_find() for cleanup

[Yue Haibing <yuehaibing@huawei.com>]

On 2025/8/27 22:45, Alexander Lobakin wrote:
> From: Yue Haibing <yuehaibing@huawei.com>
> Date: Wed, 27 Aug 2025 12:00:27 +0800
> 
>> Extract the dst lookup logic from ipip6_tunnel_xmit() into new helper
>> ipip6_tunnel_dst_find() to reduce code duplication and enhance readability.
>>
>> No functional change intended.
> 
> ...but bloat-o-meter stats would be nice to see here. I'm curious
> whether the object code got any changes or the compilers still just
> inline this function to both call sites.

On a x86_64, with allmodconfig:
before:
   text    data     bss     dec     hex filename
  96973   14687     256  111916   1b52c net/ipv6/sit.o

after:
   text    data     bss     dec     hex filename
  96699   14599     256  111554   1b3c2 net/ipv6/sit.o

I will add this in v2.

> 
>>
>> Signed-off-by: Yue Haibing <yuehaibing@huawei.com>
>> ---
>>  net/ipv6/sit.c | 93 +++++++++++++++++++++++---------------------------
>>  1 file changed, 43 insertions(+), 50 deletions(-)
>>
>> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
>> index 12496ba1b7d4..bcd261ff985b 100644
>> --- a/net/ipv6/sit.c
>> +++ b/net/ipv6/sit.c
>> @@ -848,6 +848,47 @@ static inline __be32 try_6rd(struct ip_tunnel *tunnel,
>>  	return dst;
>>  }
>>  
>> +static bool ipip6_tunnel_dst_find(struct sk_buff *skb, __be32 *dst,
>> +				  bool is_isatap)
>> +{
>> +	const struct ipv6hdr *iph6 = ipv6_hdr(skb);
>> +	struct neighbour *neigh = NULL;
>> +	const struct in6_addr *addr6;
>> +	bool found = false;
>> +	int addr_type;
>> +
>> +	if (skb_dst(skb))
>> +		neigh = dst_neigh_lookup(skb_dst(skb), &iph6->daddr);
>> +
>> +	if (!neigh) {
>> +		net_dbg_ratelimited("nexthop == NULL\n");
>> +		return found;
> 
> Return false here right away.
> 
>> +	}
>> +
>> +	addr6 = (const struct in6_addr *)&neigh->primary_key;
>> +	addr_type = ipv6_addr_type(addr6);
>> +
>> +	if (is_isatap) {
>> +		if ((addr_type & IPV6_ADDR_UNICAST) &&
>> +		    ipv6_addr_is_isatap(addr6)) {
>> +			*dst = addr6->s6_addr32[3];
>> +			found = true;
>> +		}
>> +	} else {
>> +		if (addr_type == IPV6_ADDR_ANY) {
>> +			addr6 = &ipv6_hdr(skb)->daddr;
>> +			addr_type = ipv6_addr_type(addr6);
>> +		}
>> +
>> +		if ((addr_type & IPV6_ADDR_COMPATv4) != 0) {
>> +			*dst = addr6->s6_addr32[3];
>> +			found = true;
>> +		}
>> +	}
>> +	neigh_release(neigh);
>> +	return found;
> 
> I'd put 2 additional newlines here:
> 
> 	}
> 
> 	neigh_release(neigh);
> 
> 	return found;
> }
> 
> for readability purposes and also a NL before the final `return` is
> usually mandatory.

OK， thanks.
> 
> Thanks,
> Olek
>