From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f194.google.com (mail-wr0-f194.google.com [209.85.128.194]) by dpdk.org (Postfix) with ESMTP id AFD3D1CC7A for ; Fri, 6 Apr 2018 22:32:43 +0200 (CEST) Received: by mail-wr0-f194.google.com with SMTP id d17so2838991wre.1 for ; Fri, 06 Apr 2018 13:32:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=38H84jifytAogmDa2oBcCgdSmXvuifKMbYISL3dOimY=; b=TINQAWXMs82R0JtsqOWmpjeP0crAjAc4ezv5nxy4rBsdXFhT5mzg1kozqGTUhrEo2c Vjl8BwWeYSd8N1Y9uh8RFxMlxwaPIfIIpvs5xe3Ia6LZ23xD9ieQNG3ZuBWh53NMKemi 47JfRRt8SO0d8uTizuPce2q1TcqwJSBGnwUXhUSxeF6vCoxwX2Uc9juev6ur4o/RezeD UOtpbFmODvpX06bdIf53o1cWqZGtVcFmm8N0Cpauz/yORq6ur06fjBX8Ra74j6FChLhh ByXanlZE7Sbsro3+QriHjOpVWWng6XGsqD7iHlxAduujUy2Ps05j/btZFnADMuzHdsXU 42QA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=38H84jifytAogmDa2oBcCgdSmXvuifKMbYISL3dOimY=; b=RDEoPdHDJsiu1waM7eBeruv1WtooapH1AQ4hu2JRXggxu4aVf7rGoQxWxigMyCAAdH 1o+Msv53eozSw+j51kn49Zn++dYhATHxHoLIqUfqhCh8iOWE7MiXZsOLsDPijT1qVTjl 8jiNWl9n81Yj0CXM3HK3dN4cCnIvta3YMm6k+g60P8Ylhco8a5WHIyAxYg8MkJvIdPch 8dUWzUJhnkACARcdbRgKQft9reyd0Z9ZTed7rdyZKL7u2tffVzMyiojNjb1ctxooimzk 7kXKCfPDbl59J6ttNPXYjpRla6M1OkaULq4v4Pku8Mu/0FZEEE3i0aRWVdbvveDmXQQZ 5yGA== X-Gm-Message-State: AElRT7HoyqgJ3lPIV5GcyaC91HjBYNu4OCqRe+/8Kv2H9a8gdXrus2Gb 9/m/JgT+v9ydPJst3KsF3Q/Eqw== X-Google-Smtp-Source: AIpwx48LH0f6eP3Ywfg0WUq02r7A1Yb8UB33H0nJEGh9xZgTz9oWKmrtSTqkPelddkcovBgITo/UnQ== X-Received: by 10.223.195.132 with SMTP id p4mr19459610wrf.279.1523046400181; Fri, 06 Apr 2018 13:26:40 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id i141sm10283726wmd.6.2018.04.06.13.26.38 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 06 Apr 2018 13:26:39 -0700 (PDT) Date: Fri, 6 Apr 2018 22:26:25 +0200 From: Adrien Mazarguil To: Declan Doherty Cc: dev@dpdk.org Message-ID: <20180406202625.GR4957@6wind.com> References: <1523017443-12414-1-git-send-email-declan.doherty@intel.com> <1523017443-12414-2-git-send-email-declan.doherty@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1523017443-12414-2-git-send-email-declan.doherty@intel.com> Subject: Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add group 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: Fri, 06 Apr 2018 20:32:43 -0000 On Fri, Apr 06, 2018 at 01:24:00PM +0100, Declan Doherty wrote: > Add new RTE_FLOW_ACTION_TYPE_GROUP_COUNT action type to enable shared > counters across multiple flows on a single port or across multiple > flows on multiple ports within the same switch domain. > > Introduce new API rte_flow_query_group_count to allow querying of group > counters. > > Signed-off-by: Declan Doherty Both features are definitely needed, however I suggest to enhance the existing action type and query function instead, given the rte_flow ABI won't be maintained for the 18.05 release [1]. Counters and query support were defined as a kind of PoC in preparation for future requirements back in DPDK 17.02 and so far few PMDs have implemented the query callback (mlx5 and failsafe, and the latter isn't really a PMD). Due to the behavior change of action lists [2], providing an action type as a query parameter is not specific enough anymore, for instance if a list contains multiple COUNT, the application should be able to tell which needs to be queried. Therefore I suggest to redefine the query function as follows: int rte_flow_query(uint16_t port_id, struct rte_flow *flow, const struct rte_flow_action *action, void *data, struct rte_flow_error *error); Third argument is an action definition with the same configuration (if any) as previously defined in the action list originally used to create the flow rule (not necessarily the same pointer, only the contents matter). It means two perfectly identical actions can't be distinguished, and that's how group counters will work. Instead of adding a new action type to distinguish groups, a configuration structure is added to the existing RTE_FLOW_ACTION_TYPE_COUNT, with non-shared counters as a default behavior: 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. */ }; Doing so will impact some existing code in mlx5 and librte_flow_classify, but that shouldn't be much of an issue. Keep in mind testpmd and its documentation must be updated as well. Thoughts? A few nits below for the sake of commenting. [1] "Flow API overhaul for switch offloads" http://dpdk.org/ml/archives/dev/2018-April/095774.html [2] "ethdev: alter behavior of flow API actions" http://dpdk.org/ml/archives/dev/2018-April/095779.html > --- > doc/guides/prog_guide/rte_flow.rst | 35 +++++++++++++++++++++ > lib/librte_ether/rte_ethdev_version.map | 8 +++++ > lib/librte_ether/rte_flow.c | 21 +++++++++++++ > lib/librte_ether/rte_flow.h | 56 ++++++++++++++++++++++++++++++++- > lib/librte_ether/rte_flow_driver.h | 6 ++++ > 5 files changed, 125 insertions(+), 1 deletion(-) > > diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst > index 961943d..fd33d19 100644 > --- a/doc/guides/prog_guide/rte_flow.rst > +++ b/doc/guides/prog_guide/rte_flow.rst > @@ -1698,6 +1698,41 @@ Return values: > > - 0 on success, a negative errno value otherwise and ``rte_errno`` is set. > > + Unnecessary empty line. > +Group Count Query > +~~~~~~~~~~~~~~~~~ > + > +Query group counter which can be associated with multiple flows on a specified > +port. > + > +This function allows retrieving of group counters. A group counter is a > +counter which can be shared among multiple flows on a single port or among > +multiple flows on multiple ports within the same switch domain. Data is > +gathered by special actions which must be present in the flow rule > +definition. > + > +.. code-block:: c > + > + int > + rte_flow_query_group_count(uint16_t port_id, > + uint32_t group_counter_id, > + struct rte_flow_query_count *count, > + struct rte_flow_error *error); > + > +Arguments: > + > +- ``port_id``: port identifier of Ethernet device. > +- ``group_counter_id``: group counter identifier. > +- ``count``: group counter parameters. > +- ``error``: perform verbose error reporting if not NULL. PMDs initialize > + this structure in case of error only. > + > +Return values: > + > +- 0 on success, a negative errno value otherwise and ``rte_errno`` is set. > + > + > + More unnecessary empty lines. > Isolated mode > ------------- > > diff --git a/lib/librte_ether/rte_ethdev_version.map b/lib/librte_ether/rte_ethdev_version.map > index 34df6c8..cff6807 100644 > --- a/lib/librte_ether/rte_ethdev_version.map > +++ b/lib/librte_ether/rte_ethdev_version.map > @@ -229,3 +229,11 @@ EXPERIMENTAL { > rte_mtr_stats_update; > > } DPDK_17.11; > + > + One more. > +EXPERIMENTAL { > + global: > + > + rte_flow_query_group_count > + > +} DPDK_18.05; > diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c > index 38f2d27..e10b1d0 100644 > --- a/lib/librte_ether/rte_flow.c > +++ b/lib/librte_ether/rte_flow.c > @@ -418,3 +418,24 @@ rte_flow_copy(struct rte_flow_desc *desc, size_t len, > } > return 0; > } > + > +int __rte_experimental > +rte_flow_query_group_count(uint16_t port_id, > + uint32_t group_count_id, > + struct rte_flow_query_count *count, > + struct rte_flow_error *error) This function lacks a short documentation comment (see rte_flow_query()). > +{ > + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error); > + > + if (!ops) > + return -rte_errno; > + if (likely(!!ops->query_group_count)) > + return flow_err(port_id, > + ops->query_group_count(dev, group_count_id, > + count, error), > + error); > + return rte_flow_error_set(error, ENOSYS, > + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > + NULL, rte_strerror(ENOSYS)); > +} > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > index 13e4202..7d1f89d 100644 > --- a/lib/librte_ether/rte_flow.h > +++ b/lib/librte_ether/rte_flow.h > @@ -1010,7 +1010,19 @@ enum rte_flow_action_type { > * > * See struct rte_flow_action_security. > */ > - RTE_FLOW_ACTION_TYPE_SECURITY > + RTE_FLOW_ACTION_TYPE_SECURITY, > + > + /** > + * Enable a shared flow group counter for flow. Group counters can be > + * associated with multiples flows on the same port or on port within > + * the same switch domain if supported by that device. > + * > + * Group counters can be retrieved and reset through > + * rte_flow_query_group_count() > + * > + * See struct rte_flow_action_group_count. > + */ > + RTE_FLOW_ACTION_TYPE_GROUP_COUNT Don't forget the trailing comma. > }; > > /** > @@ -1149,6 +1161,18 @@ struct rte_flow_action_security { > }; > > /** > + * RTE_FLOW_ACTION_TYPE_GROUP_COUNT > + * > + * A packet/byte counter which can be shared across a group of flows programmed > + * on the same port/switch domain. > + * > + * Non-terminating by default. > + */ > +struct rte_flow_action_group_count { > + uint32_t id; > +}; > + > +/** > * Definition of a single action. > * > * A list of actions is terminated by a END action. > @@ -1476,6 +1500,36 @@ rte_flow_copy(struct rte_flow_desc *fd, size_t len, > const struct rte_flow_item *items, > const struct rte_flow_action *actions); > > + Caught another empty line. > +/** > + * Get hit/bytes count for group counter. > + * > + * A group counter is a counter which can be shared among multiple flows on a > + * single port or among multiple flows on multiple ports within the same > + * switch domain. > + * > + * In the case of ports within the same switch domain a global name space is > + * assumed for group_count_id value. > + * > + * @param[in] port_id > + * Port identifier of Ethernet device. > + * @param[in] group_count_id > + * Group counter identifier to query > + * @param[out] count > + * Group counter value > + * @param[out] error > + * Perform verbose error reporting if not NULL. PMDs initialize this > + * structure in case of error only. > + * > + * @return > + * Negative error code (errno value) and rte_errno is set. > + */ > +int __rte_experimental > +rte_flow_query_group_count(uint16_t port_id, > + uint32_t group_count_id, > + struct rte_flow_query_count *count, > + struct rte_flow_error *error); > + > #ifdef __cplusplus > } > #endif > diff --git a/lib/librte_ether/rte_flow_driver.h b/lib/librte_ether/rte_flow_driver.h > index 7778c8e..ef09465 100644 > --- a/lib/librte_ether/rte_flow_driver.h > +++ b/lib/librte_ether/rte_flow_driver.h > @@ -96,6 +96,12 @@ struct rte_flow_ops { > (struct rte_eth_dev *, > int, > struct rte_flow_error *); > + /** See rte_flow_query_group_count(). */ > + int (*query_group_count) > + (struct rte_eth_dev *, > + uint32_t, > + struct rte_flow_query_count *, > + struct rte_flow_error *); > }; > > /** > -- > 2.7.4 > -- Adrien Mazarguil 6WIND