| Message ID | 20200228172505.14386-2-jiri@resnulli.us |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: allow user specify TC action HW stats type | expand |
On Fri, Feb 28, 2020 at 06:24:54PM +0100, Jiri Pirko wrote: > From: Jiri Pirko <jiri@mellanox.com> > > Initially, pass "ANY" (struct is zeroed) to the drivers as that is the > current implicit value coming down to flow_offload. Add a bool > indicating that entries have mixed HW stats type. > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > --- > v1->v2: > - moved to actions > - add mixed bool > --- > include/net/flow_offload.h | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > index 4e864c34a1b0..eee1cbc5db3c 100644 > --- a/include/net/flow_offload.h > +++ b/include/net/flow_offload.h > @@ -154,6 +154,10 @@ enum flow_action_mangle_base { > FLOW_ACT_MANGLE_HDR_TYPE_UDP, > }; > > +enum flow_action_hw_stats_type { > + FLOW_ACTION_HW_STATS_TYPE_ANY, > +}; > + > typedef void (*action_destr)(void *priv); > > struct flow_action_cookie { > @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); > > struct flow_action_entry { > enum flow_action_id id; > + enum flow_action_hw_stats_type hw_stats_type; > action_destr destructor; > void *destructor_priv; > union { > @@ -228,6 +233,7 @@ struct flow_action_entry { > }; > > struct flow_action { > + bool mixed_hw_stats_types; Why do you want to place this built-in into the struct flow_action as a boolean? You can express the same thing through a new FLOW_ACTION_COUNTER. I know tc has implicit counters in actions, in that case tc can just generate the counter right after the action. Please, explain me why it would be a problem from the driver side to provide a separated counter action.
Sat, Feb 29, 2020 at 08:29:47PM CET, pablo@netfilter.org wrote: >On Fri, Feb 28, 2020 at 06:24:54PM +0100, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@mellanox.com> >> >> Initially, pass "ANY" (struct is zeroed) to the drivers as that is the >> current implicit value coming down to flow_offload. Add a bool >> indicating that entries have mixed HW stats type. >> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com> >> --- >> v1->v2: >> - moved to actions >> - add mixed bool >> --- >> include/net/flow_offload.h | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h >> index 4e864c34a1b0..eee1cbc5db3c 100644 >> --- a/include/net/flow_offload.h >> +++ b/include/net/flow_offload.h >> @@ -154,6 +154,10 @@ enum flow_action_mangle_base { >> FLOW_ACT_MANGLE_HDR_TYPE_UDP, >> }; >> >> +enum flow_action_hw_stats_type { >> + FLOW_ACTION_HW_STATS_TYPE_ANY, >> +}; >> + >> typedef void (*action_destr)(void *priv); >> >> struct flow_action_cookie { >> @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); >> >> struct flow_action_entry { >> enum flow_action_id id; >> + enum flow_action_hw_stats_type hw_stats_type; >> action_destr destructor; >> void *destructor_priv; >> union { >> @@ -228,6 +233,7 @@ struct flow_action_entry { >> }; >> >> struct flow_action { >> + bool mixed_hw_stats_types; > >Why do you want to place this built-in into the struct flow_action as >a boolean? Because it is convenient for the driver to know if multiple hw_stats_type values are used for multiple actions. > >You can express the same thing through a new FLOW_ACTION_COUNTER. I don't see how. >I know tc has implicit counters in actions, in that case tc can just >generate the counter right after the action. I don't follow. Each action has a separate stats. > >Please, explain me why it would be a problem from the driver side to >provide a separated counter action. I don't see any point in doing that. The action itself implies that has stats, you don't need a separate action for that for the flow_offload abstraction layer. What you would end up with is: counter_action1, actual_action1, counter_action2, actual_action2,... What is the point of that?
On Sun, Mar 01, 2020 at 09:44:43AM +0100, Jiri Pirko wrote: > Sat, Feb 29, 2020 at 08:29:47PM CET, pablo@netfilter.org wrote: > >On Fri, Feb 28, 2020 at 06:24:54PM +0100, Jiri Pirko wrote: [...] > >> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > >> index 4e864c34a1b0..eee1cbc5db3c 100644 > >> --- a/include/net/flow_offload.h > >> +++ b/include/net/flow_offload.h > >> @@ -154,6 +154,10 @@ enum flow_action_mangle_base { > >> FLOW_ACT_MANGLE_HDR_TYPE_UDP, > >> }; > >> > >> +enum flow_action_hw_stats_type { > >> + FLOW_ACTION_HW_STATS_TYPE_ANY, > >> +}; > >> + > >> typedef void (*action_destr)(void *priv); > >> > >> struct flow_action_cookie { > >> @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); > >> > >> struct flow_action_entry { > >> enum flow_action_id id; > >> + enum flow_action_hw_stats_type hw_stats_type; > >> action_destr destructor; > >> void *destructor_priv; > >> union { > >> @@ -228,6 +233,7 @@ struct flow_action_entry { > >> }; > >> > >> struct flow_action { > >> + bool mixed_hw_stats_types; > > > >Why do you want to place this built-in into the struct flow_action as > >a boolean? > > Because it is convenient for the driver to know if multiple hw_stats_type > values are used for multiple actions. > > >You can express the same thing through a new FLOW_ACTION_COUNTER. [...] > >Please, explain me why it would be a problem from the driver side to > >provide a separated counter action. > > I don't see any point in doing that. The action itself implies that has > stats, you don't need a separate action for that for the flow_offload > abstraction layer. What you would end up with is: > counter_action1, actual_action1, counter_action2, actual_action2,... > > What is the point of that? Yes, it's a bit more work for tc to generate counter action + actual action. However, netfilter has two ways to use counters: 1) per-rule counter, in this case the counter is updated after rule matching, right before calling the action. This is the legacy mode. 2) explicit counter action, in this case the user specifies explicitly that it needs a counter in a given position of the rule. This counter might come before or after the actual action. ethtool does not have counters yet. Now there is a netlink interface for it, there might be counters there at some point. I'm suggesting a model that would work for the existing front-ends using the flow_action API.
Mon, Mar 02, 2020 at 02:20:16PM CET, pablo@netfilter.org wrote: >On Sun, Mar 01, 2020 at 09:44:43AM +0100, Jiri Pirko wrote: >> Sat, Feb 29, 2020 at 08:29:47PM CET, pablo@netfilter.org wrote: >> >On Fri, Feb 28, 2020 at 06:24:54PM +0100, Jiri Pirko wrote: >[...] >> >> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h >> >> index 4e864c34a1b0..eee1cbc5db3c 100644 >> >> --- a/include/net/flow_offload.h >> >> +++ b/include/net/flow_offload.h >> >> @@ -154,6 +154,10 @@ enum flow_action_mangle_base { >> >> FLOW_ACT_MANGLE_HDR_TYPE_UDP, >> >> }; >> >> >> >> +enum flow_action_hw_stats_type { >> >> + FLOW_ACTION_HW_STATS_TYPE_ANY, >> >> +}; >> >> + >> >> typedef void (*action_destr)(void *priv); >> >> >> >> struct flow_action_cookie { >> >> @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); >> >> >> >> struct flow_action_entry { >> >> enum flow_action_id id; >> >> + enum flow_action_hw_stats_type hw_stats_type; >> >> action_destr destructor; >> >> void *destructor_priv; >> >> union { >> >> @@ -228,6 +233,7 @@ struct flow_action_entry { >> >> }; >> >> >> >> struct flow_action { >> >> + bool mixed_hw_stats_types; >> > >> >Why do you want to place this built-in into the struct flow_action as >> >a boolean? >> >> Because it is convenient for the driver to know if multiple hw_stats_type >> values are used for multiple actions. >> >> >You can express the same thing through a new FLOW_ACTION_COUNTER. >[...] >> >Please, explain me why it would be a problem from the driver side to >> >provide a separated counter action. >> >> I don't see any point in doing that. The action itself implies that has >> stats, you don't need a separate action for that for the flow_offload >> abstraction layer. What you would end up with is: >> counter_action1, actual_action1, counter_action2, actual_action2,... >> >> What is the point of that? > >Yes, it's a bit more work for tc to generate counter action + actual >action. > >However, netfilter has two ways to use counters: > >1) per-rule counter, in this case the counter is updated after rule > matching, right before calling the action. This is the legacy mode. > >2) explicit counter action, in this case the user specifies explicitly > that it needs a counter in a given position of the rule. This > counter might come before or after the actual action. > >ethtool does not have counters yet. Now there is a netlink interface >for it, there might be counters there at some point. > >I'm suggesting a model that would work for the existing front-ends >using the flow_action API. I see. I'm interested in 1) now. If you ever want to implement 2), I see no problem in doing it.
On 02/03/2020 13:20, Pablo Neira Ayuso wrote: > 2) explicit counter action, in this case the user specifies explicitly > that it needs a counter in a given position of the rule. This > counter might come before or after the actual action. But the existing API can already do this, with a gact pipe. Plus, Jiri's new API will allow specifying a counter on any action (rather than only, implicitly, those which have .stats_update()) should that prove to be necessary. I really think the 'explicit counter action' is a solution in search of a problem, let's not add random orthogonality violations. (Equally if the counter action had been there first, I'd be against adding counters to the other actions.) -ed
On Mon, Mar 02, 2020 at 04:29:32PM +0000, Edward Cree wrote: > On 02/03/2020 13:20, Pablo Neira Ayuso wrote: > > 2) explicit counter action, in this case the user specifies explicitly > > that it needs a counter in a given position of the rule. This > > counter might come before or after the actual action. > > But the existing API can already do this, with a gact pipe. Plus, Jiri's > new API will allow specifying a counter on any action (rather than only, > implicitly, those which have .stats_update()) should that prove to be > necessary. > > I really think the 'explicit counter action' is a solution in search of a > problem, let's not add random orthogonality violations. (Equally if the > counter action had been there first, I'd be against adding counters to > the other actions.) It looks to me that you want to restrict the API to tc for no good _technical_ reason.
On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote: > On Mon, Mar 02, 2020 at 04:29:32PM +0000, Edward Cree wrote: > > On 02/03/2020 13:20, Pablo Neira Ayuso wrote: > > > 2) explicit counter action, in this case the user specifies explicitly > > > that it needs a counter in a given position of the rule. This > > > counter might come before or after the actual action. > > > > But the existing API can already do this, with a gact pipe. Plus, Jiri's > > new API will allow specifying a counter on any action (rather than only, > > implicitly, those which have .stats_update()) should that prove to be > > necessary. > > > > I really think the 'explicit counter action' is a solution in search of a > > problem, let's not add random orthogonality violations. (Equally if the > > counter action had been there first, I'd be against adding counters to > > the other actions.) > > It looks to me that you want to restrict the API to tc for no good > _technical_ reason. Undeniably part of the reason is that given how complex flow offloads got there may be some resistance to large re-factoring. IMHO well thought out refactoring of stats is needed.. but I'm not convinced this is the direction. Could you give us clearer understanding of what the use cases for the counter action is? AFAIK right now actions do the accounting on input. That seems like the only logical option. Either action takes the packet out of the action pipeline, in which case even the counter action after will not see it, or it doesn't and the input counter of the next action can be used. Given counters must be next to real actions and not other counter to have value, having them as a separate action seems to make no difference at all (if users are silly, we can use the pipe/no-op). IOW modeling the stats as attribute of other actions or a separate action is entirely equivalent, and there's nothing to be gained from moving from the existing scheme to explicit actions... other than it'd make it look more like nft actions... :)
On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote: > On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote: > > On Mon, Mar 02, 2020 at 04:29:32PM +0000, Edward Cree wrote: > > > On 02/03/2020 13:20, Pablo Neira Ayuso wrote: > > > > 2) explicit counter action, in this case the user specifies explicitly > > > > that it needs a counter in a given position of the rule. This > > > > counter might come before or after the actual action. > > > > > > But the existing API can already do this, with a gact pipe. Plus, Jiri's > > > new API will allow specifying a counter on any action (rather than only, > > > implicitly, those which have .stats_update()) should that prove to be > > > necessary. > > > > > > I really think the 'explicit counter action' is a solution in search of a > > > problem, let's not add random orthogonality violations. (Equally if the > > > counter action had been there first, I'd be against adding counters to > > > the other actions.) > > > > It looks to me that you want to restrict the API to tc for no good > > _technical_ reason. > > Undeniably part of the reason is that given how complex flow offloads > got there may be some resistance to large re-factoring. IMHO well > thought out refactoring of stats is needed.. but I'm not convinced > this is the direction. > > Could you give us clearer understanding of what the use cases for the > counter action is? > > AFAIK right now actions do the accounting on input. That seems like the > only logical option. Either action takes the packet out of the action > pipeline, in which case even the counter action after will not see it, > or it doesn't and the input counter of the next action can be used. > > Given counters must be next to real actions and not other counter > to have value, having them as a separate action seems to make no > difference at all (if users are silly, we can use the pipe/no-op). This model that is proposed here is correct in the tc world, where counters are tied to actions (as you describe above). However, the flow_offload API already supports for ethtool and netfilter these days. In Netfilter, counters are detached from actions. Obviously, a counter must be placed before the action _if_ the action gets the packet out of the pipeline, e.g. ip saddr 1.1.1.1 counter drop In this case, the counter is placed before the 'drop' action. Users that need no counters have to remove 'counter' from the rule syntax to opt-out. > IOW modeling the stats as attribute of other actions or a separate > action is entirely equivalent, and there's nothing to be gained from > moving from the existing scheme to explicit actions... other than it'd > make it look more like nft actions... :) I just wonder if a model that allows tc and netfilter to use this new statistics infrastructure would make everyone happy. My understanding is that it is not far away from what this patchset provides. The retorical question here probably is if you still want to allow the Netfilter front-end to benefit from this new flow_action API extension. The real question is: if you think this tc counter+action scheme can be used by netfilter, then please explain how.
On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote: > On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote: > > On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote: > > > On Mon, Mar 02, 2020 at 04:29:32PM +0000, Edward Cree wrote: > > > > On 02/03/2020 13:20, Pablo Neira Ayuso wrote: > > > > > 2) explicit counter action, in this case the user specifies explicitly > > > > > that it needs a counter in a given position of the rule. This > > > > > counter might come before or after the actual action. > > > > > > > > But the existing API can already do this, with a gact pipe. Plus, Jiri's > > > > new API will allow specifying a counter on any action (rather than only, > > > > implicitly, those which have .stats_update()) should that prove to be > > > > necessary. > > > > > > > > I really think the 'explicit counter action' is a solution in search of a > > > > problem, let's not add random orthogonality violations. (Equally if the > > > > counter action had been there first, I'd be against adding counters to > > > > the other actions.) > > > > > > It looks to me that you want to restrict the API to tc for no good > > > _technical_ reason. > > > > Undeniably part of the reason is that given how complex flow offloads > > got there may be some resistance to large re-factoring. IMHO well > > thought out refactoring of stats is needed.. but I'm not convinced > > this is the direction. > > > > Could you give us clearer understanding of what the use cases for the > > counter action is? > > > > AFAIK right now actions do the accounting on input. That seems like the > > only logical option. Either action takes the packet out of the action > > pipeline, in which case even the counter action after will not see it, > > or it doesn't and the input counter of the next action can be used. > > > > Given counters must be next to real actions and not other counter > > to have value, having them as a separate action seems to make no > > difference at all (if users are silly, we can use the pipe/no-op). > > This model that is proposed here is correct in the tc world, where > counters are tied to actions (as you describe above). However, the > flow_offload API already supports for ethtool and netfilter these > days. > > In Netfilter, counters are detached from actions. Obviously, a counter > must be placed before the action _if_ the action gets the packet out > of the pipeline, e.g. > > ip saddr 1.1.1.1 counter drop > > In this case, the counter is placed before the 'drop' action. Users > that need no counters have to remove 'counter' from the rule syntax to > opt-out. In Jiri's set if counter exists DROP should get the ANY flag, if counter is not there - DISABLED. > > IOW modeling the stats as attribute of other actions or a separate > > action is entirely equivalent, and there's nothing to be gained from > > moving from the existing scheme to explicit actions... other than it'd > > make it look more like nft actions... :) > > I just wonder if a model that allows tc and netfilter to use this new > statistics infrastructure would make everyone happy. My understanding > is that it is not far away from what this patchset provides. > > The retorical question here probably is if you still want to allow the > Netfilter front-end to benefit from this new flow_action API > extension. > > The real question is: if you think this tc counter+action scheme can > be used by netfilter, then please explain how. In Jiri's latest patch set the counter type is per action, so just "merge right" the counter info into the next action and the models are converted. If user is silly and has multiple counter actions in a row - the pipe/no-op action comes into play (that isn't part of this set, as Jiri said). Can you give us examples of what wouldn't work? Can you for instance share the counter across rules? Also neither proposal addresses the problem of reporting _different_ counter values at different stages in the pipeline, i.e. moving from stats per flow to per action. But nobody seems to be willing to work on that. AFAICT with Jiri's change we only need one check in the drivers to convert from old scheme to new, with explicit action we need two (additional one being ignoring the counter action). Not a big deal, but 1 is less than 2 🤷♂️
On Mon, Mar 02, 2020 at 02:49:28PM -0800, Jakub Kicinski wrote: > On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote: [...] > > The real question is: if you think this tc counter+action scheme can > > be used by netfilter, then please explain how. > > In Jiri's latest patch set the counter type is per action, so just > "merge right" the counter info into the next action and the models > are converted. The input "merge right" approach might work. > If user is silly and has multiple counter actions in a row - the > pipe/no-op action comes into play (that isn't part of this set, > as Jiri said). Probably gact pipe action with counters can be mapped to the counter action that netfilter needs. Is this a valid use-case you consider for the tc hardware offload? > Can you give us examples of what wouldn't work? Can you for instance > share the counter across rules? Yes, there might be counters that are shared accross rules, see nfacct. Two different rules might refer to the same counter, IIRC there is a way to do this in tc too. > Also neither proposal addresses the problem of reporting _different_ > counter values at different stages in the pipeline, i.e. moving from > stats per flow to per action. But nobody seems to be willing to work > on that. You mean, in case that different counter types are specified, eg. one action using delayed and another action using immediate? > AFAICT with Jiri's change we only need one check in the drivers to > convert from old scheme to new, with explicit action we need two > (additional one being ignoring the counter action). Not a big deal, > but 1 is less than 2 🤷♂️ What changes are expected to retrieve counter stats? Will per-flow stats remain in place after this place? Thank you.
On 02/03/2020 22:49, Jakub Kicinski wrote: > On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote: >> On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote: >>> On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote: >>>> It looks to me that you want to restrict the API to tc for no good >>>> _technical_ reason. The technical reason is that having two ways to do things where one would suffice means more code to be written, tested, debugged. So if you want to add this you need to convince us that the existing way (a) doesn't meet your needs and (b) can't be extended to cover them. > Also neither proposal addresses the problem of reporting _different_ > counter values at different stages in the pipeline, i.e. moving from > stats per flow to per action. But nobody seems to be willing to work > on that. For the record, I produced a patch series[1] to support that, but it wasn't acceptable because none of the in-tree drivers implemented the facility. My hope is that we'll be upstreaming our new driver Real Soon Now™, at which point I'll rebase and repost those changes. Alternatively if any other vendor wants to support it in their driver they could use those patches as a base. -ed [1]: http://patchwork.ozlabs.org/cover/1110071/ ("flow_offload: Re-add per-action statistics")
On 28/02/2020 17:24, Jiri Pirko wrote: > From: Jiri Pirko <jiri@mellanox.com> > > Initially, pass "ANY" (struct is zeroed) to the drivers as that is the > current implicit value coming down to flow_offload. Add a bool > indicating that entries have mixed HW stats type. > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > --- > v1->v2: > - moved to actions > - add mixed bool > --- > include/net/flow_offload.h | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > index 4e864c34a1b0..eee1cbc5db3c 100644 > --- a/include/net/flow_offload.h > +++ b/include/net/flow_offload.h > @@ -154,6 +154,10 @@ enum flow_action_mangle_base { > FLOW_ACT_MANGLE_HDR_TYPE_UDP, > }; > > +enum flow_action_hw_stats_type { > + FLOW_ACTION_HW_STATS_TYPE_ANY, > +}; > + > typedef void (*action_destr)(void *priv); > > struct flow_action_cookie { > @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); > > struct flow_action_entry { > enum flow_action_id id; > + enum flow_action_hw_stats_type hw_stats_type; > action_destr destructor; > void *destructor_priv; > union { > @@ -228,6 +233,7 @@ struct flow_action_entry { > }; > > struct flow_action { > + bool mixed_hw_stats_types; Some sort of comment in the commit message the effect that this will be set in patch #12 would be nice (and would have saved me some reviewing time looking for it ;) Strictly speaking this violates SPOT; I know a helper to calculate this 'at runtime' in the driver would have to loop over actions, but it's control-plane so performance doesn't matter :grin: I'd suggest something like adding an internal-use-only MIXED value to the enum, and then having a helper enum flow_action_hw_state_type flow_action_single_stats_type(struct flow_action *action); which could return FLOW_ACTION_HW_STATS_TYPE_MIXED or else whichever type all the actions have (except that the 'different' check might be further complicated to ignore DISABLED, since most flow_action_entries will be for TC actions with no .update_stats() which thus don't want stats and can use DISABLED to express that). That then avoids having to rely on the first entry having the stats type (so flow_action_first_entry_get() goes away). -ed > unsigned int num_entries; > struct flow_action_entry entries[0]; > };
On Tue, 3 Mar 2020 18:55:54 +0000 Edward Cree wrote: > > Also neither proposal addresses the problem of reporting _different_ > > counter values at different stages in the pipeline, i.e. moving from > > stats per flow to per action. But nobody seems to be willing to work > > on that. > For the record, I produced a patch series[1] to support that, but it > wasn't acceptable because none of the in-tree drivers implemented the > facility. My hope is that we'll be upstreaming our new driver Real > Soon Now™, at which point I'll rebase and repost those changes. Sorry, I wasn't completely fair :) Looking forward :) > Alternatively if any other vendor wants to support it in their driver > they could use those patches as a base.
On Tue, 3 Mar 2020 18:25:25 +0100 Pablo Neira Ayuso wrote: > On Mon, Mar 02, 2020 at 02:49:28PM -0800, Jakub Kicinski wrote: > > On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote: > [...] > > > The real question is: if you think this tc counter+action scheme can > > > be used by netfilter, then please explain how. > > > > In Jiri's latest patch set the counter type is per action, so just > > "merge right" the counter info into the next action and the models > > are converted. > > The input "merge right" approach might work. > > > If user is silly and has multiple counter actions in a row - the > > pipe/no-op action comes into play (that isn't part of this set, > > as Jiri said). > > Probably gact pipe action with counters can be mapped to the counter > action that netfilter needs. Is this a valid use-case you consider for > the tc hardware offload? Once actions can be shared I think it'd be a pretty useful thing for tc hardware offloads in case HW has limited counters. > > Can you give us examples of what wouldn't work? Can you for instance > > share the counter across rules? > > Yes, there might be counters that are shared accross rules, see > nfacct. Two different rules might refer to the same counter, IIRC > there is a way to do this in tc too. Yup, not implemented for offload, tho. > > Also neither proposal addresses the problem of reporting _different_ > > counter values at different stages in the pipeline, i.e. moving from > > stats per flow to per action. But nobody seems to be willing to work > > on that. > > You mean, in case that different counter types are specified, eg. one > action using delayed and another action using immediate? I meant the work Ed just pointed to, and what you ask about below. > > AFAICT with Jiri's change we only need one check in the drivers to > > convert from old scheme to new, with explicit action we need two > > (additional one being ignoring the counter action). Not a big deal, > > but 1 is less than 2 🤷♂️ > > What changes are expected to retrieve counter stats? > > Will per-flow stats remain in place after this place? In theory it doesn't have to, because action stats are more flexible. In practice I doubt anyone will take on the conversion, so we'll have to live with two ways for a while..
On Tue, Mar 03, 2020 at 06:55:54PM +0000, Edward Cree wrote: > On 02/03/2020 22:49, Jakub Kicinski wrote: > > On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote: > >> On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote: > >>> On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote: > >>>> It looks to me that you want to restrict the API to tc for no good > >>>> _technical_ reason. > > The technical reason is that having two ways to do things where one would > suffice means more code to be written, tested, debugged. So if you want > to add this you need to convince us that the existing way (a) doesn't > meet your needs and (b) can't be extended to cover them. One single unified way to express the hardware offload for _every_ supported frontend is the way to go. The flow_offload API provides a framework to model all hardware offloads for each existing front-end. I understand your motivation might be a specific front-end of your choice, that's fair enough. > > Also neither proposal addresses the problem of reporting _different_ > > counter values at different stages in the pipeline, i.e. moving from > > stats per flow to per action. But nobody seems to be willing to work > > on that. > For the record, I produced a patch series[1] to support that, but it > wasn't acceptable because none of the in-tree drivers implemented the > facility. My hope is that we'll be upstreaming our new driver Real > Soon Now™, at which point I'll rebase and repost those changes. > Alternatively if any other vendor wants to support it in their driver > they could use those patches as a base. Great, I am very much looking forward to reviewing your upstream code. Just keep in my mind that whatever proposal you make must work for netfilter too. Thank you.
On 03/03/2020 20:27, Pablo Neira Ayuso wrote: > On Tue, Mar 03, 2020 at 06:55:54PM +0000, Edward Cree wrote: >> On 02/03/2020 22:49, Jakub Kicinski wrote: >>> On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote: >>>> On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote: >>>>> On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote: >>>>>> It looks to me that you want to restrict the API to tc for no good >>>>>> _technical_ reason. >> The technical reason is that having two ways to do things where one would >> suffice means more code to be written, tested, debugged. So if you want >> to add this you need to convince us that the existing way (a) doesn't >> meet your needs and (b) can't be extended to cover them. > One single unified way to express the hardware offload for _every_ > supported frontend is the way to go. The flow_offload API provides a > framework to model all hardware offloads for each existing front-end. > > I understand your motivation might be a specific front-end of your > choice, that's fair enough. I think we've misunderstood each other (90% my fault). When you wrote "restrict the API to tc" I read that as "restrict growth of the API for flow offloading" (which I *do* want); I've now re-parsed and believe you meant it as "limit the API so that only tc may use it" (which is not my desire at all). Thus, when I spoke of "two ways to do things" I meant that _within_ the (unified) flow_offload API there should be a single approach to stats (the counters attached to actions), to which levels above and below it impedance-match as necessary (e.g. by merging netfilter count actions onto the following action as Jakub described), rather than bundling two interfaces (tc-style counters and separate counter actions) into one API (which would mean that drivers would all need to write code to handle both kinds, at no gain of expressiveness). I was *not* referring to tc and netfilter as the "two different ways", but I can see why you read it that way. I hope that makes sense now. -ed
On Tue, Mar 03, 2020 at 09:06:48PM +0000, Edward Cree wrote: > On 03/03/2020 20:27, Pablo Neira Ayuso wrote: > > On Tue, Mar 03, 2020 at 06:55:54PM +0000, Edward Cree wrote: > >> On 02/03/2020 22:49, Jakub Kicinski wrote: > >>> On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote: > >>>> On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote: > >>>>> On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote: > >>>>>> It looks to me that you want to restrict the API to tc for no good > >>>>>> _technical_ reason. > >> The technical reason is that having two ways to do things where one would > >> suffice means more code to be written, tested, debugged. So if you want > >> to add this you need to convince us that the existing way (a) doesn't > >> meet your needs and (b) can't be extended to cover them. > > One single unified way to express the hardware offload for _every_ > > supported frontend is the way to go. The flow_offload API provides a > > framework to model all hardware offloads for each existing front-end. > > > > I understand your motivation might be a specific front-end of your > > choice, that's fair enough. > I think we've misunderstood each other (90% my fault). > > When you wrote "restrict the API to tc" I read that as "restrict growth of > the API for flow offloading" (which I *do* want); I've now re-parsed and > believe you meant it as "limit the API so that only tc may use it" (which > is not my desire at all). > > Thus, when I spoke of "two ways to do things" I meant that _within_ the > (unified) flow_offload API there should be a single approach to stats > (the counters attached to actions), to which levels above and below it > impedance-match as necessary (e.g. by merging netfilter count actions > onto the following action as Jakub described) rather than bundling > two interfaces (tc-style counters and separate counter actions) > into one API (which would mean that drivers would all need to write > code to handle both kinds, at no gain of expressiveness). It's not that natural to express counters like you prefer for netfilter, but fair enough, we'll carry on that extra burden of merging counters to actions. Sometimes decisions just need a second round: I will expect broken endianness in drivers because of the 32-bit word choice for the payload mangling API. But that's a different story. Noone to blame, there is still experimentation going on in this API. > I was *not* referring to tc and netfilter > as the "two different ways", but I can see why you read it that > way. I don't follow, sorry. > I hope that makes sense now. Sure, thank you.
Tue, Mar 03, 2020 at 08:13:23PM CET, ecree@solarflare.com wrote: >On 28/02/2020 17:24, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@mellanox.com> >> >> Initially, pass "ANY" (struct is zeroed) to the drivers as that is the >> current implicit value coming down to flow_offload. Add a bool >> indicating that entries have mixed HW stats type. >> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com> >> --- >> v1->v2: >> - moved to actions >> - add mixed bool >> --- >> include/net/flow_offload.h | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h >> index 4e864c34a1b0..eee1cbc5db3c 100644 >> --- a/include/net/flow_offload.h >> +++ b/include/net/flow_offload.h >> @@ -154,6 +154,10 @@ enum flow_action_mangle_base { >> FLOW_ACT_MANGLE_HDR_TYPE_UDP, >> }; >> >> +enum flow_action_hw_stats_type { >> + FLOW_ACTION_HW_STATS_TYPE_ANY, >> +}; >> + >> typedef void (*action_destr)(void *priv); >> >> struct flow_action_cookie { >> @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); >> >> struct flow_action_entry { >> enum flow_action_id id; >> + enum flow_action_hw_stats_type hw_stats_type; >> action_destr destructor; >> void *destructor_priv; >> union { >> @@ -228,6 +233,7 @@ struct flow_action_entry { >> }; >> >> struct flow_action { >> + bool mixed_hw_stats_types; >Some sort of comment in the commit message the effect that this will > be set in patch #12 would be nice (and would have saved me some > reviewing time looking for it ;) >Strictly speaking this violates SPOT; I know a helper to calculate > this 'at runtime' in the driver would have to loop over actions, > but it's control-plane so performance doesn't matter :grin: That is what I wanted to avoid. >I'd suggest something like adding an internal-use-only MIXED value to > the enum, and then having a helper > enum flow_action_hw_state_type flow_action_single_stats_type(struct flow_action *action); > which could return FLOW_ACTION_HW_STATS_TYPE_MIXED or else whichever > type all the actions have (except that the 'different' check might > be further complicated to ignore DISABLED, since most > flow_action_entries will be for TC actions with no .update_stats() > which thus don't want stats and can use DISABLED to express that). >That then avoids having to rely on the first entry having the stats > type (so flow_action_first_entry_get() goes away). No problem. I can call a helper that would go over the entries from driver. As you say, it is a slow path. Will do. > >-ed >> unsigned int num_entries; >> struct flow_action_entry entries[0]; >> };
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index 4e864c34a1b0..eee1cbc5db3c 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -154,6 +154,10 @@ enum flow_action_mangle_base { FLOW_ACT_MANGLE_HDR_TYPE_UDP, }; +enum flow_action_hw_stats_type { + FLOW_ACTION_HW_STATS_TYPE_ANY, +}; + typedef void (*action_destr)(void *priv); struct flow_action_cookie { @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); struct flow_action_entry { enum flow_action_id id; + enum flow_action_hw_stats_type hw_stats_type; action_destr destructor; void *destructor_priv; union { @@ -228,6 +233,7 @@ struct flow_action_entry { }; struct flow_action { + bool mixed_hw_stats_types; unsigned int num_entries; struct flow_action_entry entries[0]; };