Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Mon, 16 Oct 2023 10:54:58 -0700
From:   Yury Norov <yury.norov@gmail.com>
To:     Alexander Lobakin <aleksander.lobakin@intel.com>
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Alexander Potapenko <glider@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        David Ahern <dsahern@kernel.org>,
        Przemek Kitszel <przemyslaw.kitszel@intel.com>,
        Simon Horman <simon.horman@corigine.com>,
        netdev@vger.kernel.org, linux-btrfs@vger.kernel.org,
        dm-devel@redhat.com, ntfs3@lists.linux.dev,
        linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 00/13] ip_tunnel: convert __be16 tunnel flags to
 bitmaps
Message-ID: <ZS148saIsG7WY8ul@yury-ThinkPad>
References: <20231016165247.14212-1-aleksander.lobakin@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231016165247.14212-1-aleksander.lobakin@intel.com>
Precedence: bulk

On Mon, Oct 16, 2023 at 06:52:34PM +0200, Alexander Lobakin wrote:
> Based on top of "Implement MTE tag compression for swapped pages"[0]
> from Alexander Potapenko as it uses its bitmap_{read,write}() functions
> to not introduce another pair of similar ones.
> 
> Derived from the PFCP support series[1] as this grew bigger (2 -> 13
> commits) and involved more core bitmap changes. Only commits 10 and 11
> are from the mentioned tree, the rest is new. PFCP itself still depends
> on this series.
> 
> IP tunnels have their flags defined as `__be16`, including UAPI, and
> after GTP was accepted, there are no more free bits left. UAPI (incl.
> direct usage of one of the user structs) and explicit Endianness only
> complicate things.
> Since it would either way end up with hundreds of locs due to all that,
> pick bitmaps right from the start to store the flags in the most native
> and scalable format with rich API. I don't think it's worth trying to
> praise luck and pick smth like u32 only to redo everything in x years :)
> More details regarding the IP tunnel flags is in 11 and 13.
> 
> The rest is just a good bunch of prereqs and tests: a couple of new
> helpers and extensions to the old ones, a few optimizations to partially
> mitigate IP tunnel object code growth due to __be16 -> long, and
> decouping one UAPI struct used throughout the whole kernel into the
> userspace and the kernel space counterparts to eliminate the dependency.
> 
> [0] https://lore.kernel.org/lkml/20231011172836.2579017-1-glider@google.com
> [1] https://lore.kernel.org/netdev/20230721071532.613888-1-marcin.szycik@linux.intel.com
> 
> Alexander Lobakin (13):
>   bitops: add missing prototype check
>   bitops: make BYTES_TO_BITS() treewide-available
>   bitops: let the compiler optimize {__,}assign_bit()
>   linkmode: convert linkmode_{test,set,clear,mod}_bit() to macros
>   s390/cio: rename bitmap_size() -> idset_bitmap_size()
>   fs/ntfs3: add prefix to bitmap_size() and use BITS_TO_U64()
>   btrfs: rename bitmap_set_bits() -> btrfs_bitmap_set_bits()
>   bitmap: introduce generic optimized bitmap_size()
>   bitmap: make bitmap_{get,set}_value8() use bitmap_{read,write}()
>   ip_tunnel: use a separate struct to store tunnel params in the kernel
>   ip_tunnel: convert __be16 tunnel flags to bitmaps
>   lib/bitmap: add compile-time test for __assign_bit() optimization
>   lib/bitmap: add tests for IP tunnel flags conversion helpers
> 
>  drivers/md/dm-clone-metadata.c                |   5 -
>  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        |   8 +-
>  .../mellanox/mlx5/core/en/tc_tun_vxlan.c      |   9 +-
>  .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  16 +-
>  .../ethernet/mellanox/mlxsw/spectrum_ipip.c   |  56 ++++---
>  .../ethernet/mellanox/mlxsw/spectrum_ipip.h   |   2 +-
>  .../ethernet/mellanox/mlxsw/spectrum_span.c   |  10 +-
>  .../ethernet/netronome/nfp/flower/action.c    |  27 +++-
>  drivers/net/geneve.c                          |  44 +++---
>  drivers/net/vxlan/vxlan_core.c                |  14 +-
>  drivers/s390/cio/idset.c                      |  12 +-
>  fs/btrfs/free-space-cache.c                   |   8 +-
>  fs/ntfs3/bitmap.c                             |   4 +-
>  fs/ntfs3/fsntfs.c                             |   2 +-
>  fs/ntfs3/index.c                              |  11 +-
>  fs/ntfs3/ntfs_fs.h                            |   4 +-
>  fs/ntfs3/super.c                              |   2 +-
>  include/linux/bitmap.h                        |  46 ++----
>  include/linux/bitops.h                        |  23 +--
>  include/linux/cpumask.h                       |   2 +-
>  include/linux/linkmode.h                      |  27 +---
>  include/linux/netdevice.h                     |   7 +-
>  include/net/dst_metadata.h                    |  10 +-
>  include/net/flow_dissector.h                  |   2 +-
>  include/net/gre.h                             |  70 +++++----
>  include/net/ip6_tunnel.h                      |   4 +-
>  include/net/ip_tunnels.h                      | 136 ++++++++++++++---
>  include/net/udp_tunnel.h                      |   4 +-
>  include/uapi/linux/if_tunnel.h                |  33 ++++
>  kernel/trace/trace_probe.c                    |   2 -
>  lib/math/prime_numbers.c                      |   2 -
>  lib/test_bitmap.c                             | 123 ++++++++++++++-
>  net/bridge/br_vlan_tunnel.c                   |   9 +-
>  net/core/filter.c                             |  26 ++--
>  net/core/flow_dissector.c                     |  20 ++-
>  net/ipv4/fou_bpf.c                            |   2 +-
>  net/ipv4/gre_demux.c                          |   2 +-
>  net/ipv4/ip_gre.c                             | 144 +++++++++++-------
>  net/ipv4/ip_tunnel.c                          | 109 ++++++++-----
>  net/ipv4/ip_tunnel_core.c                     |  82 ++++++----
>  net/ipv4/ip_vti.c                             |  41 +++--
>  net/ipv4/ipip.c                               |  33 ++--
>  net/ipv4/ipmr.c                               |   2 +-
>  net/ipv4/udp_tunnel_core.c                    |   5 +-
>  net/ipv6/addrconf.c                           |   3 +-
>  net/ipv6/ip6_gre.c                            |  85 ++++++-----
>  net/ipv6/ip6_tunnel.c                         |  14 +-
>  net/ipv6/sit.c                                |  38 ++---
>  net/netfilter/ipvs/ip_vs_core.c               |   6 +-
>  net/netfilter/ipvs/ip_vs_xmit.c               |  20 +--
>  net/netfilter/nft_tunnel.c                    |  44 +++---
>  net/openvswitch/flow_netlink.c                |  61 +++++---
>  net/psample/psample.c                         |  26 ++--
>  net/sched/act_tunnel_key.c                    |  36 ++---
>  net/sched/cls_flower.c                        |  27 ++--
>  tools/include/linux/bitmap.h                  |   8 +-
>  tools/include/linux/bitops.h                  |   2 +
>  tools/perf/util/probe-finder.c                |   2 -
>  62 files changed, 1011 insertions(+), 600 deletions(-)
> 
> ---
> Not sure whether it's fine to have that all in one series, but OTOH
> there's not much stuff I could split (like, 3 commits), it either
> depends directly (new helpers etc.) or will just generate suboptimal
> code w/o some of the commits.
> 
> I'm also thinking of which tree this would ideally be taken through.
> The main subject is networking, but most of the commits are generic.
> My idea is to push this via Yury / bitmaps and then ask the netdev
> maintainers to pull his tree before they take PFCP (dependent on this
> one).

