[RFC] add flow action context API
Checks
Commit Message
This commit introduces extension of DPDK flow action API enabling
modification of single rte_flow_action.
Motivation and example
===
Adding or removing one or more queues to RSS actions cloned in multiple
flow rules imposes per rule toll for current DPDK flow API; the scenario
requires for each flow sharing cloned RSS action:
- call `rte_flow_destroy()`
- call `rte_flow_create()` with modified RSS action
In order to prevent the overhead of multiple RSS flow rules reconfiguration
API for in-place flow action modification introduced in this commit.
Change description
===
Provide an API to create single rte_flow_action context to point/reference
rte_flow_action object contents from multiple rte_flow_rule objects.
Actually the introduced API makes action object shared and modification
of such an action effects all the rules referencing the action via context
(see struct rte_flow_action_ctx).
Action context lifetime
---
Once action context created (see rte_flow_action_ctx_create()) it can be
safely reused for:
- new flow rule creation
- action configuration/state modification
(see rte_flow_action_ctx_modify())
- action state query (see rte_flow_action_ctx_query())
Once rte_flow_action_ctx_destroy() called the destroyed action context
should not be used i.e. result of the usage undefined.
Action query
---
Provide separate API to query action shared by multiple flows via action
context detached from any specific flow. Taking a counter as an example:
query returns value virtually aggregated across all flow rules referencing
the counter object via action context.
PMD support
---
The support of introduced API is pure PMD specific design and
responsibility for each action type (see struct rte_flow_ops).
testpmd
===
In order to utilize introduced API testpmd cli may implement following
extension create/modify/destroy/query action context iaccordingly
flow action_ctx create {port_id} [index] {action}
flow action_ctx modify {port_id} {index} {action}
flow action_ctx destroy {port_id} {index}
flow action_ctx query {port_id} {index}
example
---
configure rss to queues 1 & 2
testpmd> flow action_ctx create 0 100 rss 1 2
create flow rule utilizing action context
testpmd> flow create 0 ingress \
pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \
actions ctx 100 end / end
add 2 more queues
testpmd> flow action_ctx modify 0 100 rss 1 2 3 4
Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
---
lib/librte_ethdev/rte_ethdev_version.map | 6 +
lib/librte_ethdev/rte_flow.c | 85 ++++++++++++++
lib/librte_ethdev/rte_flow.h | 135 ++++++++++++++++++++++-
lib/librte_ethdev/rte_flow_driver.h | 22 ++++
4 files changed, 247 insertions(+), 1 deletion(-)
Comments
20/05/2020 11:18, Andrey Vesnovaty:
> This commit introduces extension of DPDK flow action API enabling
> modification of single rte_flow_action.
>
> Motivation and example
> ===
> Adding or removing one or more queues to RSS actions cloned in multiple
> flow rules imposes per rule toll for current DPDK flow API; the scenario
> requires for each flow sharing cloned RSS action:
> - call `rte_flow_destroy()`
> - call `rte_flow_create()` with modified RSS action
>
> In order to prevent the overhead of multiple RSS flow rules reconfiguration
> API for in-place flow action modification introduced in this commit.
It seems there is an usability improvement with this new API.
If I understand well, the main motivation is to improve the performance?
The PMD implementation should try to keep a shared object
to benefit of the performance improvement, right?
The existing rte_flow API functions are:
rte_flow_validate()
rte_flow_create()
rte_flow_destroy()
rte_flow_flush()
rte_flow_query()
rte_flow_isolate()
rte_flow_get_aged_flows()
> + # added in 20.08
> + rte_flow_action_ctx_create;
> + rte_flow_action_ctx_destoy;
> + rte_flow_action_ctx_modify;
> + rte_flow_action_ctx_query;
We had "create", "destroy", "query", but no "modify" capability.
The new API is adding 2 things in my opinion:
- shared action object
- "modify" capability (is "update" a better wording?)
About the wording, do we need "ctx"?
I feel rte_flow_action is a good enough prefix for this API,
and should be documented as a shared action object.
I think the word "object" is more meaningful than "context".
Am I missing something?
> + /**
> + * Describes action context.
> + *
> + * Enables multiple rules reference the same action by id/ctx.
> + *
> + * No action specific struct here (void*) since it can be any
> + * action type.
> + */
> + RTE_FLOW_ACTION_TYPE_CTX,
Why do we need a new action type?
> @@ -101,6 +101,28 @@ struct rte_flow_ops {
> + /** See rte_flow_action_ctx_destoy() */
> + void *(*action_ctx_create)
> + (struct rte_eth_dev *dev,
> + const struct rte_flow_action *action,
> + struct rte_flow_error *error);
> + /** See rte_flow_action_ctx_create() */
> + int (*action_ctx_destroy)
> + (struct rte_eth_dev *dev,
> + void *ctx,
> + struct rte_flow_error *error);
> + /** See rte_flow_action_ctx_modify() */
> + int (*action_ctx_modify)
> + (struct rte_eth_dev *dev,
> + void *ctx,
> + const void *action_conf,
> + struct rte_flow_error *error);
> + /** See rte_flow_action_ctx_query() */
> + int (*action_ctx_query)
> + (struct rte_eth_dev *dev,
> + const void *ctx,
> + void *data,
> + struct rte_flow_error *error);
API functions are directly linked to PMD ops, it looks simple and good.
On Wed, May 20, 2020 at 2:48 PM Andrey Vesnovaty
<andrey.vesnovaty@gmail.com> wrote:
>
> This commit introduces extension of DPDK flow action API enabling
> modification of single rte_flow_action.
>
> Motivation and example
> ===
> Adding or removing one or more queues to RSS actions cloned in multiple
> flow rules imposes per rule toll for current DPDK flow API; the scenario
> requires for each flow sharing cloned RSS action:
> - call `rte_flow_destroy()`
> - call `rte_flow_create()` with modified RSS action
>
> In order to prevent the overhead of multiple RSS flow rules reconfiguration
> API for in-place flow action modification introduced in this commit.
>
> Change description
> ===
> Provide an API to create single rte_flow_action context to point/reference
> rte_flow_action object contents from multiple rte_flow_rule objects.
> Actually the introduced API makes action object shared and modification
> of such an action effects all the rules referencing the action via context
> (see struct rte_flow_action_ctx).
>
> Action context lifetime
> ---
> Once action context created (see rte_flow_action_ctx_create()) it can be
> safely reused for:
> - new flow rule creation
> - action configuration/state modification
> (see rte_flow_action_ctx_modify())
> - action state query (see rte_flow_action_ctx_query())
> Once rte_flow_action_ctx_destroy() called the destroyed action context
> should not be used i.e. result of the usage undefined.
Motivation makes sense to me. It will be an improvement.
But creating shared objects adds additional complexity to "PMD and
application" as it
needs to manage the state. Since rte_flow_action already expressed in
vendor natural format, What will be the benefit of a shared context?
Would the following additional API suffice the motivation?
rte_flow_modify_action(struct rte_flow * flow, const struct
rte_flow_action actions[])
On Wed, Jun 3, 2020 at 1:02 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> 20/05/2020 11:18, Andrey Vesnovaty:
> > This commit introduces extension of DPDK flow action API enabling
> > modification of single rte_flow_action.
> >
> > Motivation and example
> > ===
> > Adding or removing one or more queues to RSS actions cloned in multiple
> > flow rules imposes per rule toll for current DPDK flow API; the scenario
> > requires for each flow sharing cloned RSS action:
> > - call `rte_flow_destroy()`
> > - call `rte_flow_create()` with modified RSS action
> >
> > In order to prevent the overhead of multiple RSS flow rules
> reconfiguration
> > API for in-place flow action modification introduced in this commit.
>
> It seems there is an usability improvement with this new API.
> If I understand well, the main motivation is to improve the performance?
> The PMD implementation should try to keep a shared object
> to benefit of the performance improvement, right?
>
> Right, the goal is performance improvement.
Single API call modifies behaviour of multiple flows.
>
> The existing rte_flow API functions are:
> rte_flow_validate()
> rte_flow_create()
> rte_flow_destroy()
> rte_flow_flush()
> rte_flow_query()
> rte_flow_isolate()
> rte_flow_get_aged_flows()
>
> > + # added in 20.08
> > + rte_flow_action_ctx_create;
> > + rte_flow_action_ctx_destoy;
> > + rte_flow_action_ctx_modify;
> > + rte_flow_action_ctx_query;
>
> We had "create", "destroy", "query", but no "modify" capability.
> The new API is adding 2 things in my opinion:
> - shared action object
> - "modify" capability (is "update" a better wording?)
>
Naming is one of the most challenging parts of this RFC.
Some similarity I have found in existing code is
rte_mtr_policer_actions_update()
Is there any existing code having update/modify semantics?
>
> About the wording, do we need "ctx"?
> I feel rte_flow_action is a good enough prefix for this API,
> and should be documented as a shared action object.
> I think the word "object" is more meaningful than "context".
> Am I missing something?
>
> CTX comes for the fact that each flow_rule doesn't have an ownership for
the given action but operates inside some context (shared action context
actually).
As mentioned above, naming is one of the most challenging parts of this
RFC.
>
> > + /**
> > + * Describes action context.
> > + *
> > + * Enables multiple rules reference the same action by id/ctx.
> > + *
> > + * No action specific struct here (void*) since it can be any
> > + * action type.
> > + */
> > + RTE_FLOW_ACTION_TYPE_CTX,
>
> Why do we need a new action type?
>
> Because it's not an action itself but a reference/handle to it.
>
> > @@ -101,6 +101,28 @@ struct rte_flow_ops {
> > + /** See rte_flow_action_ctx_destoy() */
> > + void *(*action_ctx_create)
> > + (struct rte_eth_dev *dev,
> > + const struct rte_flow_action *action,
> > + struct rte_flow_error *error);
> > + /** See rte_flow_action_ctx_create() */
> > + int (*action_ctx_destroy)
> > + (struct rte_eth_dev *dev,
> > + void *ctx,
> > + struct rte_flow_error *error);
> > + /** See rte_flow_action_ctx_modify() */
> > + int (*action_ctx_modify)
> > + (struct rte_eth_dev *dev,
> > + void *ctx,
> > + const void *action_conf,
> > + struct rte_flow_error *error);
> > + /** See rte_flow_action_ctx_query() */
> > + int (*action_ctx_query)
> > + (struct rte_eth_dev *dev,
> > + const void *ctx,
> > + void *data,
> > + struct rte_flow_error *error);
>
> API functions are directly linked to PMD ops, it looks simple and good.
>
>
>
On Wed, Jun 3, 2020 at 1:53 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> On Wed, May 20, 2020 at 2:48 PM Andrey Vesnovaty
> <andrey.vesnovaty@gmail.com> wrote:
> >
> > This commit introduces extension of DPDK flow action API enabling
> > modification of single rte_flow_action.
> >
> > Motivation and example
> > ===
> > Adding or removing one or more queues to RSS actions cloned in multiple
> > flow rules imposes per rule toll for current DPDK flow API; the scenario
> > requires for each flow sharing cloned RSS action:
> > - call `rte_flow_destroy()`
> > - call `rte_flow_create()` with modified RSS action
> >
> > In order to prevent the overhead of multiple RSS flow rules
> reconfiguration
> > API for in-place flow action modification introduced in this commit.
> >
> > Change description
> > ===
> > Provide an API to create single rte_flow_action context to
> point/reference
> > rte_flow_action object contents from multiple rte_flow_rule objects.
> > Actually the introduced API makes action object shared and modification
> > of such an action effects all the rules referencing the action via
> context
> > (see struct rte_flow_action_ctx).
> >
> > Action context lifetime
> > ---
> > Once action context created (see rte_flow_action_ctx_create()) it can be
> > safely reused for:
> > - new flow rule creation
> > - action configuration/state modification
> > (see rte_flow_action_ctx_modify())
> > - action state query (see rte_flow_action_ctx_query())
> > Once rte_flow_action_ctx_destroy() called the destroyed action context
> > should not be used i.e. result of the usage undefined.
>
> Motivation makes sense to me. It will be an improvement.
>
> But creating shared objects adds additional complexity to "PMD and
> application" as it
> needs to manage the state. Since rte_flow_action already expressed in
> vendor natural format, What will be the benefit of a shared context?
>
> The benefit (or goal of the proposed API extension) is to modify the
behaviour of multiple flows by single API call.
> Would the following additional API suffice the motivation?
>
> rte_flow_modify_action(struct rte_flow * flow, const struct
> rte_flow_action actions[])
>
This API limits the scope to single flow which isn't the goal for the
proposed change.
I would suggest adding rte_flow driver implementers if there is a
change in rte_flow_ops in RFC so that
your patch will get enough. I added the maintainers of rte_flow PMD[1]
implementers in Cc.
>>
>> Would the following additional API suffice the motivation?
>>
>> rte_flow_modify_action(struct rte_flow * flow, const struct
>> rte_flow_action actions[])
>
>
> This API limits the scope to single flow which isn't the goal for the proposed change.
Yes. But we need to find the balance between HW features(driver
interface) and public API?
Is Mellanox HW has the support for native shared HW ACTION context?
if so, it makes sense to have such a fat public API as you suggested.
If not, it is a matter of application to iterate over needed flows to
modify action through rte_flow_modify_action().
Assume Mellanox HW has native HW ACTION context and the majority of
the other HW[1] does not
have, then IMO, We should have common code, to handle in this complex
state machine
implementation of action_ctx_create, action_ctx_destroy,
rte_flow_action_ctx_modify, action_ctx_query
in case PMD does not support it. (If PMD/HW supports it then it can
use native implementation)
Reason:
1) I think, All the HW will have the the option to update the ACTION
for the given flow.
octeontx2 has it. If not, let's discuss what is typical HW abstraction
ACTION only update.
2) This case can be implemented if PMD just has flow_modify_action() support.
Multiple flows will be matter will be iterating over all registered flow.
3) Avoid code duplication on all of the below PMDs[1]
[1]
drivers/net/hinic/hinic_pmd_flow.c:const struct rte_flow_ops hinic_flow_ops = {
drivers/net/ipn3ke/ipn3ke_flow.h:extern const struct rte_flow_ops
ipn3ke_flow_ops;
drivers/net/i40e/i40e_flow.c:const struct rte_flow_ops i40e_flow_ops = {
drivers/net/qede/qede_filter.c:const struct rte_flow_ops qede_flow_ops = {
drivers/net/octeontx2/otx2_flow.h:extern const struct rte_flow_ops
otx2_flow_ops;
drivers/net/ice/ice_generic_flow.h:extern const struct rte_flow_ops
ice_flow_ops;
drivers/net/tap/tap_flow.c:static const struct rte_flow_ops tap_flow_ops = {
drivers/net/dpaa2/dpaa2_ethdev.h:extern const struct rte_flow_ops
dpaa2_flow_ops;
drivers/net/e1000/e1000_ethdev.h:extern const struct rte_flow_ops igb_flow_ops;
drivers/net/enic/enic_flow.c:const struct rte_flow_ops enic_flow_ops = {
drivers/net/bonding/rte_eth_bond_flow.c:const struct rte_flow_ops
bond_flow_ops = {
drivers/net/mlx5/mlx5_flow.c:static const struct rte_flow_ops mlx5_flow_ops = {
drivers/net/igc/igc_flow.h:extern const struct rte_flow_ops igc_flow_ops;
drivers/net/cxgbe/cxgbe_flow.c:static const struct rte_flow_ops
cxgbe_flow_ops = {
drivers/net/failsafe/failsafe_private.h:extern const struct
rte_flow_ops fs_flow_ops;
drivers/net/mvpp2/mrvl_flow.c:const struct rte_flow_ops mrvl_flow_ops = {
drivers/net/iavf/iavf_generic_flow.c:const struct rte_flow_ops iavf_flow_ops = {
drivers/net/hns3/hns3_flow.c:static const struct rte_flow_ops hns3_flow_ops = {
drivers/net/bnxt/bnxt_flow.c:const struct rte_flow_ops bnxt_flow_ops = {
drivers/net/mlx4/mlx4_flow.c:static const struct rte_flow_ops mlx4_flow_ops = {
drivers/net/sfc/sfc_flow.c:const struct rte_flow_ops sfc_flow_ops = {
drivers/net/softnic/rte_eth_softnic_flow.c:const struct rte_flow_ops
pmd_flow_ops = {
drivers/net/ixgbe/ixgbe_ethdev.h:extern const struct rte_flow_ops
ixgbe_flow_ops;
On Thu, Jun 4, 2020 at 3:37 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> I would suggest adding rte_flow driver implementers if there is a
> change in rte_flow_ops in RFC so that
> your patch will get enough. I added the maintainers of rte_flow PMD[1]
> implementers in Cc.
>
>
> >>
> >> Would the following additional API suffice the motivation?
> >>
> >> rte_flow_modify_action(struct rte_flow * flow, const struct
> >> rte_flow_action actions[])
> >
> >
> > This API limits the scope to single flow which isn't the goal for the
> proposed change.
>
> Yes. But we need to find the balance between HW features(driver
> interface) and public API?
> Is Mellanox HW has the support for native shared HW ACTION context?
>
Yes, I'm working on a shared context for RSS action, patched will be
available in a couple of weeks.
Other candidates for this kind of API extension are counters/meters.
> if so, it makes sense to have such a fat public API as you suggested.
> If not, it is a matter of application to iterate over needed flows to
> modify action through rte_flow_modify_action().
>
> Assume Mellanox HW has native HW ACTION context and the majority of
> the other HW[1] does not
> have, then IMO, We should have common code, to handle in this complex
> state machine
> implementation of action_ctx_create, action_ctx_destroy,
> rte_flow_action_ctx_modify, action_ctx_query
> in case PMD does not support it. (If PMD/HW supports it then it can
> use native implementation)
>
Does it mean that all PMDs will support action-ctx create/destroy/update
for all types of actions?
Reason:
> 1) I think, All the HW will have the the option to update the ACTION
> for the given flow.
> octeontx2 has it. If not, let's discuss what is typical HW abstraction
> ACTION only update.
>
> 2) This case can be implemented if PMD just has flow_modify_action()
> support.
> Multiple flows will be matter will be iterating over all registered flow.
>
General case won't be just iteration over all flows but:
1. destroy flow
2. create "modified" action
3. create flow with the action from (2)
Is this what you mean by "common code" to handle action-ctx
create/destroy/update implementation?
>
> 3) Avoid code duplication on all of the below PMDs[1]
>
>
>
> [1]
> drivers/net/hinic/hinic_pmd_flow.c:const struct rte_flow_ops
> hinic_flow_ops = {
> drivers/net/ipn3ke/ipn3ke_flow.h:extern const struct rte_flow_ops
> ipn3ke_flow_ops;
> drivers/net/i40e/i40e_flow.c:const struct rte_flow_ops i40e_flow_ops = {
> drivers/net/qede/qede_filter.c:const struct rte_flow_ops qede_flow_ops = {
> drivers/net/octeontx2/otx2_flow.h:extern const struct rte_flow_ops
> otx2_flow_ops;
> drivers/net/ice/ice_generic_flow.h:extern const struct rte_flow_ops
> ice_flow_ops;
> drivers/net/tap/tap_flow.c:static const struct rte_flow_ops tap_flow_ops =
> {
> drivers/net/dpaa2/dpaa2_ethdev.h:extern const struct rte_flow_ops
> dpaa2_flow_ops;
> drivers/net/e1000/e1000_ethdev.h:extern const struct rte_flow_ops
> igb_flow_ops;
> drivers/net/enic/enic_flow.c:const struct rte_flow_ops enic_flow_ops = {
> drivers/net/bonding/rte_eth_bond_flow.c:const struct rte_flow_ops
> bond_flow_ops = {
> drivers/net/mlx5/mlx5_flow.c:static const struct rte_flow_ops
> mlx5_flow_ops = {
> drivers/net/igc/igc_flow.h:extern const struct rte_flow_ops igc_flow_ops;
> drivers/net/cxgbe/cxgbe_flow.c:static const struct rte_flow_ops
> cxgbe_flow_ops = {
> drivers/net/failsafe/failsafe_private.h:extern const struct
> rte_flow_ops fs_flow_ops;
> drivers/net/mvpp2/mrvl_flow.c:const struct rte_flow_ops mrvl_flow_ops = {
> drivers/net/iavf/iavf_generic_flow.c:const struct rte_flow_ops
> iavf_flow_ops = {
> drivers/net/hns3/hns3_flow.c:static const struct rte_flow_ops
> hns3_flow_ops = {
> drivers/net/bnxt/bnxt_flow.c:const struct rte_flow_ops bnxt_flow_ops = {
> drivers/net/mlx4/mlx4_flow.c:static const struct rte_flow_ops
> mlx4_flow_ops = {
> drivers/net/sfc/sfc_flow.c:const struct rte_flow_ops sfc_flow_ops = {
> drivers/net/softnic/rte_eth_softnic_flow.c:const struct rte_flow_ops
> pmd_flow_ops = {
> drivers/net/ixgbe/ixgbe_ethdev.h:extern const struct rte_flow_ops
> ixgbe_flow_ops;
>
04/06/2020 13:12, Andrey Vesnovaty:
> Thomas Monjalon <thomas@monjalon.net> wrote:
> > 20/05/2020 11:18, Andrey Vesnovaty:
> > We had "create", "destroy", "query", but no "modify" capability.
> > The new API is adding 2 things in my opinion:
> > - shared action object
> > - "modify" capability (is "update" a better wording?)
>
> Naming is one of the most challenging parts of this RFC.
> Some similarity I have found in existing code is
> rte_mtr_policer_actions_update()
> Is there any existing code having update/modify semantics?
Except one callback in librte_fib, no DPDK API has "modify" in its name.
You can find the word "update" in the API of multiple DPDK libs.
I would like having the opinion of a native english speaker here.
> > About the wording, do we need "ctx"?
> > I feel rte_flow_action is a good enough prefix for this API,
> > and should be documented as a shared action object.
> > I think the word "object" is more meaningful than "context".
> > Am I missing something?
>
> CTX comes for the fact that each flow_rule doesn't have an ownership for
> the given action but operates inside some context (shared action context
> actually).
> As mentioned above, naming is one of the most challenging parts of this
> RFC.
I think we can replace "context" with "object" in the explanations.
The functions could be named without "ctx":
- rte_flow_action_create
- rte_flow_action_destroy
- rte_flow_action_update
- rte_flow_action_query
> > > + /**
> > > + * Describes action context.
> > > + *
> > > + * Enables multiple rules reference the same action by id/ctx.
> > > + *
> > > + * No action specific struct here (void*) since it can be any
> > > + * action type.
> > > + */
> > > + RTE_FLOW_ACTION_TYPE_CTX,
> >
> > Why do we need a new action type?
> >
> Because it's not an action itself but a reference/handle to it.
Sorry I don't understand when RTE_FLOW_ACTION_TYPE_CTX has to be used.
There is no mention of RTE_FLOW_ACTION_TYPE_CTX in the explanations.
On Thu, Jun 04, 2020 at 07:23:04PM +0200, Thomas Monjalon wrote:
> 04/06/2020 13:12, Andrey Vesnovaty:
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> > > 20/05/2020 11:18, Andrey Vesnovaty:
> > > We had "create", "destroy", "query", but no "modify" capability.
> > > The new API is adding 2 things in my opinion:
> > > - shared action object
> > > - "modify" capability (is "update" a better wording?)
> >
> > Naming is one of the most challenging parts of this RFC.
> > Some similarity I have found in existing code is
> > rte_mtr_policer_actions_update()
> > Is there any existing code having update/modify semantics?
>
> Except one callback in librte_fib, no DPDK API has "modify" in its name.
> You can find the word "update" in the API of multiple DPDK libs.
> I would like having the opinion of a native english speaker here.
>
From a language viewpoint either is fine for conveying what the API does.
In this case "update" would seem to be better for consistency reasons.
05/06/2020 10:30, Bruce Richardson:
> On Thu, Jun 04, 2020 at 07:23:04PM +0200, Thomas Monjalon wrote:
> > 04/06/2020 13:12, Andrey Vesnovaty:
> > > Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > 20/05/2020 11:18, Andrey Vesnovaty:
> > > > We had "create", "destroy", "query", but no "modify" capability.
> > > > The new API is adding 2 things in my opinion:
> > > > - shared action object
> > > > - "modify" capability (is "update" a better wording?)
> > >
> > > Naming is one of the most challenging parts of this RFC.
> > > Some similarity I have found in existing code is
> > > rte_mtr_policer_actions_update()
> > > Is there any existing code having update/modify semantics?
> >
> > Except one callback in librte_fib, no DPDK API has "modify" in its name.
> > You can find the word "update" in the API of multiple DPDK libs.
> > I would like having the opinion of a native english speaker here.
> >
> From a language viewpoint either is fine for conveying what the API does.
> In this case "update" would seem to be better for consistency reasons.
OK, thank you Bruce
On Thu, Jun 4, 2020 at 9:27 PM Andrey Vesnovaty
<andrey.vesnovaty@gmail.com> wrote:
>
> On Thu, Jun 4, 2020 at 3:37 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>>
>> I would suggest adding rte_flow driver implementers if there is a
>> change in rte_flow_ops in RFC so that
>> your patch will get enough. I added the maintainers of rte_flow PMD[1]
>> implementers in Cc.
>>
>>
>> >>
>> >> Would the following additional API suffice the motivation?
>> >>
>> >> rte_flow_modify_action(struct rte_flow * flow, const struct
>> >> rte_flow_action actions[])
>> >
>> >
>> > This API limits the scope to single flow which isn't the goal for the proposed change.
>>
>> Yes. But we need to find the balance between HW features(driver
>> interface) and public API?
>> Is Mellanox HW has the support for native shared HW ACTION context?
>
>
> Yes, I'm working on a shared context for RSS action, patched will be available in a couple of weeks.
> Other candidates for this kind of API extension are counters/meters.
OK.
>
>>
>> if so, it makes sense to have such a fat public API as you suggested.
>> If not, it is a matter of application to iterate over needed flows to
>> modify action through rte_flow_modify_action().
>>
>> Assume Mellanox HW has native HW ACTION context and the majority of
>> the other HW[1] does not
>> have, then IMO, We should have common code, to handle in this complex
>> state machine
>> implementation of action_ctx_create, action_ctx_destroy,
>> rte_flow_action_ctx_modify, action_ctx_query
>> in case PMD does not support it. (If PMD/HW supports it then it can
>> use native implementation)
>
>
> Does it mean that all PMDs will support action-ctx create/destroy/update for all types of actions?
That can be based on following _driver_ op interface return:
flow_modify_action(struct rte_flow * flow, const struct
rte_flow_action actions[])
>
>> Reason:
>> 1) I think, All the HW will have the the option to update the ACTION
>> for the given flow.
>> octeontx2 has it. If not, let's discuss what is typical HW abstraction
>> ACTION only update.
>
>
>>
>> 2) This case can be implemented if PMD just has flow_modify_action() support.
>> Multiple flows will be matter will be iterating over all registered flow.
>
>
> General case won't be just iteration over all flows but:
> 1. destroy flow
> 2. create "modified" action
> 3. create flow with the action from (2)
> Is this what you mean by "common code" to handle action-ctx create/destroy/update implementation?
Yes. Not sure why to destroy the flow if the only action is getting updated.
(2) and (3) driver op can be just implemented over,
flow_modify_action(struct rte_flow * flow, const struct
rte_flow_action actions[])
>
>>
>> 3) Avoid code duplication on all of the below PMDs[1]
>>
>>
>>
>> [1]
>> drivers/net/hinic/hinic_pmd_flow.c:const struct rte_flow_ops hinic_flow_ops = {
>> drivers/net/ipn3ke/ipn3ke_flow.h:extern const struct rte_flow_ops
>> ipn3ke_flow_ops;
>> drivers/net/i40e/i40e_flow.c:const struct rte_flow_ops i40e_flow_ops = {
>> drivers/net/qede/qede_filter.c:const struct rte_flow_ops qede_flow_ops = {
>> drivers/net/octeontx2/otx2_flow.h:extern const struct rte_flow_ops
>> otx2_flow_ops;
>> drivers/net/ice/ice_generic_flow.h:extern const struct rte_flow_ops
>> ice_flow_ops;
>> drivers/net/tap/tap_flow.c:static const struct rte_flow_ops tap_flow_ops = {
>> drivers/net/dpaa2/dpaa2_ethdev.h:extern const struct rte_flow_ops
>> dpaa2_flow_ops;
>> drivers/net/e1000/e1000_ethdev.h:extern const struct rte_flow_ops igb_flow_ops;
>> drivers/net/enic/enic_flow.c:const struct rte_flow_ops enic_flow_ops = {
>> drivers/net/bonding/rte_eth_bond_flow.c:const struct rte_flow_ops
>> bond_flow_ops = {
>> drivers/net/mlx5/mlx5_flow.c:static const struct rte_flow_ops mlx5_flow_ops = {
>> drivers/net/igc/igc_flow.h:extern const struct rte_flow_ops igc_flow_ops;
>> drivers/net/cxgbe/cxgbe_flow.c:static const struct rte_flow_ops
>> cxgbe_flow_ops = {
>> drivers/net/failsafe/failsafe_private.h:extern const struct
>> rte_flow_ops fs_flow_ops;
>> drivers/net/mvpp2/mrvl_flow.c:const struct rte_flow_ops mrvl_flow_ops = {
>> drivers/net/iavf/iavf_generic_flow.c:const struct rte_flow_ops iavf_flow_ops = {
>> drivers/net/hns3/hns3_flow.c:static const struct rte_flow_ops hns3_flow_ops = {
>> drivers/net/bnxt/bnxt_flow.c:const struct rte_flow_ops bnxt_flow_ops = {
>> drivers/net/mlx4/mlx4_flow.c:static const struct rte_flow_ops mlx4_flow_ops = {
>> drivers/net/sfc/sfc_flow.c:const struct rte_flow_ops sfc_flow_ops = {
>> drivers/net/softnic/rte_eth_softnic_flow.c:const struct rte_flow_ops
>> pmd_flow_ops = {
>> drivers/net/ixgbe/ixgbe_ethdev.h:extern const struct rte_flow_ops
>> ixgbe_flow_ops;
@@ -230,4 +230,10 @@ EXPERIMENTAL {
# added in 20.02
rte_flow_dev_dump;
+
+ # added in 20.08
+ rte_flow_action_ctx_create;
+ rte_flow_action_ctx_destoy;
+ rte_flow_action_ctx_modify;
+ rte_flow_action_ctx_query;
};
@@ -1231,3 +1231,88 @@ rte_flow_dev_dump(uint16_t port_id, FILE *file, struct rte_flow_error *error)
RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
NULL, rte_strerror(ENOSYS));
}
+
+void *
+rte_flow_action_ctx_create(uint16_t port_id,
+ const struct rte_flow_action *action,
+ struct rte_flow_error *error)
+{
+ struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+ void *ctx;
+ const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
+
+ if (unlikely(!ops))
+ return NULL;
+ if (likely(!!ops->action_ctx_create)) {
+ ctx = ops->action_ctx_create(dev, action, error);
+ if (ctx == NULL)
+ flow_err(port_id, -rte_errno, error);
+ return ctx;
+ }
+ rte_flow_error_set(error, ENOSYS, RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+ NULL, rte_strerror(ENOSYS));
+ return NULL;
+}
+
+int
+rte_flow_action_ctx_destoy(uint16_t port_id,
+ void *ctx,
+ struct rte_flow_error *error)
+{
+ (void)(port_id);
+ (void)(ctx);
+ (void)(error);
+
+ struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+ const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
+
+ if (unlikely(!ops))
+ return -rte_errno;
+ if (likely(!!ops->action_ctx_destroy))
+ return flow_err(port_id,
+ ops->action_ctx_destroy(dev, ctx, error),
+ error);
+ return rte_flow_error_set(error, ENOSYS,
+ RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+ NULL, rte_strerror(ENOSYS));
+}
+
+int
+rte_flow_action_ctx_modify(uint16_t port_id,
+ void *ctx,
+ const void *action_conf,
+ struct rte_flow_error *error)
+{
+ struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+ const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
+
+ if (unlikely(!ops))
+ return -rte_errno;
+ if (likely(!!ops->action_ctx_modify))
+ return flow_err(port_id, ops->action_ctx_modify(dev, ctx,
+ action_conf, error),
+ error);
+ return rte_flow_error_set(error, ENOSYS,
+ RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+ NULL, rte_strerror(ENOSYS));
+}
+
+int
+rte_flow_action_ctx_query(uint16_t port_id,
+ const void *ctx,
+ void *data,
+ struct rte_flow_error *error)
+{
+ struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+ const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
+
+ if (unlikely(!ops))
+ return -rte_errno;
+ if (likely(!!ops->action_ctx_query))
+ return flow_err(port_id, ops->action_ctx_query(dev, ctx,
+ data, error),
+ error);
+ return rte_flow_error_set(error, ENOSYS,
+ RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+ NULL, rte_strerror(ENOSYS));
+}
@@ -1643,7 +1643,8 @@ enum rte_flow_action_type {
/**
* Enables counters for this flow rule.
*
- * These counters can be retrieved and reset through rte_flow_query(),
+ * These counters can be retrieved and reset through rte_flow_query() or
+ * rte_flow_action_ctx_query() if the action referenced via context/id,
* see struct rte_flow_query_count.
*
* See struct rte_flow_action_count.
@@ -2051,6 +2052,16 @@ enum rte_flow_action_type {
* See struct rte_flow_action_set_dscp.
*/
RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP,
+
+ /**
+ * Describes action context.
+ *
+ * Enables multiple rules reference the same action by id/ctx.
+ *
+ * No action specific struct here (void*) since it can be any
+ * action type.
+ */
+ RTE_FLOW_ACTION_TYPE_CTX,
};
/**
@@ -3224,6 +3235,128 @@ rte_flow_conv(enum rte_flow_conv_op op,
const void *src,
struct rte_flow_error *error);
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Create the action context pointing to the action via id/ctx.
+ *
+ * @param[in] port_id
+ * The port identifier of the Ethernet device.
+ * @param[in] action
+ * Action to be pointed via id/ctx.
+ * @param[out] error
+ * Perform verbose error reporting if not NULL. PMDs initialize this
+ * structure in case of error only.
+ * @return
+ * A valid handle in case of success, NULL otherwise and rte_errno is set
+ * to one of the error codes defined:
+ * - (ENOSYS) if underlying device does not support this functionality.
+ * - (EIO) if underlying device is removed.
+ * - (EINVAL) if *action* invalid.
+ * - (ENOTSUP) if *action* valid but unsupported.
+ */
+__rte_experimental
+void *
+rte_flow_action_ctx_create(uint16_t port_id,
+ const struct rte_flow_action *action,
+ struct rte_flow_error *error);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Destroys the action pointed by action context.
+ *
+ * @param[in] port_id
+ * The port identifier of the Ethernet device.
+ * @param[in] ctx
+ * Describes id/ctx pinting to the action to be destroyed.
+ * @param[out] error
+ * Perform verbose error reporting if not NULL. PMDs initialize this
+ * structure in case of error only.
+ * @return
+ * - (0) if success.
+ * - (-ENOSYS) if underlying device does not support this functionality.
+ * - (-EIO) if underlying device is removed.
+ * - (-ENOENT) if action pointed by *ctx* was not found.
+ * - (-ETOOMANYREFS) if action pointed by *ctx* still referenced by one or
+ * more rules
+ * rte_errno is also set.
+ */
+__rte_experimental
+int
+rte_flow_action_ctx_destoy(uint16_t port_id,
+ void *ctx,
+ struct rte_flow_error *error);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Modifies inplace the action configuration pointed by action context
+ * created via rte_flow_action_ctx_create().
+ *
+ * @param[in] port_id
+ * The port identifier of the Ethernet device.
+ * @param[in] ctx
+ * Action ctx pointing to the action to be modified.
+ * @param[in] action_conf
+ * Action specification used to modify the action pointed by ctx.
+ * action_conf should be of same type with the action pointed by ctx,
+ * otherwise function behavior undefined.
+ * @param[out] error
+ * Perform verbose error reporting if not NULL. PMDs initialize this
+ * structure in case of error only.
+ * @return
+ * - (0) if success.
+ * - (-ENOSYS) if underlying device does not support this functionality.
+ * - (-EIO) if underlying device is removed.
+ * - (-EINVAL) if *action_conf* invalid.
+ * - (-ENOTSUP) if *action_conf* valid but unsupported.
+ * - (-ENOENT) if action pointed by *ctx* was not found.
+ * rte_errno is also set.
+ */
+__rte_experimental
+int
+rte_flow_action_ctx_modify(uint16_t port_id,
+ void *ctx,
+ const void *action_conf,
+ struct rte_flow_error *error);
+
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Query an existing action referenced via id/context.
+ *
+ * This function allows retrieving action-specific data such as counters.
+ * Data is gathered by special action which may be present/referenced in
+ * more than one flow rule definition.
+ *
+ * \see RTE_FLOW_ACTION_TYPE_COUNT
+ *
+ * @param port_id
+ * Port identifier of Ethernet device.
+ * @param[in] ctx
+ * Action ctx pointing to the action to query.
+ * @param[in, out] data
+ * Pointer to storage for the associated query data type.
+ * @param[out] error
+ * Perform verbose error reporting if not NULL. PMDs initialize this
+ * structure in case of error only.
+ *
+ * @return
+ * 0 on success, a negative errno value otherwise and rte_errno is set.
+ */
+__rte_experimental
+int
+rte_flow_action_ctx_query(uint16_t port_id,
+ const void *ctx,
+ void *data,
+ struct rte_flow_error *error);
+
#ifdef __cplusplus
}
#endif
@@ -101,6 +101,28 @@ struct rte_flow_ops {
(struct rte_eth_dev *dev,
FILE *file,
struct rte_flow_error *error);
+ /** See rte_flow_action_ctx_destoy() */
+ void *(*action_ctx_create)
+ (struct rte_eth_dev *dev,
+ const struct rte_flow_action *action,
+ struct rte_flow_error *error);
+ /** See rte_flow_action_ctx_create() */
+ int (*action_ctx_destroy)
+ (struct rte_eth_dev *dev,
+ void *ctx,
+ struct rte_flow_error *error);
+ /** See rte_flow_action_ctx_modify() */
+ int (*action_ctx_modify)
+ (struct rte_eth_dev *dev,
+ void *ctx,
+ const void *action_conf,
+ struct rte_flow_error *error);
+ /** See rte_flow_action_ctx_query() */
+ int (*action_ctx_query)
+ (struct rte_eth_dev *dev,
+ const void *ctx,
+ void *data,
+ struct rte_flow_error *error);
};
/**