mbox series

[net-next,0/3] flow_offload: follow-ups to HW stats type patchset

Message ID 20200310154909.3970-1-jiri@resnulli.us
Headers show
Series flow_offload: follow-ups to HW stats type patchset | expand

Message

Jiri Pirko March 10, 2020, 3:49 p.m. UTC
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(-)

Comments

Edward Cree March 10, 2020, 5:47 p.m. UTC | #1
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>
Jakub Kicinski March 10, 2020, 7:05 p.m. UTC | #2
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...
David Miller March 10, 2020, 11:04 p.m. UTC | #3
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.
Jiri Pirko March 11, 2020, 7:19 a.m. UTC | #4
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...
Jakub Kicinski March 11, 2020, 8:30 p.m. UTC | #5
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?
Jiri Pirko March 12, 2020, 7:03 a.m. UTC | #6
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. :/
Jakub Kicinski March 12, 2020, 7:40 p.m. UTC | #7
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.