| Message ID | d4d0bb2c6fb0ad0a3666e9e0a78e432634f58a0d.1698452036.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: remove unused functions | expand |
On Sat, Oct 28, 2023 at 10:43:59AM +1030, Qu Wenruo wrote: > When compiling with clang 16.0.6, there are the following unused > functions get reported. > > - memmove_leaf_data() > - copy_leaf_data() > - memmove_leaf_items() > - copy_leaf_items() > Those are replaced by memmove/memcopy_extent_buffer(). > Thus they can be removed safely. The files in kernel-shared directory could have unused functions due to the file-to-file sync, so as long as the function exists in kernel then it should say in progs too. I was looking for some linker tricks to remove them from the final binary, but the resulting binaries increased the size (it's either function sections with garbage collection or LTO). The warnings could be suppressed on a case by case basis by some function attribute like __maybe_unused eventually. > - btrfs_inode_combine_flags() > This is only utilized by kernel for vfs inode structure, meanwhile > it's totally unused in progs as we don't have inode structure at all. > Thus it can be removed safely. Yeah same, if it's in kernel then it should stay. > - LOADU64() from blake2b-avx2.c > This is only utilized by the fallback implementation of > DECLARE_MESSAGE_WORD(). > We can move it it just before the fallback implementation. Ok.
On 2023/10/31 00:36, David Sterba wrote: > On Sat, Oct 28, 2023 at 10:43:59AM +1030, Qu Wenruo wrote: >> When compiling with clang 16.0.6, there are the following unused >> functions get reported. >> >> - memmove_leaf_data() >> - copy_leaf_data() >> - memmove_leaf_items() >> - copy_leaf_items() >> Those are replaced by memmove/memcopy_extent_buffer(). >> Thus they can be removed safely. > > The files in kernel-shared directory could have unused functions due to > the file-to-file sync, so as long as the function exists in kernel then > it should say in progs too. I was looking for some linker tricks to > remove them from the final binary, but the resulting binaries increased > the size (it's either function sections with garbage collection or LTO). > > The warnings could be suppressed on a case by case basis by some > function attribute like __maybe_unused eventually. Indeed, I forgot they can be synced from kernel. But at the same time, I can also argue that, if they are not utilized at all, there is no need to sync them at the moment. Thanks, Qu > >> - btrfs_inode_combine_flags() >> This is only utilized by kernel for vfs inode structure, meanwhile >> it's totally unused in progs as we don't have inode structure at all. >> Thus it can be removed safely. > > Yeah same, if it's in kernel then it should stay. > >> - LOADU64() from blake2b-avx2.c >> This is only utilized by the fallback implementation of >> DECLARE_MESSAGE_WORD(). >> We can move it it just before the fallback implementation. > > Ok.
On Tue, Oct 31, 2023 at 06:51:08AM +1030, Qu Wenruo wrote: > > > On 2023/10/31 00:36, David Sterba wrote: > > On Sat, Oct 28, 2023 at 10:43:59AM +1030, Qu Wenruo wrote: > >> When compiling with clang 16.0.6, there are the following unused > >> functions get reported. > >> > >> - memmove_leaf_data() > >> - copy_leaf_data() > >> - memmove_leaf_items() > >> - copy_leaf_items() > >> Those are replaced by memmove/memcopy_extent_buffer(). > >> Thus they can be removed safely. > > > > The files in kernel-shared directory could have unused functions due to > > the file-to-file sync, so as long as the function exists in kernel then > > it should say in progs too. I was looking for some linker tricks to > > remove them from the final binary, but the resulting binaries increased > > the size (it's either function sections with garbage collection or LTO). > > > > The warnings could be suppressed on a case by case basis by some > > function attribute like __maybe_unused eventually. > > Indeed, I forgot they can be synced from kernel. > > But at the same time, I can also argue that, if they are not utilized at > all, there is no need to sync them at the moment. Yeah, sort of. Files in kernel with a counterpart in progs should be synced even with some unused functions. We can potentially add __KERNEL__ ifdefs around bigger chunks of code if needed. Completely unused files don't make much sense, I tried that last week and then deleted the commits.
diff --git a/crypto/blake2b-avx2.c b/crypto/blake2b-avx2.c index 6c35427a5173..9fb1e146f2b9 100644 --- a/crypto/blake2b-avx2.c +++ b/crypto/blake2b-avx2.c @@ -41,12 +41,6 @@ #define LOADU(p) _mm256_loadu_si256( (__m256i *)(p) ) #define STOREU(p,r) _mm256_storeu_si256((__m256i *)(p), r) -static BLAKE2_INLINE uint64_t LOADU64(void const * p) { - uint64_t v; - memcpy(&v, p, sizeof v); - return v; -} - #define ROTATE16 _mm256_setr_epi8( 2, 3, 4, 5, 6, 7, 0, 1, 10, 11, 12, 13, 14, 15, 8, 9, \ 2, 3, 4, 5, 6, 7, 0, 1, 10, 11, 12, 13, 14, 15, 8, 9 ) @@ -186,6 +180,13 @@ ALIGN(64) static const uint32_t indices[12][16] = { const __m256i m7 = _mm256_broadcastsi128_si256(LOADU128((m) + 112)); \ __m256i t0, t1; #else + +static BLAKE2_INLINE uint64_t LOADU64(void const * p) { + uint64_t v; + memcpy(&v, p, sizeof v); + return v; +} + #define DECLARE_MESSAGE_WORDS(m) \ const uint64_t m0 = LOADU64((m) + 0); \ const uint64_t m1 = LOADU64((m) + 8); \ diff --git a/kernel-shared/ctree.c b/kernel-shared/ctree.c index 6bf9233b7358..4ee70facf894 100644 --- a/kernel-shared/ctree.c +++ b/kernel-shared/ctree.c @@ -66,91 +66,6 @@ static unsigned int leaf_data_end(const struct extent_buffer *leaf) return btrfs_item_offset(leaf, nr - 1); } -/* - * Move data in a @leaf (using memmove, safe for overlapping ranges). - * - * @leaf: leaf that we're doing a memmove on - * @dst_offset: item data offset we're moving to - * @src_offset: item data offset were' moving from - * @len: length of the data we're moving - * - * Wrapper around memmove_extent_buffer() that takes into account the header on - * the leaf. The btrfs_item offset's start directly after the header, so we - * have to adjust any offsets to account for the header in the leaf. This - * handles that math to simplify the callers. - */ -static inline void memmove_leaf_data(const struct extent_buffer *leaf, - unsigned long dst_offset, - unsigned long src_offset, - unsigned long len) -{ - memmove_extent_buffer(leaf, btrfs_item_nr_offset(leaf, 0) + dst_offset, - btrfs_item_nr_offset(leaf, 0) + src_offset, len); -} - -/* - * Copy item data from @src into @dst at the given @offset. - * - * @dst: destination leaf that we're copying into - * @src: source leaf that we're copying from - * @dst_offset: item data offset we're copying to - * @src_offset: item data offset were' copying from - * @len: length of the data we're copying - * - * Wrapper around copy_extent_buffer() that takes into account the header on - * the leaf. The btrfs_item offset's start directly after the header, so we - * have to adjust any offsets to account for the header in the leaf. This - * handles that math to simplify the callers. - */ -static inline void copy_leaf_data(const struct extent_buffer *dst, - const struct extent_buffer *src, - unsigned long dst_offset, - unsigned long src_offset, unsigned long len) -{ - copy_extent_buffer(dst, src, btrfs_item_nr_offset(dst, 0) + dst_offset, - btrfs_item_nr_offset(src, 0) + src_offset, len); -} - -/* - * Move items in a @leaf (using memmove). - * - * @dst: destination leaf for the items - * @dst_item: the item nr we're copying into - * @src_item: the item nr we're copying from - * @nr_items: the number of items to copy - * - * Wrapper around memmove_extent_buffer() that does the math to get the - * appropriate offsets into the leaf from the item numbers. - */ -static inline void memmove_leaf_items(const struct extent_buffer *leaf, - int dst_item, int src_item, int nr_items) -{ - memmove_extent_buffer(leaf, btrfs_item_nr_offset(leaf, dst_item), - btrfs_item_nr_offset(leaf, src_item), - nr_items * sizeof(struct btrfs_item)); -} - -/* - * Copy items from @src into @dst at the given @offset. - * - * @dst: destination leaf for the items - * @src: source leaf for the items - * @dst_item: the item nr we're copying into - * @src_item: the item nr we're copying from - * @nr_items: the number of items to copy - * - * Wrapper around copy_extent_buffer() that does the math to get the - * appropriate offsets into the leaf from the item numbers. - */ -static inline void copy_leaf_items(const struct extent_buffer *dst, - const struct extent_buffer *src, - int dst_item, int src_item, int nr_items) -{ - copy_extent_buffer(dst, src, btrfs_item_nr_offset(dst, dst_item), - btrfs_item_nr_offset(src, src_item), - nr_items * sizeof(struct btrfs_item)); -} - int btrfs_super_csum_size(const struct btrfs_super_block *sb) { const u16 csum_type = btrfs_super_csum_type(sb); diff --git a/kernel-shared/tree-checker.c b/kernel-shared/tree-checker.c index 003156795a43..bf9b7924bf2a 100644 --- a/kernel-shared/tree-checker.c +++ b/kernel-shared/tree-checker.c @@ -37,18 +37,6 @@ #include "kernel-shared/uapi/btrfs_tree.h" #include "common/internal.h" -/* - * btrfs_inode_item stores flags in a u64, btrfs_inode stores them in two - * separate u32s. These two functions convert between the two representations. - * - * MODIFIED: - * - Declared these here since this is the only place they're used currently. - */ -static inline u64 btrfs_inode_combine_flags(u32 flags, u32 ro_flags) -{ - return (flags | ((u64)ro_flags << 32)); -} - static inline void btrfs_inode_split_flags(u64 inode_item_flags, u32 *flags, u32 *ro_flags) {
When compiling with clang 16.0.6, there are the following unused functions get reported. - memmove_leaf_data() - copy_leaf_data() - memmove_leaf_items() - copy_leaf_items() Those are replaced by memmove/memcopy_extent_buffer(). Thus they can be removed safely. - btrfs_inode_combine_flags() This is only utilized by kernel for vfs inode structure, meanwhile it's totally unused in progs as we don't have inode structure at all. Thus it can be removed safely. - LOADU64() from blake2b-avx2.c This is only utilized by the fallback implementation of DECLARE_MESSAGE_WORD(). We can move it it just before the fallback implementation. Signed-off-by: Qu Wenruo <wqu@suse.com> --- crypto/blake2b-avx2.c | 13 +++--- kernel-shared/ctree.c | 85 ------------------------------------ kernel-shared/tree-checker.c | 12 ----- 3 files changed, 7 insertions(+), 103 deletions(-)