| Message ID | 20231009151026.66145-1-aleksander.lobakin@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ip_tunnel: convert __be16 tunnel flags to bitmaps | expand |
On Mon, Oct 09, 2023 at 05:10:19PM +0200, Alexander Lobakin wrote: > bitmap_set_bits() does not start with the FS' prefix and may collide > with a new generic helper one day. It operates with the FS-specific > types, so there's no change those two could do the same thing. > Just add the prefix to exclude such possible conflict. > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> Acked-by: David Sterba <dsterba@suse.com> We don't have any other code pending that would potentially collide with this change so I don't care when and via which tree this gets merged. I can take it by btrfs too. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Mon, Oct 09, 2023 at 05:10:18PM +0200, Alexander Lobakin wrote: > bitmap_size() is a pretty generic name and one may want to use it for > a generic bitmap API function. At the same time, its logic is > NTFS-specific, as it aligns to the sizeof(u64), not the sizeof(long) > (although it uses ideologically right ALIGN() instead of division). > Add the prefix 'ntfs3_' used for that FS (not just 'ntfs_' to not mix > it with the legacy module). > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> > --- > fs/ntfs3/bitmap.c | 4 ++-- > fs/ntfs3/fsntfs.c | 2 +- > fs/ntfs3/index.c | 11 ++++++----- > fs/ntfs3/ntfs_fs.h | 2 +- > fs/ntfs3/super.c | 2 +- > 5 files changed, 11 insertions(+), 10 deletions(-) > > diff --git a/fs/ntfs3/bitmap.c b/fs/ntfs3/bitmap.c > index 107e808e06ea..a2e18f13e93a 100644 > --- a/fs/ntfs3/bitmap.c > +++ b/fs/ntfs3/bitmap.c > @@ -653,7 +653,7 @@ int wnd_init(struct wnd_bitmap *wnd, struct super_block *sb, size_t nbits) > wnd->total_zeroes = nbits; > wnd->extent_max = MINUS_ONE_T; > wnd->zone_bit = wnd->zone_end = 0; > - wnd->nwnd = bytes_to_block(sb, bitmap_size(nbits)); > + wnd->nwnd = bytes_to_block(sb, ntfs3_bitmap_size(nbits)); > wnd->bits_last = nbits & (wbits - 1); > if (!wnd->bits_last) > wnd->bits_last = wbits; > @@ -1345,7 +1345,7 @@ int wnd_extend(struct wnd_bitmap *wnd, size_t new_bits) > return -EINVAL; > > /* Align to 8 byte boundary. */ > - new_wnd = bytes_to_block(sb, bitmap_size(new_bits)); > + new_wnd = bytes_to_block(sb, ntfs3_bitmap_size(new_bits)); > new_last = new_bits & (wbits - 1); > if (!new_last) > new_last = wbits; > diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c > index 33afee0f5559..7a14d2347f27 100644 > --- a/fs/ntfs3/fsntfs.c > +++ b/fs/ntfs3/fsntfs.c > @@ -522,7 +522,7 @@ static int ntfs_extend_mft(struct ntfs_sb_info *sbi) > ni->mi.dirty = true; > > /* Step 2: Resize $MFT::BITMAP. */ > - new_bitmap_bytes = bitmap_size(new_mft_total); > + new_bitmap_bytes = ntfs3_bitmap_size(new_mft_total); > > err = attr_set_size(ni, ATTR_BITMAP, NULL, 0, &sbi->mft.bitmap.run, > new_bitmap_bytes, &new_bitmap_bytes, true, NULL); > diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c > index 124c6e822623..ab53a4b6ddf8 100644 > --- a/fs/ntfs3/index.c > +++ b/fs/ntfs3/index.c > @@ -1453,8 +1453,8 @@ static int indx_create_allocate(struct ntfs_index *indx, struct ntfs_inode *ni, > > alloc->nres.valid_size = alloc->nres.data_size = cpu_to_le64(data_size); > > - err = ni_insert_resident(ni, bitmap_size(1), ATTR_BITMAP, in->name, > - in->name_len, &bitmap, NULL, NULL); > + err = ni_insert_resident(ni, ntfs3_bitmap_size(1), ATTR_BITMAP, > + in->name, in->name_len, &bitmap, NULL, NULL); > if (err) > goto out2; > > @@ -1515,8 +1515,9 @@ static int indx_add_allocate(struct ntfs_index *indx, struct ntfs_inode *ni, > if (bmp) { > /* Increase bitmap. */ > err = attr_set_size(ni, ATTR_BITMAP, in->name, in->name_len, > - &indx->bitmap_run, bitmap_size(bit + 1), > - NULL, true, NULL); > + &indx->bitmap_run, > + ntfs3_bitmap_size(bit + 1), NULL, true, > + NULL); > if (err) > goto out1; > } > @@ -2089,7 +2090,7 @@ static int indx_shrink(struct ntfs_index *indx, struct ntfs_inode *ni, > if (in->name == I30_NAME) > ni->vfs_inode.i_size = new_data; > > - bpb = bitmap_size(bit); > + bpb = ntfs3_bitmap_size(bit); > if (bpb * 8 == nbits) > return 0; > > diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h > index 629403ede6e5..93333156aac6 100644 > --- a/fs/ntfs3/ntfs_fs.h > +++ b/fs/ntfs3/ntfs_fs.h > @@ -961,7 +961,7 @@ static inline bool run_is_empty(struct runs_tree *run) > } > > /* NTFS uses quad aligned bitmaps. */ > -static inline size_t bitmap_size(size_t bits) > +static inline size_t ntfs3_bitmap_size(size_t bits) > { > return ALIGN((bits + 7) >> 3, 8); > } This looks like duplicating BITS_TO_U64(). If so, why not just switch to using the macro while you're here? > diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c > index cfec5e0c7f66..b1fb6efe7084 100644 > --- a/fs/ntfs3/super.c > +++ b/fs/ntfs3/super.c > @@ -1285,7 +1285,7 @@ static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc) > > /* Check bitmap boundary. */ > tt = sbi->used.bitmap.nbits; > - if (inode->i_size < bitmap_size(tt)) { > + if (inode->i_size < ntfs3_bitmap_size(tt)) { > ntfs_err(sb, "$Bitmap is corrupted."); > err = -EINVAL; > goto put_inode_out; > -- > 2.41.0 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
From: Yury Norov <yury.norov@gmail.com> Date: Mon, 9 Oct 2023 09:50:15 -0700 > On Mon, Oct 09, 2023 at 05:10:18PM +0200, Alexander Lobakin wrote: >> bitmap_size() is a pretty generic name and one may want to use it for [...] >> @@ -961,7 +961,7 @@ static inline bool run_is_empty(struct runs_tree *run) >> } >> >> /* NTFS uses quad aligned bitmaps. */ >> -static inline size_t bitmap_size(size_t bits) >> +static inline size_t ntfs3_bitmap_size(size_t bits) >> { >> return ALIGN((bits + 7) >> 3, 8); >> } > > This looks like duplicating BITS_TO_U64(). If so, why not just switch > to using the macro while you're here? I thought that this return BITS_TO_U64(bits) * sizeof(u64); would give worse optimization, but actually add/remove: 0/0 grow/shrink: 0/6 up/down: 0/-32 (-32) Nice, makes sense to do it. > >> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c >> index cfec5e0c7f66..b1fb6efe7084 100644 >> --- a/fs/ntfs3/super.c >> +++ b/fs/ntfs3/super.c >> @@ -1285,7 +1285,7 @@ static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc) >> >> /* Check bitmap boundary. */ >> tt = sbi->used.bitmap.nbits; >> - if (inode->i_size < bitmap_size(tt)) { >> + if (inode->i_size < ntfs3_bitmap_size(tt)) { >> ntfs_err(sb, "$Bitmap is corrupted."); >> err = -EINVAL; >> goto put_inode_out; >> -- >> 2.41.0 Thanks, Olek
Derived from the PFCP support series[0] as this grew bigger (2 -> 14 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 14. 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/netdev/20230721071532.613888-1-marcin.szycik@linux.intel.com Alexander Lobakin (14): 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: rename bitmap_size() -> ntfs3_bitmap_size() btrfs: rename bitmap_set_bits() -> btrfs_bitmap_set_bits() bitmap: introduce generic optimized bitmap_size() bitmap: extend bitmap_{get,set}_value8() to bitmap_{get,set}_bits() 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 bitmap_{get,set}_bits() 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 | 10 +- 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 | 2 +- fs/ntfs3/super.c | 2 +- include/linux/bitmap.h | 59 ++++-- include/linux/bitops.h | 13 +- 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 | 200 +++++++++++++++++- 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, 1116 insertions(+), 571 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). Speaking of bitmap_{read,write}() from [1] vs bitmap_{get,set}_bits() from #09: they don't really conflict, because the former are generic-generic and support bound crossing, while the latter require the width to be a pow-2 and the offset to be a multiple of the width in order to preserve the optimization level as close to the current bitmap_{get,set}_value8() as possible... 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. [1] https://lore.kernel.org/lkml/20231006134529.2816540-2-glider@google.com