Hi Ori,
On 10/11/21 1:02 PM, Ori Kam wrote:
> Hi Andrew,
>
> Sorry but I think I missed something.
>
>> -----Original Message-----
>> From: dev <dev-boun...@dpdk.org> On Behalf Of Andrew Rybchenko
>> Sent: Friday, October 8, 2021 1:26 PM
>>
>> Indirect actions should be used to do shared counters.
>>
>> Signed-off-by: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
>> Acked-by: Thomas Monjalon <tho...@monjalon.net>
>> Acked-by: Ajit Khaparde <ajit.khapa...@broadcom.com>
>> Acked-by: Somnath Kotur <somnath.ko...@broadcom.com>
>> Acked-by: Ori Kam <or...@nvidia.com>
>> ---
>> v2:
>> - remove reserved field from count structure (review from Stephen)
>> - apply mlx5 review notes from Matan
>>
>> app/test-pmd/cmdline_flow.c | 10 --
>> doc/guides/prog_guide/rte_flow.rst | 19 +---
>> doc/guides/rel_notes/deprecation.rst | 4 -
>> doc/guides/rel_notes/release_21_11.rst | 4 +
>> drivers/net/bnxt/tf_ulp/ulp_rte_parser.c | 5 -
>> drivers/net/cnxk/cnxk_rte_flow.c | 8 --
>> drivers/net/hns3/hns3_flow.c | 3 +-
>> drivers/net/ice/ice_fdir_filter.c | 4 +-
>> drivers/net/mlx5/mlx5.c | 11 --
>> drivers/net/mlx5/mlx5.h | 9 --
>> drivers/net/mlx5/mlx5_flow_dv.c | 118 ++-------------------
>> drivers/net/mlx5/mlx5_flow_verbs.c | 22 +---
>> drivers/net/octeontx2/otx2_flow_parse.c | 10 --
>> drivers/net/sfc/sfc_mae.c | 9 +-
>> drivers/net/softnic/rte_eth_softnic_flow.c | 7 --
>> lib/ethdev/rte_flow.h | 16 +--
>> 16 files changed, 22 insertions(+), 237 deletions(-)
>>
>
> [Snip]
>
>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
>> 7b1ed7f110..9819c25d2f 100644
>> --- a/lib/ethdev/rte_flow.h
>> +++ b/lib/ethdev/rte_flow.h
>> @@ -75,7 +75,7 @@ extern "C" {
>> * At least one direction must be specified.
>> *
>> * Specifying both directions at once for a given rule is not recommended
>> - * but may be valid in a few cases (e.g. shared counter).
>> + * but may be valid in a few cases.
>> */
>> struct rte_flow_attr {
>> uint32_t group; /**< Priority group. */ @@ -2498,24 +2498,10 @@ struct
>> rte_flow_query_age
>> {
>> * Counters can be retrieved and reset through ``rte_flow_query()``, see
>> * ``struct rte_flow_query_count``.
>> *
>> - * @deprecated Shared attribute is deprecated, use generic
>> - * RTE_FLOW_ACTION_TYPE_INDIRECT action.
>> - *
>> - * The shared flag indicates whether the counter is unique to the flow rule
>> the
>> - * action is specified with, or whether it is a shared counter.
>> - *
>> - * For a count action with the shared flag set, then then a global device
>> - * namespace is assumed for the counter id, so that any matched flow rules
>> using
>> - * a count action with the same counter id on the same port will contribute
>> to
>> - * that counter.
>> - *
>> * For ports within the same switch domain then the counter id namespace
>> extends
>> * to all ports within that switch domain.
>
> I don't think we need this anymore.
I agree. I'll remove it in v3 if required, but I hope it could
be removed on apply as well.
>
>> */
>> struct rte_flow_action_count {
>> - /** @deprecated Share counter ID with other flow rules. */
>> - uint32_t shared:1;
>> - uint32_t reserved:31; /**< Reserved, must be zero. */
>> uint32_t id; /**< Counter ID. */
>
> Why do we need to keep the id field?
It is a very good question. I thought about it and preserved it
for the corner case of two COUNT actions in the same rule.
If so, id is required to distinguish on query.
I don't know if we really need it to have two basically
duplicate counters in the same rule. However, since order
of actions matter, COUNT, VXLAN_ENCAP, COUNT should produce
different byte counters.
I suggest to continue discussion and gather more thought on it,
but do not block the patch, since strictly speaking it is a
bit separate topic as noted above.
Thanks,
Andrew.
