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

[Simon Horman <simon.horman@corigine.com>]

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>

> 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

...

> 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