Message-ID: <5775952b-943a-f8ad-55a1-c4d0fd08475f@intel.com>
Date: Wed, 26 Jul 2023 13:09:22 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Andy Shevchenko <andy@...nel.org>, Yury Norov <yury.norov@...il.com>,
	Marcin Szycik <marcin.szycik@...ux.intel.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<wojciech.drewek@...el.com>, <michal.swiatkowski@...ux.intel.com>,
	<davem@...emloft.net>, <kuba@...nel.org>, <jiri@...nulli.us>,
	<pabeni@...hat.com>, <jesse.brandeburg@...el.com>,
	<simon.horman@...igine.com>, <idosch@...dia.com>
Subject: Re: [PATCH iwl-next v3 2/6] ip_tunnel: convert __be16 tunnel flags to
 bitmaps

From: Andy Shevchenko <andy@...nel.org>, Yury Norov <yury.norov@...il.com>
Date: Fri, 21 Jul 2023 17:42:12 +0300

> +Cc: Yury on bitmap assignments.

I told Marcin to add you to Cc when sending, but forgot Yury, my
apologies =\

> 
> (Yury, JFYI,
>  if you need the whole series, take message ID as $MSG_ID of this email
>  and execute
> 
>    `b4 mbox $MSG_ID`
> 
>  to retrieve it)
> 
> On Fri, Jul 21, 2023 at 09:15:28AM +0200, Marcin Szycik wrote:
>> From: Alexander Lobakin <aleksander.lobakin@...el.com>
>>
>> Historically, tunnel flags like TUNNEL_CSUM or TUNNEL_ERSPAN_OPT
>> have been defined as __be16. Now all of those 16 bits are occupied
>> and there's no more free space for new flags.
>> It can't be simply switched to a bigger container with no
>> adjustments to the values, since it's an explicit Endian storage,
>> and on LE systems (__be16)0x0001 equals to
>> (__be64)0x0001000000000000.
>> We could probably define new 64-bit flags depending on the
>> Endianness, i.e. (__be64)0x0001 on BE and (__be64)0x00010000... on
>> LE, but that would introduce an Endianness dependency and spawn a
>> ton of Sparse warnings. To mitigate them, all of those places which
>> were adjusted with this change would be touched anyway, so why not
>> define stuff properly if there's no choice.
>>
>> Define IP_TUNNEL_*_BIT counterparts as a bit number instead of the
>> value already coded and a fistful of <16 <-> bitmap> converters and
>> helpers. The two flags which have a different bit position are
>> SIT_ISATAP_64 and VTI_ISVTI_64, as they were defined not as
>> __cpu_to_be16(), but as (__force __be16), i.e. had different
>> positions on LE and BE. Now they have a strongly defined place.
>> Change all __be16 fields which were used to store those flags, to
>> IP_TUNNEL_DECLARE_FLAGS() -> DECLARE_BITMAP(__IP_TUNNEL_FLAG_NUM) ->
>> unsigned long[1] for now, and replace all TUNNEL_* occurencies to
>> their 64-bit bitmap counterparts. Use the converters in the places
>> which talk to the userspace, hardware (NFP) or other hosts (GRE
>> header). The rest must explicitly use the new flags only. This must
>> be done at once, otherwise there will be too much conversions
>> throughout the code in the intermediate commits.
>> Finally, disable the old __be16 flags for use in the kernel code
>> (except for the two 'irregular' flags mentioned above), to prevent
>> any accidental (mis)use of them. For the userspace, nothing is
>> changed, only additions were made.
> 
> Any bloat-o-meter statistics, just in case?

Sure, will add. IIRC that was something like +150-200 bytes to vmlinux.

> 
> ...
> 
>> +static inline void ip_tunnel_flags_from_be16(unsigned long *dst, __be16 flags)
>> +{
>> +	bitmap_zero(dst, __IP_TUNNEL_FLAG_NUM);
> 
>> +	*dst = be16_to_cpu(flags);
> 
> Oh, This is not good. What you need is something like bitmap_set_value16() in
> analogue with bitmap_set_value8().

But I don't need `start`, those flag will always be in the first word
and I don't need to replace only some range, but to clear everything and
then set only the flags which are set in that __be16.
Why shouldn't this work?

> 
>> +	if (flags & VTI_ISVTI)
>> +		__set_bit(IP_TUNNEL_VTI_BIT, dst);
>> +}
>> +
>> +static inline __be16 ip_tunnel_flags_to_be16(const unsigned long *flags)
>> +{
>> +	__be16 ret;
> 
>> +	ret = cpu_to_be16(*flags & U16_MAX);
> 
> Same as above.
> 
> ' & U16_MAX' is redundant.

Ah, right ._.

> 
>> +	if (test_bit(IP_TUNNEL_VTI_BIT, flags))
>> +		ret |= VTI_ISVTI;
>> +
>> +	return ret;
>> +}
> 
> Side note: Make sure you know the difference between
> bitmap_zero(bitmap, nbits) and bitmap_clear(bitmap, 0, nbits).
> Similar for fill and set.

zero() and fill() rewrite the whole longs, I know :D I use them
deliberately here.

> 
> ...
> 
>> +	__set_bit(IP_TUNNEL_KEY_BIT, info->key.tun_flags);
>> +	__set_bit(IP_TUNNEL_CSUM_BIT, info->key.tun_flags);
>> +	__set_bit(IP_TUNNEL_NOCACHE_BIT, info->key.tun_flags);
>>  	if (flags & BPF_F_DONT_FRAGMENT)
>> -		info->key.tun_flags |= TUNNEL_DONT_FRAGMENT;
>> +		__set_bit(IP_TUNNEL_DONT_FRAGMENT_BIT, info->key.tun_flags);
>>  	if (flags & BPF_F_ZERO_CSUM_TX)
>> -		info->key.tun_flags &= ~TUNNEL_CSUM;
>> +		__clear_bit(IP_TUNNEL_CSUM_BIT, info->key.tun_flags);
> 
> Instead of set/clear, use assign, i.e. __asign_bit().

Just to make it clear, you mean

	__assign_bit(IP_TUNNEL_CSUM_BIT, info->key.tun_flags,
		     flags & BPF_F_ZERO_CSUM_TX);

right?

> 
>>  	if (flags & BPF_F_SEQ_NUMBER)
>> -		info->key.tun_flags |= TUNNEL_SEQ;
>> +		__set_bit(IP_TUNNEL_SEQ_BIT, info->key.tun_flags);
>>  	if (flags & BPF_F_NO_TUNNEL_KEY)
>> -		info->key.tun_flags &= ~TUNNEL_KEY;
>> +		__clear_bit(IP_TUNNEL_KEY_BIT, info->key.tun_flags);
> 
> Ditto.
> 
> Check the rest of your code for the similar.

+

> 
> ...
> 
>> +	if (test_bit(IP_TUNNEL_KEY_BIT, p->i_flags)) {
>> +		if (test_bit(IP_TUNNEL_KEY_BIT, flags))
>>  			return key == p->i_key;
> 
>>  		else
> 
> Redundant.
> 
>>  			/* key expected, none present */
>>  			return false;
> 
>> +	} else {
> 
> Ditto.

Seems like it was here before me, but I'm fine to fix it while at it,
why not.

> 
>> +		return !test_bit(IP_TUNNEL_KEY_BIT, flags);
>> +	}
> 

Thanks!
Olek

Powered by blists - more mailing lists