From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 892718E5B for ; Thu, 26 Apr 2018 16:48:29 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Apr 2018 07:48:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,330,1520924400"; d="scan'208";a="34842281" Received: from dwdohert-mobl.ger.corp.intel.com (HELO [10.252.2.128]) ([10.252.2.128]) by fmsmga007.fm.intel.com with ESMTP; 26 Apr 2018 07:48:27 -0700 To: Ori Kam , Ferruh Yigit , "dev@dpdk.org" , Matan Azrad , Shahaf Shuler References: <20180426120817.6612-1-declan.doherty@intel.com> <20180426120817.6612-5-declan.doherty@intel.com> <5f430dca-6cfe-76e2-9250-415d41aac295@intel.com> From: "Doherty, Declan" Message-ID: Date: Thu, 26 Apr 2018 15:48:26 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 4/4] ethdev: add shared counter support to rte_flow X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 26 Apr 2018 14:48:30 -0000 On 26/04/2018 3:43 PM, Ori Kam wrote: > Hi, > > PSB > > Ori >> -----Original Message----- >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] >> Sent: Thursday, April 26, 2018 5:28 PM >> To: Ori Kam ; Declan Doherty >> ; dev@dpdk.org; Matan Azrad >> >> Subject: Re: [dpdk-dev] [PATCH v6 4/4] ethdev: add shared counter support >> to rte_flow >> >> On 4/26/2018 3:06 PM, Ori Kam wrote: >>> Hi Declan, >>> >>> You are changing API (port_flow_query) which is in use in both MLX5 >>> and MLX4 this results in breaking the build. >> >> Hi Ori, >> >> Do you mean "rte_flow_query"? port_flow_query() is a function in testpmd, >> how mlx is using it? >> > > My bad let me be clearer. > MLX5 and MLX4 are implementing the rte_flow_ops query function. > This patch changes the prototype for the query function which results in > compilation error and should also result in some change in the MLX5 and MLX4 implementation. > > Hey Ori, I don't see any references to the query in MLX4 code. static const struct rte_flow_ops mlx4_flow_ops = { .validate = mlx4_flow_validate, .create = mlx4_flow_create, .destroy = mlx4_flow_destroy, .flush = mlx4_flow_flush, .isolate = mlx4_flow_isolate, }; I just looking at fixing the MLX5 code compilation, I missed that due to the function being ifdefined out with HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT the flag and I missed the reference. >>> >>> Best, >>> Ori >>> >>>> -----Original Message----- >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Declan Doherty >>>> Sent: Thursday, April 26, 2018 3:08 PM >>>> To: dev@dpdk.org >>>> Cc: Ferruh Yigit ; Declan Doherty >>>> >>>> Subject: [dpdk-dev] [PATCH v6 4/4] ethdev: add shared counter support >>>> to rte_flow >>>> >>>> Add rte_flow_action_count action data structure to enable shared >>>> counters across multiple flows on a single port or across multiple >>>> flows on multiple ports within the same switch domain. Also this >>>> enables multiple count actions to be specified in a single flow action. >>>> >>>> This patch also modifies the existing rte_flow_query API to take the >>>> rte_flow_action structure as an input parameter instead of the >>>> rte_flow_action_type enumeration to allow querying a specific action >>>> from a flow rule when multiple actions of the same type are specified. >>>> >>>> This patch also contains updates for the bonding and failsafe PMDs >>>> and testpmd application which are affected by this API change. >>>> >>>> Signed-off-by: Declan Doherty >>>> --- >>>> app/test-pmd/cmdline_flow.c | 6 ++-- >>>> app/test-pmd/config.c | 15 +++++---- >>>> app/test-pmd/testpmd.h | 2 +- >>>> doc/guides/prog_guide/rte_flow.rst | 59 +++++++++++++++++++++-- >> ---- >>>> ------ >>>> drivers/net/bonding/rte_eth_bond_flow.c | 9 ++--- >>>> drivers/net/failsafe/failsafe_flow.c | 4 +-- >>>> lib/librte_ether/rte_flow.c | 2 +- >>>> lib/librte_ether/rte_flow.h | 38 +++++++++++++++++++-- >>>> lib/librte_ether/rte_flow_driver.h | 2 +- >>>> 9 files changed, 93 insertions(+), 44 deletions(-) >>>> >>>> diff --git a/app/test-pmd/cmdline_flow.c >>>> b/app/test-pmd/cmdline_flow.c index 1ac04a0ab..5754e7858 100644 >>>> --- a/app/test-pmd/cmdline_flow.c >>>> +++ b/app/test-pmd/cmdline_flow.c >>>> @@ -420,7 +420,7 @@ struct buffer { >>>> } destroy; /**< Destroy arguments. */ >>>> struct { >>>> uint32_t rule; >>>> - enum rte_flow_action_type action; >>>> + struct rte_flow_action action; >>>> } query; /**< Query arguments. */ >>>> struct { >>>> uint32_t *group; >>>> @@ -1101,7 +1101,7 @@ static const struct token token_list[] = { >>>> .next = NEXT(NEXT_ENTRY(QUERY_ACTION), >>>> NEXT_ENTRY(RULE_ID), >>>> NEXT_ENTRY(PORT_ID)), >>>> - .args = ARGS(ARGS_ENTRY(struct buffer, args.query.action), >>>> + .args = ARGS(ARGS_ENTRY(struct buffer, >>>> args.query.action.type), >>>> ARGS_ENTRY(struct buffer, args.query.rule), >>>> ARGS_ENTRY(struct buffer, port)), >>>> .call = parse_query, >>>> @@ -3842,7 +3842,7 @@ cmd_flow_parsed(const struct buffer *in) >>>> break; >>>> case QUERY: >>>> port_flow_query(in->port, in->args.query.rule, >>>> - in->args.query.action); >>>> + &in->args.query.action); >>>> break; >>>> case LIST: >>>> port_flow_list(in->port, in->args.list.group_n, diff --git >>>> a/app/test-pmd/config.c b/app/test-pmd/config.c index >>>> 0f2425229..cd6102dfc 100644 >>>> --- a/app/test-pmd/config.c >>>> +++ b/app/test-pmd/config.c >>>> @@ -1452,7 +1452,7 @@ port_flow_flush(portid_t port_id) >>>> /** Query a flow rule. */ >>>> int >>>> port_flow_query(portid_t port_id, uint32_t rule, >>>> - enum rte_flow_action_type action) >>>> + const struct rte_flow_action *action) >>>> { >>>> struct rte_flow_error error; >>>> struct rte_port *port; >>>> @@ -1474,15 +1474,16 @@ port_flow_query(portid_t port_id, uint32_t >> rule, >>>> return -ENOENT; >>>> } >>>> if ((unsigned int)action >= RTE_DIM(flow_action) || >>>> - !flow_action[action].name) >>>> + !flow_action[action->type].name) >>>> name = "unknown"; >>>> else >>>> - name = flow_action[action].name; >>>> - switch (action) { >>>> + name = flow_action[action->type].name; >>>> + switch (action->type) { >>>> case RTE_FLOW_ACTION_TYPE_COUNT: >>>> break; >>>> default: >>>> - printf("Cannot query action type %d (%s)\n", action, name); >>>> + printf("Cannot query action type %d (%s)\n", >>>> + action->type, name); >>>> return -ENOTSUP; >>>> } >>>> /* Poisoning to make sure PMDs update it in case of error. */ @@ >>>> -1490,7 +1491,7 @@ port_flow_query(portid_t port_id, uint32_t rule, >>>> memset(&query, 0, sizeof(query)); >>>> if (rte_flow_query(port_id, pf->flow, action, &query, &error)) >>>> return port_flow_complain(&error); >>>> - switch (action) { >>>> + switch (action->type) { >>>> case RTE_FLOW_ACTION_TYPE_COUNT: >>>> printf("%s:\n" >>>> " hits_set: %u\n" >>>> @@ -1505,7 +1506,7 @@ port_flow_query(portid_t port_id, uint32_t >> rule, >>>> break; >>>> default: >>>> printf("Cannot display result for action type %d (%s)\n", >>>> - action, name); >>>> + action->type, name); >>>> break; >>>> } >>>> return 0; >>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index >>>> a33b525e2..1af87b8f4 100644 >>>> --- a/app/test-pmd/testpmd.h >>>> +++ b/app/test-pmd/testpmd.h >>>> @@ -620,7 +620,7 @@ int port_flow_create(portid_t port_id, int >>>> port_flow_destroy(portid_t port_id, uint32_t n, const uint32_t >>>> *rule); int port_flow_flush(portid_t port_id); int >>>> port_flow_query(portid_t port_id, uint32_t rule, >>>> - enum rte_flow_action_type action); >>>> + const struct rte_flow_action *action); >>>> void port_flow_list(portid_t port_id, uint32_t n, const uint32_t >>>> *group); int port_flow_isolate(portid_t port_id, int set); >>>> >>>> diff --git a/doc/guides/prog_guide/rte_flow.rst >>>> b/doc/guides/prog_guide/rte_flow.rst >>>> index 301f8762e..88bfc87eb 100644 >>>> --- a/doc/guides/prog_guide/rte_flow.rst >>>> +++ b/doc/guides/prog_guide/rte_flow.rst >>>> @@ -1277,17 +1277,19 @@ Actions are performed in list order: >>>> >>>> .. table:: Mark, count then redirect >>>> >>>> - +-------+--------+-----------+-------+ >>>> - | Index | Action | Field | Value | >>>> - +=======+========+===========+=======+ >>>> - | 0 | MARK | ``mark`` | 0x2a | >>>> - +-------+--------+-----------+-------+ >>>> - | 1 | COUNT | >>>> - +-------+--------+-----------+-------+ >>>> - | 2 | QUEUE | ``queue`` | 10 | >>>> - +-------+--------+-----------+-------+ >>>> - | 3 | END | >>>> - +-------+----------------------------+ >>>> + +-------+--------+------------+-------+ >>>> + | Index | Action | Field | Value | >>>> + +=======+========+============+=======+ >>>> + | 0 | MARK | ``mark`` | 0x2a | >>>> + +-------+--------+------------+-------+ >>>> + | 1 | COUNT | ``shared`` | 0 | >>>> + | | +------------+-------+ >>>> + | | | ``id`` | 0 | >>>> + +-------+--------+------------+-------+ >>>> + | 2 | QUEUE | ``queue`` | 10 | >>>> + +-------+--------+------------+-------+ >>>> + | 3 | END | >>>> + +-------+-----------------------------+ >>>> >>>> | >>>> >>>> @@ -1516,23 +1518,36 @@ Drop packets. >>>> Action: ``COUNT`` >>>> ^^^^^^^^^^^^^^^^^ >>>> >>>> -Enables counters for this rule. >>>> +Adds a counter action to a matched flow. >>>> + >>>> +If more than one count action is specified in a single flow rule, >>>> +then each action must specify a unique id. >>>> >>>> -These counters can be retrieved and reset through >>>> ``rte_flow_query()``, see >>>> +Counters can be retrieved and reset through ``rte_flow_query()``, >>>> +see >>>> ``struct rte_flow_query_count``. >>>> >>>> -- Counters can be retrieved with ``rte_flow_query()``. >>>> -- No configurable properties. >>>> +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. >>>> >>>> .. _table_rte_flow_action_count: >>>> >>>> .. table:: COUNT >>>> >>>> - +---------------+ >>>> - | Field | >>>> - +===============+ >>>> - | no properties | >>>> - +---------------+ >>>> + +------------+---------------------+ >>>> + | Field | Value | >>>> + +============+=====================+ >>>> + | ``shared`` | shared counter flag | >>>> + +------------+---------------------+ >>>> + | ``id`` | counter id | >>>> + +------------+---------------------+ >>>> >>>> Query structure to retrieve and reset flow rule counters: >>>> >>>> @@ -2282,7 +2297,7 @@ definition. >>>> int >>>> rte_flow_query(uint16_t port_id, >>>> struct rte_flow *flow, >>>> - enum rte_flow_action_type action, >>>> + const struct rte_flow_action *action, >>>> void *data, >>>> struct rte_flow_error *error); >>>> >>>> @@ -2290,7 +2305,7 @@ Arguments: >>>> >>>> - ``port_id``: port identifier of Ethernet device. >>>> - ``flow``: flow rule handle to query. >>>> -- ``action``: action type to query. >>>> +- ``action``: action to query, this must match prototype from flow rule. >>>> - ``data``: pointer to storage for the associated query data type. >>>> - ``error``: perform verbose error reporting if not NULL. PMDs initialize >>>> this structure in case of error only. >>>> diff --git a/drivers/net/bonding/rte_eth_bond_flow.c >>>> b/drivers/net/bonding/rte_eth_bond_flow.c >>>> index 8093c04f5..31e4bcaeb 100644 >>>> --- a/drivers/net/bonding/rte_eth_bond_flow.c >>>> +++ b/drivers/net/bonding/rte_eth_bond_flow.c >>>> @@ -152,6 +152,7 @@ bond_flow_flush(struct rte_eth_dev *dev, struct >>>> rte_flow_error *err) >>>> >>>> static int >>>> bond_flow_query_count(struct rte_eth_dev *dev, struct rte_flow >>>> *flow, >>>> + const struct rte_flow_action *action, >>>> struct rte_flow_query_count *count, >>>> struct rte_flow_error *err) >>>> { >>>> @@ -165,7 +166,7 @@ bond_flow_query_count(struct rte_eth_dev >> *dev, >>>> struct rte_flow *flow, >>>> rte_memcpy(&slave_count, count, sizeof(slave_count)); >>>> for (i = 0; i < internals->slave_count; i++) { >>>> ret = rte_flow_query(internals->slaves[i].port_id, >>>> - flow->flows[i], >>>> RTE_FLOW_ACTION_TYPE_COUNT, >>>> + flow->flows[i], action, >>>> &slave_count, err); >>>> if (unlikely(ret != 0)) { >>>> RTE_BOND_LOG(ERR, "Failed to query flow on" >>>> @@ -182,12 +183,12 @@ bond_flow_query_count(struct rte_eth_dev >> *dev, >>>> struct rte_flow *flow, >>>> >>>> static int >>>> bond_flow_query(struct rte_eth_dev *dev, struct rte_flow *flow, >>>> - enum rte_flow_action_type type, void *arg, >>>> + const struct rte_flow_action *action, void *arg, >>>> struct rte_flow_error *err) >>>> { >>>> - switch (type) { >>>> + switch (action->type) { >>>> case RTE_FLOW_ACTION_TYPE_COUNT: >>>> - return bond_flow_query_count(dev, flow, arg, err); >>>> + return bond_flow_query_count(dev, flow, action, arg, err); >>>> default: >>>> return rte_flow_error_set(err, ENOTSUP, >>>> RTE_FLOW_ERROR_TYPE_ACTION, >>>> arg, >>>> diff --git a/drivers/net/failsafe/failsafe_flow.c >>>> b/drivers/net/failsafe/failsafe_flow.c >>>> index a97f4075d..bfe42fcee 100644 >>>> --- a/drivers/net/failsafe/failsafe_flow.c >>>> +++ b/drivers/net/failsafe/failsafe_flow.c >>>> @@ -174,7 +174,7 @@ fs_flow_flush(struct rte_eth_dev *dev, static >>>> int fs_flow_query(struct rte_eth_dev *dev, >>>> struct rte_flow *flow, >>>> - enum rte_flow_action_type type, >>>> + const struct rte_flow_action *action, >>>> void *arg, >>>> struct rte_flow_error *error) { @@ -185,7 +185,7 @@ >>>> fs_flow_query(struct rte_eth_dev *dev, >>>> if (sdev != NULL) { >>>> int ret = rte_flow_query(PORT_ID(sdev), >>>> flow->flows[SUB_ID(sdev)], >>>> - type, arg, error); >>>> + action, arg, error); >>>> >>>> if ((ret = fs_err(sdev, ret))) { >>>> fs_unlock(dev, 0); >>>> diff --git a/lib/librte_ether/rte_flow.c >>>> b/lib/librte_ether/rte_flow.c index 4f94ac9b5..7947529da 100644 >>>> --- a/lib/librte_ether/rte_flow.c >>>> +++ b/lib/librte_ether/rte_flow.c >>>> @@ -233,7 +233,7 @@ rte_flow_flush(uint16_t port_id, int >>>> rte_flow_query(uint16_t port_id, >>>> struct rte_flow *flow, >>>> - enum rte_flow_action_type action, >>>> + const struct rte_flow_action *action, >>>> void *data, >>>> struct rte_flow_error *error) { diff --git >>>> a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h index >>>> d390bbf5a..f8ba71cdb 100644 >>>> --- a/lib/librte_ether/rte_flow.h >>>> +++ b/lib/librte_ether/rte_flow.h >>>> @@ -1314,7 +1314,7 @@ enum rte_flow_action_type { >>>> * These counters can be retrieved and reset through >>>> rte_flow_query(), >>>> * see struct rte_flow_query_count. >>>> * >>>> - * No associated configuration structure. >>>> + * See struct rte_flow_action_count. >>>> */ >>>> RTE_FLOW_ACTION_TYPE_COUNT, >>>> >>>> @@ -1546,6 +1546,38 @@ struct rte_flow_action_queue { >>>> uint16_t index; /**< Queue index to use. */ }; >>>> >>>> + >>>> +/** >>>> + * @warning >>>> + * @b EXPERIMENTAL: this structure may change without prior notice >>>> + * >>>> + * RTE_FLOW_ACTION_TYPE_COUNT >>>> + * >>>> + * Adds a counter action to a matched flow. >>>> + * >>>> + * If more than one count action is specified in a single flow rule, >>>> +then each >>>> + * action must specify a unique id. >>>> + * >>>> + * Counters can be retrieved and reset through ``rte_flow_query()``, >>>> +see >>>> + * ``struct rte_flow_query_count``. >>>> + * >>>> + * 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. >>>> + */ >>>> +struct rte_flow_action_count { >>>> + uint32_t shared:1; /**< Share counter ID with other flow rules. */ >>>> + uint32_t reserved:31; /**< Reserved, must be zero. */ >>>> + uint32_t id; /**< Counter ID. */ >>>> +}; >>>> + >>>> /** >>>> * RTE_FLOW_ACTION_TYPE_COUNT (query) >>>> * >>>> @@ -2044,7 +2076,7 @@ rte_flow_flush(uint16_t port_id, >>>> * @param flow >>>> * Flow rule handle to query. >>>> * @param action >>>> - * Action type to query. >>>> + * Action definition as defined in original flow rule. >>>> * @param[in, out] data >>>> * Pointer to storage for the associated query data type. >>>> * @param[out] error >>>> @@ -2057,7 +2089,7 @@ rte_flow_flush(uint16_t port_id, int >>>> rte_flow_query(uint16_t port_id, >>>> struct rte_flow *flow, >>>> - enum rte_flow_action_type action, >>>> + const struct rte_flow_action *action, >>>> void *data, >>>> struct rte_flow_error *error); >>>> >>>> diff --git a/lib/librte_ether/rte_flow_driver.h >>>> b/lib/librte_ether/rte_flow_driver.h >>>> index 3800310ba..1c90c600d 100644 >>>> --- a/lib/librte_ether/rte_flow_driver.h >>>> +++ b/lib/librte_ether/rte_flow_driver.h >>>> @@ -88,7 +88,7 @@ struct rte_flow_ops { >>>> int (*query) >>>> (struct rte_eth_dev *, >>>> struct rte_flow *, >>>> - enum rte_flow_action_type, >>>> + const struct rte_flow_action *, >>>> void *, >>>> struct rte_flow_error *); >>>> /** See rte_flow_isolate(). */ >>>> -- >>>> 2.14.3 >>> >