diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst index a237e4fd29..80360d0688 100644 --- a/doc/guides/prog_guide/rte_flow.rst +++ b/doc/guides/prog_guide/rte_flow.rst @@ -995,28 +995,27 @@ Actions Each possible action is represented by a type. Some have associated configuration structures. Several actions combined in a list can be assigned -to a flow rule. That list is not ordered. +to a flow rule and are performed in order. They fall in three categories: -- Terminating actions that prevent processing matched packets by subsequent - flow rules, unless overridden with PASSTHRU. +- Actions that modify the fate of matching traffic, for instance by dropping + or assigning it a specific destination. -- Non-terminating actions that leave matched packets up for additional - processing by subsequent flow rules. +- Actions that modify matching traffic contents or its properties. This + includes adding/removing encapsulation, encryption, compression and marks. -- Other non-terminating meta actions that do not affect the fate of packets. +- Actions related to the flow rule itself, such as updating counters or + making it non-terminating. -When several actions are combined in a flow rule, they should all have -different types (e.g. dropping a packet twice is not possible). +Flow rules being terminating by default, not specifying any action of the +fate kind results in undefined behavior. This applies to both ingress and +egress. -Only the last action of a given type is taken into account. PMDs still -perform error checking on the entire list. +PASSTHRU, when supported, makes a flow rule non-terminating. Like matching patterns, action lists are terminated by END items. -*Note that PASSTHRU is the only action able to override a terminating rule.* - Example of action that redirects packets to queue index 10: .. _table_rte_flow_action_example: @@ -1029,12 +1028,11 @@ Example of action that redirects packets to queue index 10: | ``index`` | 10 | +-----------+-------+ -Action lists examples, their order is not significant, applications must -consider all actions to be performed simultaneously: +Actions are performed in list order: -.. _table_rte_flow_count_and_drop: +.. _table_rte_flow_count_then_drop: -.. table:: Count and drop +.. table:: Count then drop +-------+--------+ | Index | Action | @@ -1050,7 +1048,7 @@ consider all actions to be performed simultaneously: .. _table_rte_flow_mark_count_redirect: -.. table:: Mark, count and redirect +.. table:: Mark, count then redirect +-------+--------+-----------+-------+ | Index | Action | Field | Value | @@ -1080,12 +1078,15 @@ consider all actions to be performed simultaneously: | 2 | END | +-------+----------------------------+ -In the above example, considering both actions are performed simultaneously, -the end result is that only QUEUE has any effect. +In the above example, while DROP and QUEUE must be performed in order, both +have to happen before reaching END. Only QUEUE has a visible effect. -.. _table_rte_flow_redirect_queue_3: +Note that such a list may be thought as ambiguous and rejected on that +basis. -.. table:: Redirect to queue 3 +.. _table_rte_flow_redirect_queue_5_3: + +.. table:: Redirect to queues 5 and 3 +-------+--------+-----------+-------+ | Index | Action | Field | Value | @@ -1099,9 +1100,9 @@ the end result is that only QUEUE has any effect. | 3 | END | +-------+----------------------------+ -As previously described, only the last action of a given type found in the -list is taken into account. The above example also shows that VOID is -ignored. +As previously described, all actions must be taken into account. This +effectively duplicates traffic to both queues. The above example also shows +that VOID is ignored. Action types ~~~~~~~~~~~~ @@ -1151,9 +1152,8 @@ PMDs. Action: ``PASSTHRU`` ^^^^^^^^^^^^^^^^^^^^ -Leaves packets up for additional processing by subsequent flow rules. This -is the default when a rule does not contain a terminating action, but can be -specified to force a rule to become non-terminating. +Leaves traffic up for additional processing by subsequent flow rules; makes +a flow rule non-terminating. - No configurable properties. @@ -1227,8 +1227,6 @@ Action: ``QUEUE`` Assigns packets to a given queue index. -- Terminating by default. - .. _table_rte_flow_action_queue: .. table:: QUEUE @@ -1245,8 +1243,6 @@ Action: ``DROP`` Drop packets. - No configurable properties. -- Terminating by default. -- PASSTHRU overrides this action if both are specified. .. _table_rte_flow_action_drop: @@ -1309,8 +1305,6 @@ Note: RSS hash result is stored in the ``hash.rss`` mbuf field which overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi`` field only, both can be requested simultaneously. -- Terminating by default. - .. _table_rte_flow_action_rss: .. table:: RSS @@ -1331,7 +1325,6 @@ Action: ``PF`` Redirects packets to the physical function (PF) of the current device. - No configurable properties. -- Terminating by default. .. _table_rte_flow_action_pf: @@ -1353,8 +1346,6 @@ ID instead of the specified one. This parameter may not be available and is not guaranteed to work properly if the VF part is matched by a prior flow rule or if packets are not addressed to a VF in the first place. -- Terminating by default. - .. _table_rte_flow_action_vf: .. table:: VF @@ -1378,8 +1369,6 @@ action parameter. More than one flow can use the same MTR object through the meter action. The MTR object can be further updated or queried using the rte_mtr* API. -- Non-terminating by default. - .. _table_rte_flow_action_meter: .. table:: METER @@ -1415,8 +1404,6 @@ direction. Multiple flows can be configured to use the same security session. -- Non-terminating by default. - .. _table_rte_flow_action_security: .. table:: SECURITY diff --git a/doc/guides/rel_notes/release_18_05.rst b/doc/guides/rel_notes/release_18_05.rst index e6865c94ff..4deb44c49c 100644 --- a/doc/guides/rel_notes/release_18_05.rst +++ b/doc/guides/rel_notes/release_18_05.rst @@ -258,7 +258,15 @@ API Changes fall-back value. Previously, setting ``nb_tx_desc`` to zero would have resulted in an error. -* ethdev: unused DUP action was removed from the flow API. +* ethdev: several changes were made to the flow API. + + * Unused DUP action was removed. + * Actions semantics in flow rules: list order now matters ("first + to last" instead of "all simultaneously"), repeated actions are now + all performed, and they do not individually have (non-)terminating + properties anymore. + * Flow rules are now always terminating unless a PASSTHRU action is + present. ABI Changes @@ -305,8 +313,9 @@ ABI Changes This includes functions ``rte_flow_copy``, ``rte_flow_create``, ``rte_flow_destroy``, ``rte_flow_error_set``, ``rte_flow_flush``, ``rte_flow_isolate``, ``rte_flow_query`` and ``rte_flow_validate``, due to - changes in error type definitions (``enum rte_flow_error_type``) and - removal of the unused DUP action (``enum rte_flow_action_type``). + changes in error type definitions (``enum rte_flow_error_type``), removal + of the unused DUP action (``enum rte_flow_action_type``) and modified + behavior for flow rule actions (see API changes). Removed Items diff --git a/drivers/net/enic/enic_flow.c b/drivers/net/enic/enic_flow.c index b9f36587c1..c34ae84d17 100644 --- a/drivers/net/enic/enic_flow.c +++ b/drivers/net/enic/enic_flow.c @@ -3,6 +3,7 @@ */ #include +#include #include #include #include @@ -964,6 +965,9 @@ static int enic_copy_action_v1(const struct rte_flow_action actions[], struct filter_action_v2 *enic_action) { + enum { FATE = 1, }; + uint32_t overlap = 0; + FLOW_TRACE(); for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { @@ -975,6 +979,10 @@ enic_copy_action_v1(const struct rte_flow_action actions[], const struct rte_flow_action_queue *queue = (const struct rte_flow_action_queue *) actions->conf; + + if (overlap & FATE) + return ENOTSUP; + overlap |= FATE; enic_action->rq_idx = enic_rte_rq_idx_to_sop_idx(queue->index); break; @@ -984,6 +992,8 @@ enic_copy_action_v1(const struct rte_flow_action actions[], break; } } + if (!(overlap & FATE)) + return ENOTSUP; enic_action->type = FILTER_ACTION_RQ_STEERING; return 0; } @@ -1001,6 +1011,9 @@ static int enic_copy_action_v2(const struct rte_flow_action actions[], struct filter_action_v2 *enic_action) { + enum { FATE = 1, MARK = 2, }; + uint32_t overlap = 0; + FLOW_TRACE(); for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { @@ -1009,6 +1022,10 @@ enic_copy_action_v2(const struct rte_flow_action actions[], const struct rte_flow_action_queue *queue = (const struct rte_flow_action_queue *) actions->conf; + + if (overlap & FATE) + return ENOTSUP; + overlap |= FATE; enic_action->rq_idx = enic_rte_rq_idx_to_sop_idx(queue->index); enic_action->flags |= FILTER_ACTION_RQ_STEERING_FLAG; @@ -1019,6 +1036,9 @@ enic_copy_action_v2(const struct rte_flow_action actions[], (const struct rte_flow_action_mark *) actions->conf; + if (overlap & MARK) + return ENOTSUP; + overlap |= MARK; /* ENIC_MAGIC_FILTER_ID is reserved and is the highest * in the range of allows mark ids. */ @@ -1029,6 +1049,9 @@ enic_copy_action_v2(const struct rte_flow_action actions[], break; } case RTE_FLOW_ACTION_TYPE_FLAG: { + if (overlap & MARK) + return ENOTSUP; + overlap |= MARK; enic_action->filter_id = ENIC_MAGIC_FILTER_ID; enic_action->flags |= FILTER_ACTION_FILTER_ID_FLAG; break; @@ -1044,6 +1067,8 @@ enic_copy_action_v2(const struct rte_flow_action actions[], break; } } + if (!(overlap & FATE)) + return ENOTSUP; enic_action->type = FILTER_ACTION_V2; return 0; } diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c index 67fd568bcd..15cdf07b7b 100644 --- a/drivers/net/mlx4/mlx4_flow.c +++ b/drivers/net/mlx4/mlx4_flow.c @@ -637,6 +637,7 @@ mlx4_flow_prepare(struct priv *priv, struct rte_flow temp = { .ibv_attr_size = sizeof(*temp.ibv_attr) }; struct rte_flow *flow = &temp; const char *msg = NULL; + int overlap; if (attr->group) return rte_flow_error_set @@ -656,6 +657,7 @@ mlx4_flow_prepare(struct priv *priv, (error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ATTR_INGRESS, NULL, "only ingress is supported"); fill: + overlap = 0; proc = mlx4_flow_proc_item_list; /* Go over pattern. */ for (item = pattern; item->type; ++item) { @@ -702,6 +704,16 @@ mlx4_flow_prepare(struct priv *priv, } /* Go over actions list. */ for (action = actions; action->type; ++action) { + /* This one may appear anywhere multiple times. */ + if (action->type == RTE_FLOW_ACTION_TYPE_VOID) + continue; + /* Fate-deciding actions may appear exactly once. */ + if (overlap) { + msg = "cannot combine several fate-deciding actions," + " choose between DROP, QUEUE or RSS"; + goto exit_action_not_supported; + } + overlap = 1; switch (action->type) { const struct rte_flow_action_queue *queue; const struct rte_flow_action_rss *rss; @@ -709,8 +721,6 @@ mlx4_flow_prepare(struct priv *priv, uint64_t fields; unsigned int i; - case RTE_FLOW_ACTION_TYPE_VOID: - continue; case RTE_FLOW_ACTION_TYPE_DROP: flow->drop = 1; break; @@ -801,10 +811,9 @@ mlx4_flow_prepare(struct priv *priv, goto exit_action_not_supported; } } - if (!flow->rss && !flow->drop) - return rte_flow_error_set - (error, ENOTSUP, RTE_FLOW_ERROR_TYPE_UNSPECIFIED, - NULL, "no valid action"); + /* When fate is unknown, drop traffic. */ + if (!overlap) + flow->drop = 1; /* Validation ends here. */ if (!addr) { if (flow->rss) diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c index 1807e3f5b7..586e780c46 100644 --- a/drivers/net/mlx5/mlx5_flow.c +++ b/drivers/net/mlx5/mlx5_flow.c @@ -4,6 +4,7 @@ */ #include +#include #include /* Verbs header. */ @@ -646,6 +647,8 @@ mlx5_flow_convert_actions(struct rte_eth_dev *dev, struct rte_flow_error *error, struct mlx5_flow_parse *parser) { + enum { FATE = 1, MARK = 2, COUNT = 4, }; + uint32_t overlap = 0; struct priv *priv = dev->data->dev_private; int ret; @@ -662,39 +665,31 @@ mlx5_flow_convert_actions(struct rte_eth_dev *dev, if (actions->type == RTE_FLOW_ACTION_TYPE_VOID) { continue; } else if (actions->type == RTE_FLOW_ACTION_TYPE_DROP) { + if (overlap & FATE) + goto exit_action_overlap; + overlap |= FATE; parser->drop = 1; } else if (actions->type == RTE_FLOW_ACTION_TYPE_QUEUE) { const struct rte_flow_action_queue *queue = (const struct rte_flow_action_queue *) actions->conf; - uint16_t n; - uint16_t found = 0; + if (overlap & FATE) + goto exit_action_overlap; + overlap |= FATE; if (!queue || (queue->index > (priv->rxqs_n - 1))) goto exit_action_not_supported; - for (n = 0; n < parser->queues_n; ++n) { - if (parser->queues[n] == queue->index) { - found = 1; - break; - } - } - if (parser->queues_n > 1 && !found) { - rte_flow_error_set(error, ENOTSUP, - RTE_FLOW_ERROR_TYPE_ACTION, - actions, - "queue action not in RSS queues"); - return -rte_errno; - } - if (!found) { - parser->queues_n = 1; - parser->queues[0] = queue->index; - } + parser->queues_n = 1; + parser->queues[0] = queue->index; } else if (actions->type == RTE_FLOW_ACTION_TYPE_RSS) { const struct rte_flow_action_rss *rss = (const struct rte_flow_action_rss *) actions->conf; uint16_t n; + if (overlap & FATE) + goto exit_action_overlap; + overlap |= FATE; if (!rss || !rss->num) { rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_ACTION, @@ -702,26 +697,6 @@ mlx5_flow_convert_actions(struct rte_eth_dev *dev, "no valid queues"); return -rte_errno; } - if (parser->queues_n == 1) { - uint16_t found = 0; - - assert(parser->queues_n); - for (n = 0; n < rss->num; ++n) { - if (parser->queues[0] == - rss->queue[n]) { - found = 1; - break; - } - } - if (!found) { - rte_flow_error_set(error, ENOTSUP, - RTE_FLOW_ERROR_TYPE_ACTION, - actions, - "queue action not in RSS" - " queues"); - return -rte_errno; - } - } if (rss->num > RTE_DIM(parser->queues)) { rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_ACTION, @@ -755,6 +730,9 @@ mlx5_flow_convert_actions(struct rte_eth_dev *dev, (const struct rte_flow_action_mark *) actions->conf; + if (overlap & MARK) + goto exit_action_overlap; + overlap |= MARK; if (!mark) { rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_ACTION, @@ -772,14 +750,23 @@ mlx5_flow_convert_actions(struct rte_eth_dev *dev, parser->mark = 1; parser->mark_id = mark->id; } else if (actions->type == RTE_FLOW_ACTION_TYPE_FLAG) { + if (overlap & MARK) + goto exit_action_overlap; + overlap |= MARK; parser->mark = 1; } else if (actions->type == RTE_FLOW_ACTION_TYPE_COUNT && priv->config.flow_counter_en) { + if (overlap & COUNT) + goto exit_action_overlap; + overlap |= COUNT; parser->count = 1; } else { goto exit_action_not_supported; } } + /* When fate is unknown, drop traffic. */ + if (!(overlap & FATE)) + parser->drop = 1; if (parser->drop && parser->mark) parser->mark = 0; if (!parser->queues_n && !parser->drop) { @@ -792,6 +779,10 @@ mlx5_flow_convert_actions(struct rte_eth_dev *dev, rte_flow_error_set(error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ACTION, actions, "action not supported"); return -rte_errno; +exit_action_overlap: + rte_flow_error_set(error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ACTION, + actions, "overlapping actions are not supported"); + return -rte_errno; } /** diff --git a/drivers/net/sfc/sfc_flow.c b/drivers/net/sfc/sfc_flow.c index fe4c0b0c50..056405515a 100644 --- a/drivers/net/sfc/sfc_flow.c +++ b/drivers/net/sfc/sfc_flow.c @@ -1467,10 +1467,19 @@ sfc_flow_parse_actions(struct sfc_adapter *sa, } for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { + /* This one may appear anywhere multiple times. */ + if (actions->type == RTE_FLOW_ACTION_TYPE_VOID) + continue; + /* Fate-deciding actions may appear exactly once. */ + if (is_specified) { + rte_flow_error_set + (error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ACTION, + actions, + "Cannot combine several fate-deciding actions," + "choose between QUEUE, RSS or DROP"); + return -rte_errno; + } switch (actions->type) { - case RTE_FLOW_ACTION_TYPE_VOID: - break; - case RTE_FLOW_ACTION_TYPE_QUEUE: rc = sfc_flow_parse_queue(sa, actions->conf, flow); if (rc != 0) { @@ -1512,11 +1521,10 @@ sfc_flow_parse_actions(struct sfc_adapter *sa, } } + /* When fate is unknown, drop traffic. */ if (!is_specified) { - rte_flow_error_set(error, EINVAL, - RTE_FLOW_ERROR_TYPE_ACTION_NUM, actions, - "Action is unspecified"); - return -rte_errno; + flow->spec.template.efs_dmaq_id = + EFX_FILTER_SPEC_RX_DMAQ_ID_DROP; } return 0; diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c index 3b7a960b09..fe2f94010d 100644 --- a/drivers/net/tap/tap_flow.c +++ b/drivers/net/tap/tap_flow.c @@ -1140,6 +1140,7 @@ priv_flow_process(struct pmd_internals *pmd, else goto end; } +actions: for (; actions->type != RTE_FLOW_ACTION_TYPE_END; ++actions) { int err = 0; @@ -1222,6 +1223,16 @@ priv_flow_process(struct pmd_internals *pmd, if (err) goto exit_action_not_supported; } + /* When fate is unknown, drop traffic. */ + if (!action) { + static const struct rte_flow_action drop[] = { + { .type = RTE_FLOW_ACTION_TYPE_DROP, }, + { .type = RTE_FLOW_ACTION_TYPE_END, }, + }; + + actions = drop; + goto actions; + } end: if (flow) tap_nlattr_nested_finish(&flow->msg); /* nested TCA_OPTIONS */ diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h index 6ace24ff4d..96184f030d 100644 --- a/lib/librte_ether/rte_flow.h +++ b/lib/librte_ether/rte_flow.h @@ -859,32 +859,28 @@ struct rte_flow_item { * * Each possible action is represented by a type. Some have associated * configuration structures. Several actions combined in a list can be - * affected to a flow rule. That list is not ordered. + * assigned to a flow rule and are performed in order. * * They fall in three categories: * - * - Terminating actions that prevent processing matched packets by - * subsequent flow rules, unless overridden with PASSTHRU. + * - Actions that modify the fate of matching traffic, for instance by + * dropping or assigning it a specific destination. * - * - Non terminating actions that leave matched packets up for additional - * processing by subsequent flow rules. + * - Actions that modify matching traffic contents or its properties. This + * includes adding/removing encapsulation, encryption, compression and + * marks. * - * - Other non terminating meta actions that do not affect the fate of - * packets. + * - Actions related to the flow rule itself, such as updating counters or + * making it non-terminating. * - * When several actions are combined in a flow rule, they should all have - * different types (e.g. dropping a packet twice is not possible). + * Flow rules being terminating by default, not specifying any action of the + * fate kind results in undefined behavior. This applies to both ingress and + * egress. * - * Only the last action of a given type is taken into account. PMDs still - * perform error checking on the entire list. - * - * Note that PASSTHRU is the only action able to override a terminating - * rule. + * PASSTHRU, when supported, makes a flow rule non-terminating. */ enum rte_flow_action_type { /** - * [META] - * * End marker for action lists. Prevents further processing of * actions, thereby ending the list. * @@ -893,8 +889,6 @@ enum rte_flow_action_type { RTE_FLOW_ACTION_TYPE_END, /** - * [META] - * * Used as a placeholder for convenience. It is ignored and simply * discarded by PMDs. * @@ -903,18 +897,14 @@ enum rte_flow_action_type { RTE_FLOW_ACTION_TYPE_VOID, /** - * Leaves packets up for additional processing by subsequent flow - * rules. This is the default when a rule does not contain a - * terminating action, but can be specified to force a rule to - * become non-terminating. + * Leaves traffic up for additional processing by subsequent flow + * rules; makes a flow rule non-terminating. * * No associated configuration structure. */ RTE_FLOW_ACTION_TYPE_PASSTHRU, /** - * [META] - * * Attaches an integer value to packets and sets PKT_RX_FDIR and * PKT_RX_FDIR_ID mbuf flags. * @@ -923,8 +913,6 @@ enum rte_flow_action_type { RTE_FLOW_ACTION_TYPE_MARK, /** - * [META] - * * Flags packets. Similar to MARK without a specific value; only * sets the PKT_RX_FDIR mbuf flag. * @@ -949,9 +937,7 @@ enum rte_flow_action_type { RTE_FLOW_ACTION_TYPE_DROP, /** - * [META] - * - * Enables counters for this rule. + * Enables counters for this flow rule. * * These counters can be retrieved and reset through rte_flow_query(), * see struct rte_flow_query_count. @@ -1020,8 +1006,6 @@ struct rte_flow_action_mark { * RTE_FLOW_ACTION_TYPE_QUEUE * * Assign packets to a given queue index. - * - * Terminating by default. */ struct rte_flow_action_queue { uint16_t index; /**< Queue index to use. */ @@ -1050,8 +1034,6 @@ struct rte_flow_query_count { * Note: RSS hash result is stored in the hash.rss mbuf field which overlaps * hash.fdir.lo. Since the MARK action sets the hash.fdir.hi field only, * both can be requested simultaneously. - * - * Terminating by default. */ struct rte_flow_action_rss { const struct rte_eth_rss_conf *rss_conf; /**< RSS parameters. */ @@ -1069,8 +1051,6 @@ struct rte_flow_action_rss { * and is not guaranteed to work properly if the VF part is matched by a * prior flow rule or if packets are not addressed to a VF in the first * place. - * - * Terminating by default. */ struct rte_flow_action_vf { uint32_t original:1; /**< Use original VF ID if possible. */ @@ -1085,8 +1065,6 @@ struct rte_flow_action_vf { * * Packets matched by items of this type can be either dropped or passed to the * next item with their color set by the MTR object. - * - * Non-terminating by default. */ struct rte_flow_action_meter { uint32_t mtr_id; /**< MTR object ID created with rte_mtr_create(). */ @@ -1116,8 +1094,6 @@ struct rte_flow_action_meter { * direction. * * Multiple flows can be configured to use the same security session. - * - * Non-terminating by default. */ struct rte_flow_action_security { void *security_session; /**< Pointer to security session structure. */