| Message ID | 20200310154909.3970-1-jiri@resnulli.us |
|---|---|
| Headers | show |
| Series | flow_offload: follow-ups to HW stats type patchset | expand |
On 10/03/2020 15:49, Jiri Pirko wrote: > This patchset includes couple of patches in reaction to the discussions > to the original HW stats patchset. The first patch is a fix, > the other two patches are basically cosmetics. > > Jiri Pirko (3): > flow_offload: fix allowed types check > flow_offload: turn hw_stats_type into dedicated enum > flow_offload: restrict driver to pass one allowed bit to > flow_action_hw_stats_types_check() > > .../net/ethernet/mellanox/mlx5/core/en_tc.c | 4 +- > include/net/flow_offload.h | 46 +++++++++++++------ > 2 files changed, 35 insertions(+), 15 deletions(-) > Acked-by: Edward Cree <ecree@solarflare.com>
On Tue, 10 Mar 2020 16:49:06 +0100 Jiri Pirko wrote: > This patchset includes couple of patches in reaction to the discussions > to the original HW stats patchset. The first patch is a fix, > the other two patches are basically cosmetics. Reviewed-by: Jakub Kicinski <kuba@kernel.org> This problem already exists, but writing a patch for nfp I noticed that there is no way for this: if (!flow_action_hw_stats_types_check(flow_action, extack, FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT)) return -EOPNOTSUPP; to fit on a line for either bit, which kind of sucks. I may send a rename...
From: Jiri Pirko <jiri@resnulli.us> Date: Tue, 10 Mar 2020 16:49:06 +0100 > This patchset includes couple of patches in reaction to the discussions > to the original HW stats patchset. The first patch is a fix, > the other two patches are basically cosmetics. Series applied, thanks Jiri.
Tue, Mar 10, 2020 at 08:05:19PM CET, kuba@kernel.org wrote: >On Tue, 10 Mar 2020 16:49:06 +0100 Jiri Pirko wrote: >> This patchset includes couple of patches in reaction to the discussions >> to the original HW stats patchset. The first patch is a fix, >> the other two patches are basically cosmetics. > >Reviewed-by: Jakub Kicinski <kuba@kernel.org> > >This problem already exists, but writing a patch for nfp I noticed that >there is no way for this: > > if (!flow_action_hw_stats_types_check(flow_action, extack, > FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT)) > return -EOPNOTSUPP; > >to fit on a line for either bit, which kind of sucks. Yeah, I was thinking about having flow_action_hw_stats_types_check as a macro and then just simply have: if (!flow_action_hw_stats_types_check(flow_action, extack, DELAYED)) return -EOPNOTSUPP; WDYT? > >I may send a rename...
On Wed, 11 Mar 2020 08:19:55 +0100 Jiri Pirko wrote: > Tue, Mar 10, 2020 at 08:05:19PM CET, kuba@kernel.org wrote: > >On Tue, 10 Mar 2020 16:49:06 +0100 Jiri Pirko wrote: > >> This patchset includes couple of patches in reaction to the discussions > >> to the original HW stats patchset. The first patch is a fix, > >> the other two patches are basically cosmetics. > > > >Reviewed-by: Jakub Kicinski <kuba@kernel.org> > > > >This problem already exists, but writing a patch for nfp I noticed that > >there is no way for this: > > > > if (!flow_action_hw_stats_types_check(flow_action, extack, > > FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT)) > > return -EOPNOTSUPP; > > > >to fit on a line for either bit, which kind of sucks. > > Yeah, I was thinking about having flow_action_hw_stats_types_check as a > macro and then just simply have: > > if (!flow_action_hw_stats_types_check(flow_action, extack, DELAYED)) > return -EOPNOTSUPP; > > WDYT? I'd rather have the 80+ lines than not be able to grep for it :( What's wrong with flow_action_stats_ok()? Also perhaps, flow_act as a prefix?
Wed, Mar 11, 2020 at 09:30:28PM CET, kuba@kernel.org wrote: >On Wed, 11 Mar 2020 08:19:55 +0100 Jiri Pirko wrote: >> Tue, Mar 10, 2020 at 08:05:19PM CET, kuba@kernel.org wrote: >> >On Tue, 10 Mar 2020 16:49:06 +0100 Jiri Pirko wrote: >> >> This patchset includes couple of patches in reaction to the discussions >> >> to the original HW stats patchset. The first patch is a fix, >> >> the other two patches are basically cosmetics. >> > >> >Reviewed-by: Jakub Kicinski <kuba@kernel.org> >> > >> >This problem already exists, but writing a patch for nfp I noticed that >> >there is no way for this: >> > >> > if (!flow_action_hw_stats_types_check(flow_action, extack, >> > FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT)) >> > return -EOPNOTSUPP; >> > >> >to fit on a line for either bit, which kind of sucks. >> >> Yeah, I was thinking about having flow_action_hw_stats_types_check as a >> macro and then just simply have: >> >> if (!flow_action_hw_stats_types_check(flow_action, extack, DELAYED)) >> return -EOPNOTSUPP; >> >> WDYT? > >I'd rather have the 80+ lines than not be able to grep for it :( > >What's wrong with flow_action_stats_ok()? Also perhaps, flow_act >as a prefix? Well nothing, just that we'd loose consistency. Everything is "flow_action_*" and also, the name you suggest might indicate that you are checking sw stats. :/
On Thu, 12 Mar 2020 08:03:59 +0100 Jiri Pirko wrote: > Wed, Mar 11, 2020 at 09:30:28PM CET, kuba@kernel.org wrote: > >On Wed, 11 Mar 2020 08:19:55 +0100 Jiri Pirko wrote: > >> Tue, Mar 10, 2020 at 08:05:19PM CET, kuba@kernel.org wrote: > >> >On Tue, 10 Mar 2020 16:49:06 +0100 Jiri Pirko wrote: > >> >> This patchset includes couple of patches in reaction to the discussions > >> >> to the original HW stats patchset. The first patch is a fix, > >> >> the other two patches are basically cosmetics. > >> > > >> >Reviewed-by: Jakub Kicinski <kuba@kernel.org> > >> > > >> >This problem already exists, but writing a patch for nfp I noticed that > >> >there is no way for this: > >> > > >> > if (!flow_action_hw_stats_types_check(flow_action, extack, > >> > FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT)) > >> > return -EOPNOTSUPP; > >> > > >> >to fit on a line for either bit, which kind of sucks. > >> > >> Yeah, I was thinking about having flow_action_hw_stats_types_check as a > >> macro and then just simply have: > >> > >> if (!flow_action_hw_stats_types_check(flow_action, extack, DELAYED)) > >> return -EOPNOTSUPP; > >> > >> WDYT? > > > >I'd rather have the 80+ lines than not be able to grep for it :( > > > >What's wrong with flow_action_stats_ok()? Also perhaps, flow_act > >as a prefix? > > Well nothing, just that we'd loose consistency. Everything is > "flow_action_*" and also, the name you suggest might indicate that you > are checking sw stats. :/ SW stats in flow action? flow stuff is an abstraction for HW/drivers.
This patchset includes couple of patches in reaction to the discussions to the original HW stats patchset. The first patch is a fix, the other two patches are basically cosmetics. Jiri Pirko (3): flow_offload: fix allowed types check flow_offload: turn hw_stats_type into dedicated enum flow_offload: restrict driver to pass one allowed bit to flow_action_hw_stats_types_check() .../net/ethernet/mellanox/mlx5/core/en_tc.c | 4 +- include/net/flow_offload.h | 46 +++++++++++++------ 2 files changed, 35 insertions(+), 15 deletions(-)