[v2] ethdev: add Tx queue flow matching item
Checks
Commit Message
From: Kiran Kumar K <kirankumark@marvell.com>
Adding support for Tx queue flow matching item.
This item is valid only for egress rules.
An example use case would be that application can
set different vlan insert rules with different PCP values
based on Tx queue number.
Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
---
app/test-pmd/cmdline_flow.c | 28 +++++++++++++++++++++
doc/guides/prog_guide/rte_flow.rst | 7 ++++++
doc/guides/rel_notes/release_23_07.rst | 5 ++++
doc/guides/testpmd_app_ug/testpmd_funcs.rst | 4 +++
lib/ethdev/rte_flow.c | 1 +
lib/ethdev/rte_flow.h | 26 +++++++++++++++++++
6 files changed, 71 insertions(+)
Comments
Hi Kirankumark,
> -----Original Message-----
> From: kirankumark@marvell.com <kirankumark@marvell.com>
> Sent: Monday, May 8, 2023 4:50 PM
>
> From: Kiran Kumar K <kirankumark@marvell.com>
>
> Adding support for Tx queue flow matching item.
> This item is valid only for egress rules.
> An example use case would be that application can
> set different vlan insert rules with different PCP values
> based on Tx queue number.
>
> Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> ---
Acked-by: Ori Kam <orika@nvidia.com>
Best,
Ori
08/05/2023 15:49, kirankumark@marvell.com:
> From: Kiran Kumar K <kirankumark@marvell.com>
>
> Adding support for Tx queue flow matching item.
> This item is valid only for egress rules.
> An example use case would be that application can
> set different vlan insert rules with different PCP values
> based on Tx queue number.
>
> Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> ---
> app/test-pmd/cmdline_flow.c | 28 +++++++++++++++++++++
> doc/guides/prog_guide/rte_flow.rst | 7 ++++++
> doc/guides/rel_notes/release_23_07.rst | 5 ++++
> doc/guides/testpmd_app_ug/testpmd_funcs.rst | 4 +++
> lib/ethdev/rte_flow.c | 1 +
> lib/ethdev/rte_flow.h | 26 +++++++++++++++++++
> 6 files changed, 71 insertions(+)
That's only 71 lines but I could make 10 comments.
I'm fixing spacing, alignment, sorting, etc while pulling next-net,
but it would have been more confortable if more attention was paid
by the author and the reviewers.
I have a question though (it could be fixed in a later patch):
> +struct rte_flow_item_tx_queue {
> + /** Tx queue number that packet is being transmitted */
> + uint16_t tx_queue;
> +};
This field is used with host endianness, right?
> +static const struct rte_flow_item_tx_queue rte_flow_item_tx_queue_mask = {
> + .tx_queue = RTE_BE16(0xffff),
> +};
So why using RTE_BE16 to set a field which is not specifically big endian?
(not talking about the fact that 0xffff is the same no matter the endianness,
and not talking about the fact that it is better to use UINT16_MAX).
08/06/2023 21:59, Thomas Monjalon:
> 08/05/2023 15:49, kirankumark@marvell.com:
> > From: Kiran Kumar K <kirankumark@marvell.com>
> >
> > Adding support for Tx queue flow matching item.
> > This item is valid only for egress rules.
> > An example use case would be that application can
> > set different vlan insert rules with different PCP values
> > based on Tx queue number.
> >
> > Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> > ---
> > app/test-pmd/cmdline_flow.c | 28 +++++++++++++++++++++
> > doc/guides/prog_guide/rte_flow.rst | 7 ++++++
> > doc/guides/rel_notes/release_23_07.rst | 5 ++++
> > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 4 +++
> > lib/ethdev/rte_flow.c | 1 +
> > lib/ethdev/rte_flow.h | 26 +++++++++++++++++++
> > 6 files changed, 71 insertions(+)
>
> That's only 71 lines but I could make 10 comments.
> I'm fixing spacing, alignment, sorting, etc while pulling next-net,
> but it would have been more confortable if more attention was paid
> by the author and the reviewers.
>
>
> I have a question though (it could be fixed in a later patch):
>
> > +struct rte_flow_item_tx_queue {
> > + /** Tx queue number that packet is being transmitted */
> > + uint16_t tx_queue;
> > +};
>
> This field is used with host endianness, right?
>
> > +static const struct rte_flow_item_tx_queue rte_flow_item_tx_queue_mask = {
> > + .tx_queue = RTE_BE16(0xffff),
> > +};
>
> So why using RTE_BE16 to set a field which is not specifically big endian?
> (not talking about the fact that 0xffff is the same no matter the endianness,
> and not talking about the fact that it is better to use UINT16_MAX).
Repeating again:
This patch has bad quality, bad review,
I've noticed something wrong,
and there are 0 comments or reply!
Now David has sent a fix for this:
https://patches.dpdk.org/project/dpdk/patch/20230629135839.974700-1-david.marchand@redhat.com/
Don't expect next patches to be merged quickly.
@@ -496,6 +496,8 @@ enum index {
ITEM_QUOTA_STATE_NAME,
ITEM_AGGR_AFFINITY,
ITEM_AGGR_AFFINITY_VALUE,
+ ITEM_TX_QUEUE,
+ ITEM_TX_QUEUE_VALUE,
/* Validate/create actions. */
ACTIONS,
@@ -1452,6 +1454,7 @@ static const enum index next_item[] = {
ITEM_METER,
ITEM_QUOTA,
ITEM_AGGR_AFFINITY,
+ ITEM_TX_QUEUE,
END_SET,
ZERO,
};
@@ -1953,6 +1956,12 @@ static const enum index item_aggr_affinity[] = {
ZERO,
};
+static const enum index item_tx_queue[] = {
+ ITEM_TX_QUEUE_VALUE,
+ ITEM_NEXT,
+ ZERO,
+};
+
static const enum index next_action[] = {
ACTION_END,
ACTION_VOID,
@@ -6945,6 +6954,22 @@ static const struct token token_list[] = {
.args = ARGS(ARGS_ENTRY(struct rte_flow_item_aggr_affinity,
affinity)),
},
+ [ITEM_TX_QUEUE] = {
+ .name = "tx_queue",
+ .help = "match on the tx queue of send packet",
+ .priv = PRIV_ITEM(TX_QUEUE,
+ sizeof(struct rte_flow_item_tx_queue)),
+ .next = NEXT(item_tx_queue),
+ .call = parse_vc,
+ },
+ [ITEM_TX_QUEUE_VALUE] = {
+ .name = "tx_queue_value",
+ .help = "tx queue value",
+ .next = NEXT(item_tx_queue, NEXT_ENTRY(COMMON_UNSIGNED),
+ item_param),
+ .args = ARGS(ARGS_ENTRY(struct rte_flow_item_tx_queue,
+ tx_queue)),
+ },
};
/** Remove and return last entry from argument stack. */
@@ -11849,6 +11874,9 @@ flow_item_default_mask(const struct rte_flow_item *item)
case RTE_FLOW_ITEM_TYPE_AGGR_AFFINITY:
mask = &rte_flow_item_aggr_affinity_mask;
break;
+ case RTE_FLOW_ITEM_TYPE_TX_QUEUE:
+ mask = &rte_flow_item_tx_queue_mask;
+ break;
default:
break;
}
@@ -1486,6 +1486,13 @@ This item is meant to use the same structure as `Item: PORT_REPRESENTOR`_.
See also `Action: REPRESENTED_PORT`_.
+Item: ``TX_QUEUE``
+^^^^^^^^^^^^^^^^^^^^^^^
+
+Matches on the Tx queue of send packet .
+
+- ``tx_queue``: Tx queue.
+
Item: ``AGGR_AFFINITY``
^^^^^^^^^^^^^^^^^^^^^^^
@@ -55,6 +55,11 @@ New Features
Also, make sure to start the actual text at the margin.
=======================================================
+ * **Added flow matching of tx queue.**
+
+ Added ``RTE_FLOW_ITEM_TYPE_TX_QUEUE`` rte_flow pattern to match tx queue of
+ send packet.
+
Removed Items
-------------
@@ -3779,6 +3779,10 @@ This section lists supported pattern items and their attributes, if any.
- ``affinity {value}``: aggregated port (starts from 1).
+- ``tx_queue``: match tx queue of send packet.
+
+ - ``tx_queue {value}``: send queue value (starts from 0).
+
- ``send_to_kernel``: send packets to kernel.
@@ -164,6 +164,7 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
MK_FLOW_ITEM(IPV6_ROUTING_EXT, sizeof(struct rte_flow_item_ipv6_routing_ext)),
MK_FLOW_ITEM(QUOTA, sizeof(struct rte_flow_item_quota)),
MK_FLOW_ITEM(AGGR_AFFINITY, sizeof(struct rte_flow_item_aggr_affinity)),
+ MK_FLOW_ITEM(TX_QUEUE, sizeof(struct rte_flow_item_tx_queue)),
};
/** Generate flow_action[] entry. */
@@ -672,8 +672,34 @@ enum rte_flow_item_type {
* @see struct rte_flow_item_aggr_affinity.
*/
RTE_FLOW_ITEM_TYPE_AGGR_AFFINITY,
+ /**
+ * Match Tx queue number.
+ * This is valid only for egress rules.
+ *
+ * @see struct rte_flow_item_tx_queue
+ */
+ RTE_FLOW_ITEM_TYPE_TX_QUEUE,
};
+/**
+ * RTE_FLOW_ITEM_TYPE_TX_QUEUE
+ *
+ * Tx queue number
+ *
+ * @see struct rte_flow_item_tx_queue
+ */
+struct rte_flow_item_tx_queue {
+ /** Tx queue number that packet is being transmitted */
+ uint16_t tx_queue;
+};
+
+/** Default mask for RTE_FLOW_ITEM_TX_QUEUE. */
+#ifndef __cplusplus
+static const struct rte_flow_item_tx_queue rte_flow_item_tx_queue_mask = {
+ .tx_queue = RTE_BE16(0xffff),
+};
+#endif
+
/**
* @warning
* @b EXPERIMENTAL: this API may change without prior notice.