Let's wait for more comments, but I'm generally OK with the generic
part, and have nothing against moving it, or the whole series, through
bitmap-for-next.

Thanks,
Yury
 
> >From v1[2]:
>  * 03: convert assign_bit() to a macro as well, saves some bytes and
>    looks more consistent (Yury);
>  * 03: enclose each argument into own pair of braces (Yury);
>  * 06: use generic BITS_TO_U64() while at it (Yury);
>  * 07: pick Acked-by (David);
>  * 08: Acked-by, use bitmap_size() in the code from 05 as well (Yury);
>  * 09: instead of introducing a new pair of functions, use generic
>    bitmap_{read,write}() from [0]. bloat-o-meter shows no regressions
>    from the switch (Yury, also Andy).
> 
> Old pfcp -> bitmap changelog:
> 
> As for former commits (now 10 and 11), almost all of the changes were
> suggested by Andy, notably: stop violating bitmap API, use
> __assign_bit() where appropriate, and add more tests to make sure
> everything works as expected. Apart from that, add simple wrappers for
> bitmap_*() used in the IP tunnel code to avoid manually specifying
> ``__IP_TUNNEL_FLAG_NUM`` each time.
> 
> [2] https://lore.kernel.org/lkml/20231009151026.66145-1-aleksander.lobakin@intel.com
> -- 
> 2.41.0