| Message ID | 20250328131022.452068-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | RDMA/mlx5: hide unused code | expand |
On Fri, Mar 28, 2025 at 02:10:17PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > After a recent rework, a few 'static const' objects have become unused: > > In file included from drivers/infiniband/hw/mlx5/fs.c:27: > drivers/infiniband/hw/mlx5/fs.c:26:28: error: 'mlx5_ib_object_MLX5_IB_OBJECT_STEERING_ANCHOR' defined but not used [-Werror=unused-const-variable=] > include/rdma/uverbs_named_ioctl.h:52:47: note: in expansion of macro 'UVERBS_OBJECT' > 52 | static const struct uverbs_object_def UVERBS_OBJECT(_object_id) = { \ > | ^~~~~~~~~~~~~ > drivers/infiniband/hw/mlx5/fs.c:3457:1: note: in expansion of macro 'DECLARE_UVERBS_NAMED_OBJECT' > 3457 | DECLARE_UVERBS_NAMED_OBJECT( > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > drivers/infiniband/hw/mlx5/fs.c:26:28: error: 'mlx5_ib_object_MLX5_IB_OBJECT_FLOW_MATCHER' defined but not used [-Werror=unused-const-variable=] > include/rdma/uverbs_named_ioctl.h:52:47: note: in expansion of macro 'UVERBS_OBJECT' > 52 | static const struct uverbs_object_def UVERBS_OBJECT(_object_id) = { \ > | ^~~~~~~~~~~~~ > drivers/infiniband/hw/mlx5/fs.c:3429:1: note: in expansion of macro 'DECLARE_UVERBS_NAMED_OBJECT' > 3429 | DECLARE_UVERBS_NAMED_OBJECT(MLX5_IB_OBJECT_FLOW_MATCHER, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > These come from a complex set of macros, and it would be possible to > shut up the warnings here by adding __maybe_unused annotations inside > of the macros, it seems cleaner in this case to have a large #ifdef block > around all the unused parts of the file, in order to still be able to > catch unused ones elsewhere. IDK, I'm tempted to revert 36e0d433672f ("RDMA/mlx5: Compile fs.c regardless of INFINIBAND_USER_ACCESS config") I don't think that was so well thought out. The entire file was designed to be USER_ACCESS only because it uses all this mechanism. #ifdefing away half the file seems ugly. Jason
On Fri, Mar 28, 2025, at 14:15, Jason Gunthorpe wrote: > On Fri, Mar 28, 2025 at 02:10:17PM +0100, Arnd Bergmann wrote: > > IDK, I'm tempted to revert 36e0d433672f ("RDMA/mlx5: Compile fs.c > regardless of INFINIBAND_USER_ACCESS config") > > I don't think that was so well thought out. The entire file was > designed to be USER_ACCESS only because it uses all this mechanism. > > #ifdefing away half the file seems ugly. That sounds better to me, yes. I assumed that a later patch required that one, but I don't see such a dependency. Arnd
On 28/03/2025 16:15, Jason Gunthorpe wrote: > On Fri, Mar 28, 2025 at 02:10:17PM +0100, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> After a recent rework, a few 'static const' objects have become unused: >> >> In file included from drivers/infiniband/hw/mlx5/fs.c:27: >> drivers/infiniband/hw/mlx5/fs.c:26:28: error: 'mlx5_ib_object_MLX5_IB_OBJECT_STEERING_ANCHOR' defined but not used [-Werror=unused-const-variable=] >> include/rdma/uverbs_named_ioctl.h:52:47: note: in expansion of macro 'UVERBS_OBJECT' >> 52 | static const struct uverbs_object_def UVERBS_OBJECT(_object_id) = { \ >> | ^~~~~~~~~~~~~ >> drivers/infiniband/hw/mlx5/fs.c:3457:1: note: in expansion of macro 'DECLARE_UVERBS_NAMED_OBJECT' >> 3457 | DECLARE_UVERBS_NAMED_OBJECT( >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> drivers/infiniband/hw/mlx5/fs.c:26:28: error: 'mlx5_ib_object_MLX5_IB_OBJECT_FLOW_MATCHER' defined but not used [-Werror=unused-const-variable=] >> include/rdma/uverbs_named_ioctl.h:52:47: note: in expansion of macro 'UVERBS_OBJECT' >> 52 | static const struct uverbs_object_def UVERBS_OBJECT(_object_id) = { \ >> | ^~~~~~~~~~~~~ >> drivers/infiniband/hw/mlx5/fs.c:3429:1: note: in expansion of macro 'DECLARE_UVERBS_NAMED_OBJECT' >> 3429 | DECLARE_UVERBS_NAMED_OBJECT(MLX5_IB_OBJECT_FLOW_MATCHER, >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> These come from a complex set of macros, and it would be possible to >> shut up the warnings here by adding __maybe_unused annotations inside >> of the macros, it seems cleaner in this case to have a large #ifdef block >> around all the unused parts of the file, in order to still be able to >> catch unused ones elsewhere. > > IDK, I'm tempted to revert 36e0d433672f ("RDMA/mlx5: Compile fs.c > regardless of INFINIBAND_USER_ACCESS config") I believe the issue arises because uverbs_destroy_def_handler() is declared in ib_verbs.h, but if uverbs isn't built, there is no corresponding implementation of this function. One possible solution is to provide an empty implementation when USER_ACCESS is not set, similar to how rdma_user_mmap_disassociate() is handled. Alternatively, since uverbs_destroy_def_handler() currently does nothing (always returning 0), we could simply define it as a static inline function inside ib_verbs.h and resolve the issue that way. An example of the first approach would be: diff --git a/drivers/infiniband/hw/mlx5/fs.c b/drivers/infiniband/hw/mlx5/fs.c index 251246c73b33..0ff9f18a71e8 100644 --- a/drivers/infiniband/hw/mlx5/fs.c +++ b/drivers/infiniband/hw/mlx5/fs.c @@ -3461,7 +3461,6 @@ DECLARE_UVERBS_NAMED_OBJECT( &UVERBS_METHOD(MLX5_IB_METHOD_STEERING_ANCHOR_DESTROY)); const struct uapi_definition mlx5_ib_flow_defs[] = { -#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) UAPI_DEF_CHAIN_OBJ_TREE_NAMED( MLX5_IB_OBJECT_FLOW_MATCHER), UAPI_DEF_CHAIN_OBJ_TREE( @@ -3472,7 +3471,6 @@ const struct uapi_definition mlx5_ib_flow_defs[] = { UAPI_DEF_CHAIN_OBJ_TREE_NAMED( MLX5_IB_OBJECT_STEERING_ANCHOR, UAPI_DEF_IS_OBJ_SUPPORTED(mlx5_ib_shared_ft_allowed)), -#endif {}, }; diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index d42eae69d9a8..901353796fbb 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -4790,7 +4790,14 @@ void roce_del_all_netdev_gids(struct ib_device *ib_dev, struct ib_ucontext *ib_uverbs_get_ucontext_file(struct ib_uverbs_file *ufile); +#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) int uverbs_destroy_def_handler(struct uverbs_attr_bundle *attrs); +#else +static inline int uverbs_destroy_def_handler(struct uverbs_attr_bundle *attrs) +{ + return 0; +} +#endif struct net_device *rdma_alloc_netdev(struct ib_device *device, u32 port_num, enum rdma_netdev_t type, const char *name, > > I don't think that was so well thought out. The entire file was > designed to be USER_ACCESS only because it uses all this mechanism. > > #ifdefing away half the file seems ugly. > > Jason
On 3/31/2025 12:30 PM, Mark Bloch wrote: > > On 28/03/2025 16:15, Jason Gunthorpe wrote: >> On Fri, Mar 28, 2025 at 02:10:17PM +0100, Arnd Bergmann wrote: >>> From: Arnd Bergmann <arnd@arndb.de> >>> >>> After a recent rework, a few 'static const' objects have become unused: >>> >>> In file included from drivers/infiniband/hw/mlx5/fs.c:27: >>> drivers/infiniband/hw/mlx5/fs.c:26:28: error: 'mlx5_ib_object_MLX5_IB_OBJECT_STEERING_ANCHOR' defined but not used [-Werror=unused-const-variable=] >>> include/rdma/uverbs_named_ioctl.h:52:47: note: in expansion of macro 'UVERBS_OBJECT' >>> 52 | static const struct uverbs_object_def UVERBS_OBJECT(_object_id) = { \ >>> | ^~~~~~~~~~~~~ >>> drivers/infiniband/hw/mlx5/fs.c:3457:1: note: in expansion of macro 'DECLARE_UVERBS_NAMED_OBJECT' >>> 3457 | DECLARE_UVERBS_NAMED_OBJECT( >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >>> drivers/infiniband/hw/mlx5/fs.c:26:28: error: 'mlx5_ib_object_MLX5_IB_OBJECT_FLOW_MATCHER' defined but not used [-Werror=unused-const-variable=] >>> include/rdma/uverbs_named_ioctl.h:52:47: note: in expansion of macro 'UVERBS_OBJECT' >>> 52 | static const struct uverbs_object_def UVERBS_OBJECT(_object_id) = { \ >>> | ^~~~~~~~~~~~~ >>> drivers/infiniband/hw/mlx5/fs.c:3429:1: note: in expansion of macro 'DECLARE_UVERBS_NAMED_OBJECT' >>> 3429 | DECLARE_UVERBS_NAMED_OBJECT(MLX5_IB_OBJECT_FLOW_MATCHER, >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >>> These come from a complex set of macros, and it would be possible to >>> shut up the warnings here by adding __maybe_unused annotations inside >>> of the macros, it seems cleaner in this case to have a large #ifdef block >>> around all the unused parts of the file, in order to still be able to >>> catch unused ones elsewhere. >> IDK, I'm tempted to revert 36e0d433672f ("RDMA/mlx5: Compile fs.c >> regardless of INFINIBAND_USER_ACCESS config") > I believe the issue arises because uverbs_destroy_def_handler() is > declared in ib_verbs.h, but if uverbs isn't built, there is no > corresponding implementation of this function. > > One possible solution is to provide an empty implementation when USER_ACCESS > is not set, similar to how rdma_user_mmap_disassociate() is handled. > > Alternatively, since uverbs_destroy_def_handler() currently does nothing > (always returning 0), we could simply define it as a static inline > function inside ib_verbs.h and resolve the issue that way. > > An example of the first approach would be: > > diff --git a/drivers/infiniband/hw/mlx5/fs.c b/drivers/infiniband/hw/mlx5/fs.c > index 251246c73b33..0ff9f18a71e8 100644 > --- a/drivers/infiniband/hw/mlx5/fs.c > +++ b/drivers/infiniband/hw/mlx5/fs.c > @@ -3461,7 +3461,6 @@ DECLARE_UVERBS_NAMED_OBJECT( > &UVERBS_METHOD(MLX5_IB_METHOD_STEERING_ANCHOR_DESTROY)); > > const struct uapi_definition mlx5_ib_flow_defs[] = { > -#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) > UAPI_DEF_CHAIN_OBJ_TREE_NAMED( > MLX5_IB_OBJECT_FLOW_MATCHER), > UAPI_DEF_CHAIN_OBJ_TREE( > @@ -3472,7 +3471,6 @@ const struct uapi_definition mlx5_ib_flow_defs[] = { > UAPI_DEF_CHAIN_OBJ_TREE_NAMED( > MLX5_IB_OBJECT_STEERING_ANCHOR, > UAPI_DEF_IS_OBJ_SUPPORTED(mlx5_ib_shared_ft_allowed)), > -#endif > {}, > }; > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index d42eae69d9a8..901353796fbb 100644 > --- a/include/rdma/ib_verbs.h > +++ b/include/rdma/ib_verbs.h > @@ -4790,7 +4790,14 @@ void roce_del_all_netdev_gids(struct ib_device *ib_dev, > > struct ib_ucontext *ib_uverbs_get_ucontext_file(struct ib_uverbs_file *ufile); > > +#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) > int uverbs_destroy_def_handler(struct uverbs_attr_bundle *attrs); > +#else > +static inline int uverbs_destroy_def_handler(struct uverbs_attr_bundle *attrs) > +{ > + return 0; > +} > +#endif > > struct net_device *rdma_alloc_netdev(struct ib_device *device, u32 port_num, > enum rdma_netdev_t type, const char *name, > >> I don't think that was so well thought out. The entire file was >> designed to be USER_ACCESS only because it uses all this mechanism. But not really the entire file , as you said below half of it is actually UVERBS , but it has the other half is unrelated to uverbs and its compilation shouldnt be dependent on it (that half is used by iproute2 mainly), which is the reason for this change. (Also separation into two files, IMO isn't really the best way to resolve it, since in the end they are both still flow steering code which leaves us with mark solution unless there is objections to it I was planning to send it next week). >> >> #ifdefing away half the file seems ugly. agreed, which is why I think mark bloch suggestion makes more sense, do you think it is okay ? or you think there is issues with it ? >> >> Jason
On Tue, Apr 01, 2025 at 06:20:09PM +0300, Patrisious Haddad wrote: > > > #ifdefing away half the file seems ugly. > agreed, which is why I think mark bloch suggestion makes more sense, do you > think it is okay ? or you think there is issues with it ? I think you should split the file so we get the proper level of code elimination. But Mark's is small and sane enough to fix the build problems. Jason
diff --git a/drivers/infiniband/hw/mlx5/fs.c b/drivers/infiniband/hw/mlx5/fs.c index 251246c73b33..f089abbed6af 100644 --- a/drivers/infiniband/hw/mlx5/fs.c +++ b/drivers/infiniband/hw/mlx5/fs.c @@ -684,12 +684,14 @@ enum flow_table_type { #define MLX5_FS_MAX_TYPES 6 #define MLX5_FS_MAX_ENTRIES BIT(16) -static bool __maybe_unused mlx5_ib_shared_ft_allowed(struct ib_device *device) +#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) +static bool mlx5_ib_shared_ft_allowed(struct ib_device *device) { struct mlx5_ib_dev *dev = to_mdev(device); return MLX5_CAP_GEN(dev->mdev, shared_object_to_user_object_allowed); } +#endif static struct mlx5_ib_flow_prio *_get_prio(struct mlx5_ib_dev *dev, struct mlx5_flow_namespace *ns, @@ -1888,6 +1890,7 @@ static struct ib_flow *mlx5_ib_create_flow(struct ib_qp *qp, return ERR_PTR(err); } +#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) static int mlx5_ib_fill_transport_ns_info(struct mlx5_ib_dev *dev, enum mlx5_flow_namespace_type type, u32 *flags, u16 *vport_idx, @@ -2227,6 +2230,7 @@ static struct mlx5_ib_flow_handler *raw_fs_rule_add( return ERR_PTR(err); } +#endif static void destroy_flow_action_raw(struct mlx5_ib_flow_action *maction) { @@ -2263,6 +2267,7 @@ static int mlx5_ib_destroy_flow_action(struct ib_flow_action *action) return 0; } +#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) static int mlx5_ib_ft_type_to_namespace(enum mlx5_ib_uapi_flow_table_type table_type, enum mlx5_flow_namespace_type *namespace) @@ -2618,6 +2623,7 @@ static int steering_anchor_create_ft(struct mlx5_ib_dev *dev, return 0; } +#endif static void steering_anchor_destroy_ft(struct mlx5_ib_flow_prio *ft_prio) { @@ -2627,6 +2633,7 @@ static void steering_anchor_destroy_ft(struct mlx5_ib_flow_prio *ft_prio) } } +#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) static int steering_anchor_create_fg_drop(struct mlx5_ib_flow_prio *ft_prio) { @@ -2658,6 +2665,7 @@ steering_anchor_create_fg_drop(struct mlx5_ib_flow_prio *ft_prio) return err; } +#endif static void steering_anchor_destroy_fg_drop(struct mlx5_ib_flow_prio *ft_prio) @@ -2668,6 +2676,7 @@ steering_anchor_destroy_fg_drop(struct mlx5_ib_flow_prio *ft_prio) } } +#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) static int steering_anchor_create_fg_goto_table(struct mlx5_ib_flow_prio *ft_prio) { @@ -2695,6 +2704,7 @@ steering_anchor_create_fg_goto_table(struct mlx5_ib_flow_prio *ft_prio) return err; } +#endif static void steering_anchor_destroy_fg_goto_table(struct mlx5_ib_flow_prio *ft_prio) @@ -2705,6 +2715,7 @@ steering_anchor_destroy_fg_goto_table(struct mlx5_ib_flow_prio *ft_prio) } } +#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) static int steering_anchor_create_rule_drop(struct mlx5_ib_flow_prio *ft_prio) { @@ -2726,6 +2737,7 @@ steering_anchor_create_rule_drop(struct mlx5_ib_flow_prio *ft_prio) return 0; } +#endif static void steering_anchor_destroy_rule_drop(struct mlx5_ib_flow_prio *ft_prio) { @@ -2735,6 +2747,7 @@ static void steering_anchor_destroy_rule_drop(struct mlx5_ib_flow_prio *ft_prio) } } +#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) static int steering_anchor_create_rule_goto_table(struct mlx5_ib_flow_prio *ft_prio) { @@ -2761,6 +2774,7 @@ steering_anchor_create_rule_goto_table(struct mlx5_ib_flow_prio *ft_prio) return 0; } +#endif static void steering_anchor_destroy_rule_goto_table(struct mlx5_ib_flow_prio *ft_prio) @@ -2771,6 +2785,7 @@ steering_anchor_destroy_rule_goto_table(struct mlx5_ib_flow_prio *ft_prio) } } +#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) static int steering_anchor_create_res(struct mlx5_ib_dev *dev, struct mlx5_ib_flow_prio *ft_prio, enum mlx5_flow_namespace_type ns_type) @@ -2810,6 +2825,7 @@ static int steering_anchor_create_res(struct mlx5_ib_dev *dev, return err; } +#endif static void mlx5_steering_anchor_destroy_res(struct mlx5_ib_flow_prio *ft_prio) { @@ -2820,6 +2836,7 @@ static void mlx5_steering_anchor_destroy_res(struct mlx5_ib_flow_prio *ft_prio) steering_anchor_destroy_ft(ft_prio); } +#ifdef CONFIG_INFINIBAND_USER_ACCESS static int steering_anchor_cleanup(struct ib_uobject *uobject, enum rdma_remove_reason why, struct uverbs_attr_bundle *attrs) @@ -2839,6 +2856,7 @@ static int steering_anchor_cleanup(struct ib_uobject *uobject, kfree(obj); return 0; } +#endif static void fs_cleanup_anchor(struct mlx5_ib_flow_prio *prio, int count) @@ -2858,6 +2876,7 @@ void mlx5_ib_fs_cleanup_anchor(struct mlx5_ib_dev *dev) fs_cleanup_anchor(dev->flow_db->rdma_tx, MLX5_IB_NUM_FLOW_FT); } +#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) static int mlx5_ib_matcher_ns(struct uverbs_attr_bundle *attrs, struct mlx5_ib_flow_matcher *obj) { @@ -3459,6 +3478,7 @@ DECLARE_UVERBS_NAMED_OBJECT( UVERBS_TYPE_ALLOC_IDR(steering_anchor_cleanup), &UVERBS_METHOD(MLX5_IB_METHOD_STEERING_ANCHOR_CREATE), &UVERBS_METHOD(MLX5_IB_METHOD_STEERING_ANCHOR_DESTROY)); +#endif const struct uapi_definition mlx5_ib_flow_defs[] = { #if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS)