Re: [Intel-wired-lan] [PATCH iwl-next v3 2/6] ip_tunnel: convert __be16 tunnel flags to bitmaps

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Wed, 26 Jul 2023 15:01:44 +0300

> On Wed, Jul 26, 2023 at 2:11 PM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:
>> From: Andy Shevchenko <andy@kernel.org>, Yury Norov <yury.norov@gmail.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@intel.com>
> 
> ...
> 
>>>> and replace all TUNNEL_* occurencies to
> 
> occurrences
> 
> ...
> 
>>>> otherwise there will be too much conversions
> 
> too many
> (countable)

Ooof :z

> 
> ...
> 
>>>> +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?
> 
> I'm not saying it should or shouldn't (actually you need to prove that
> with some test cases added). What I'm saying is that this code is a

Good idea BTW!

> hack because of a layering violation. We do not dereference bitmaps
> with direct access. Even in your code you have bitmap_zero() followed
> by this hack. Why do you call that bitmap_zero() in the first place if
> you are so sure everything will be okay? So either you stick with

Because the bitmap can be longer than one long, but with that direct
deference I only rewrite the first one.

But I admit it's a hack (wasn't hiding that). Just thought this one is
"semi-internal" and it would be okayish to have it... I was wrong :D
What I'm thinking of now is:

	bitmap_zero() // make sure the whole bitmap is cleared
	bitmap_set_value16() // with `start` == 0

With adding bitmap_set_value16() in a separate commit obviously.
That combo shouldn't be too hard for the compiler to optimize into
a couple writes I hope.

> bitops / bitmap APIs or drop all of them and use POD types and bit
> wise ops.
> 
> ...
> 
>>>> +    ret = cpu_to_be16(*flags & U16_MAX);
> 
> Same as above.
> 
> ...
> 
>>>> +    __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?
> 
> Yes.
> 
> 

Thanks,
Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan