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

[Marcin Szycik <marcin.szycik@linux.intel.com>]

On 05.06.2023 11:23, Simon Horman wrote:
> On Thu, Jun 01, 2023 at 03:19:25PM +0200, Marcin Szycik wrote:
>> From: Alexander Lobakin <alexandr.lobakin@intel.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.
> 
> Hi Marcin,
> 
> a few nits from my side, that you can take or leave.
> Overall this looks good to me.
> 
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>

Hi Simon,

Thank you for review, I will fix the minor issues in v2.

> 
>> 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 accidential (mis)use of them. For the userspace, nothing is
> 
> nit: s/accidential/accidental/
> 
>> changed, only additions were made.
>>
>> Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
>> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> 
>> ---
>>  drivers/net/bareudp.c                         |  19 ++-
>>  .../ethernet/mellanox/mlx5/core/en/tc_tun.h   |   2 +-
>>  .../mellanox/mlx5/core/en/tc_tun_encap.c      |   6 +-
>>  .../mellanox/mlx5/core/en/tc_tun_geneve.c     |  12 +-
>>  .../mellanox/mlx5/core/en/tc_tun_gre.c        |   9 +-
>>  .../mellanox/mlx5/core/en/tc_tun_vxlan.c      |   9 +-
>>  .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  15 +-
>>  .../ethernet/mellanox/mlxsw/spectrum_ipip.c   |  32 +++--
>>  .../ethernet/mellanox/mlxsw/spectrum_span.c   |   6 +-
>>  .../ethernet/netronome/nfp/flower/action.c    |  26 +++-
>>  drivers/net/geneve.c                          |  46 +++---
>>  drivers/net/vxlan/vxlan_core.c                |  14 +-
>>  include/net/dst_metadata.h                    |  10 +-
>>  include/net/flow_dissector.h                  |   2 +-
>>  include/net/gre.h                             |  59 ++++----
>>  include/net/ip6_tunnel.h                      |   4 +-
>>  include/net/ip_tunnels.h                      |  90 ++++++++++--
>>  include/net/udp_tunnel.h                      |   4 +-
>>  include/uapi/linux/if_tunnel.h                |  33 +++++
>>  net/bridge/br_vlan_tunnel.c                   |   5 +-
>>  net/core/filter.c                             |  20 +--
>>  net/core/flow_dissector.c                     |  12 +-
>>  net/ipv4/fou_bpf.c                            |   2 +-
>>  net/ipv4/gre_demux.c                          |   2 +-
>>  net/ipv4/ip_gre.c                             | 131 +++++++++++-------
>>  net/ipv4/ip_tunnel.c                          |  51 ++++---
>>  net/ipv4/ip_tunnel_core.c                     |  81 +++++++----
>>  net/ipv4/ip_vti.c                             |  31 +++--
>>  net/ipv4/ipip.c                               |  21 ++-
>>  net/ipv4/udp_tunnel_core.c                    |   5 +-
>>  net/ipv6/ip6_gre.c                            |  87 +++++++-----
>>  net/ipv6/ip6_tunnel.c                         |  14 +-
>>  net/ipv6/sit.c                                |   9 +-
>>  net/netfilter/ipvs/ip_vs_core.c               |   6 +-
>>  net/netfilter/ipvs/ip_vs_xmit.c               |  20 +--
>>  net/netfilter/nft_tunnel.c                    |  45 +++---
>>  net/openvswitch/flow_netlink.c                |  55 ++++----
>>  net/psample/psample.c                         |  26 ++--
>>  net/sched/act_tunnel_key.c                    |  39 +++---
>>  net/sched/cls_flower.c                        |  27 ++--
>>  40 files changed, 695 insertions(+), 392 deletions(-)
> 
> nit: this is a rather long patch

Yes, but most of it comes from the fact that tunnel flags are simply used
in many places, and we need to touch all of them.

Alex, do you see a way to logically split this patch into smaller ones?

> 
> ...
> 
>> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
>> index 102119628ff5..d222e70d8621 100644
>> --- a/include/uapi/linux/if_tunnel.h
>> +++ b/include/uapi/linux/if_tunnel.h
>> @@ -161,6 +161,14 @@ enum {
>>  
>>  #define IFLA_VTI_MAX	(__IFLA_VTI_MAX - 1)
>>  
>> +#ifndef __KERNEL__
>> +/* Historically, tunnel flags have been defined as __be16 and now there are
>> + * no free bits left. It is strongly advised to switch the already existing
>> + * userspace code to u32/BIGINT and the new *_BIT definitions from down below,
>> + * as __be16 can't be simply casted to a wider type on LE systems. All new
> 
> nit: s/casted/cast/
> 
> ...
> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan