From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f196.google.com (mail-wr0-f196.google.com [209.85.128.196]) by dpdk.org (Postfix) with ESMTP id 1349C1B1A2 for ; Thu, 19 Apr 2018 15:04:04 +0200 (CEST) Received: by mail-wr0-f196.google.com with SMTP id d1-v6so13837159wrj.13 for ; Thu, 19 Apr 2018 06:04:04 -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=WqimYR96YoPJHwTp/LYlaKARfD/xytFQ/f4tnYAzFb0=; b=x7Ro3PJ4l2x/LyyDNAOdD6LbrNiZTIpsAfde26MtQk5Qw0CJB2Zff0z7drIhQj1rT9 o/XJYbqHqnvIY/s0wXFFcrKX69Irop7I72q9sUp8eJn/8sXqJPuukJi0N4tHndFhZtD2 aCGaHlYO4fjgG2FNeYLJaC6t9V03lu/C2hezs4leZruxg7vY3kSYl8oaXP1r9S2GV7qq 0blN5ThiCTsklylXMYXvbwad7qA4sOrvPvYfWjZkgj807X/cRnBqd5xIzWSQF8K8AhWv dTX4dtlAuxa672E8zaE4GI8MDz8aDvl+qpDw1SIrXQ88HAgF7X6D8a+W6xk9bH6ybkWU ZUyg== 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=WqimYR96YoPJHwTp/LYlaKARfD/xytFQ/f4tnYAzFb0=; b=QgowYejL12laBwXYIwGdmmgIH41Ns+B8dxI3jJcXrwqq9qYnLM+IuQctNkZdePZIbN coqhhhAp655CzNFstK1A+uJZ3AnrkHy94cJAH0tOpN3rxrrst90u9i+QitbmwWTlcU1Y PiMyH/4t245TGOjxKSEMTSqDnhd7I4XIv2bWuWPDDDduiuJPewIuDfwv56K9Pto+OjFj +ipOYJm7EXLC1CTHARWqFWNl1URYF98kqNgc6eY5/xw9T3OmIuFFHOE46Uxw2NH84VLp 4ZqOSmQusVdif4x5HB2biJF7KdTUuL/RZhzbtiVtzn+2UKfEF21Fcd+8kLJAIWVI5Ms3 GBmA== X-Gm-Message-State: ALQs6tCOkdgqGM2JHqAmxD6ZvFCcTp5n2N60kFJkJj1BG6dNHHHb9ZS/ tvXWZOrHAdlvLsytmtHgKwWPeA== X-Google-Smtp-Source: AIpwx4/d0NR+FO5wCo1ROZc9xLOwI9BZcx/+LHp1FxwaJFWu6ZMJ63yEkNStesU6byJtP99KVWcxpA== X-Received: by 2002:adf:e312:: with SMTP id b18-v6mr4551541wrj.247.1524143043636; Thu, 19 Apr 2018 06:04:03 -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 u13-v6sm3442248wrg.9.2018.04.19.06.04.02 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 19 Apr 2018 06:04:03 -0700 (PDT) Date: Thu, 19 Apr 2018 15:03:49 +0200 From: Adrien Mazarguil To: Declan Doherty Cc: dev@dpdk.org, Alex Rosenbaum , Ferruh Yigit , Thomas Monjalon , Shahaf Shuler , Qi Zhang , Alejandro Lucero , Andrew Rybchenko , Mohammad Abdul Awal , Remy Horton , John McNamara , Rony Efraim , Jingjing Wu , Wenzhuo Lu , Vincent Jardin , Yuanhan Liu , Bruce Richardson , Konstantin Ananyev , Zhihong Wang Message-ID: <20180419130349.GD4957@6wind.com> References: <1523017443-12414-1-git-send-email-declan.doherty@intel.com> <20180418210423.13847-1-declan.doherty@intel.com> <20180418210423.13847-7-declan.doherty@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180418210423.13847-7-declan.doherty@intel.com> Subject: Re: [dpdk-dev] [PATCH v4 6/6] 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, 19 Apr 2018 13:04:04 -0000 On Wed, Apr 18, 2018 at 10:04:23PM +0100, Declan Doherty wrote: > 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. > > 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 failsafe PMD and > testpmd application which are affected by this API change. > > Signed-off-by: Declan Doherty Patch is almost ready, please see the bunch of nits below. > --- > 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 | 52 ++++++++++++++++++++++-------------- > drivers/net/failsafe/failsafe_flow.c | 4 +-- > lib/librte_ether/rte_flow.c | 2 +- > lib/librte_ether/rte_flow.h | 30 ++++++++++++++++++--- > lib/librte_ether/rte_flow_driver.h | 2 +- > 8 files changed, 75 insertions(+), 38 deletions(-) > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c > index e6284ce11..cec9885d5 100644 > --- a/app/test-pmd/cmdline_flow.c > +++ b/app/test-pmd/cmdline_flow.c > @@ -347,7 +347,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; > @@ -874,7 +874,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, > @@ -2972,7 +2972,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 dd051f5ca..2d4ca840d 100644 > --- a/app/test-pmd/config.c > +++ b/app/test-pmd/config.c > @@ -1342,7 +1342,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; > @@ -1364,15 +1364,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. */ > @@ -1380,7 +1381,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" > @@ -1395,7 +1396,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 98d84da84..a8a046135 100644 > --- a/app/test-pmd/testpmd.h > +++ b/app/test-pmd/testpmd.h > @@ -603,7 +603,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 6f23ad909..9f9a16068 100644 > --- a/doc/guides/prog_guide/rte_flow.rst > +++ b/doc/guides/prog_guide/rte_flow.rst > @@ -1078,23 +1078,24 @@ consider all actions to be performed simultaneously: > | 2 | END | > +-------+--------+ > > -| This should stay. It's a special separator to untangle successive tables when generating documentaton. > > .. _table_rte_flow_mark_count_redirect: > > .. table:: Mark, count and 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 | > + +-------+-----------------------------+ > > | > > @@ -1321,17 +1322,28 @@ These 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. A shared counter > +scope is within that of the port the flow rule is programmed, with the exception > +of ports which share a common switch domain, it that case the shared counter > +scope is within all the ports in that switch domain. > + > +If more that one count action is specified on a single flow rule then each count > +action must specify a unique counter id. > + Extraneous empty line. > > .. _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: > > @@ -1820,7 +1832,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); > > @@ -1828,7 +1840,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/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 a3823d874..516e6530e 100644 > --- a/lib/librte_ether/rte_flow.c > +++ b/lib/librte_ether/rte_flow.c > @@ -201,7 +201,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 56e262a23..f58e96060 100644 > --- a/lib/librte_ether/rte_flow.h > +++ b/lib/librte_ether/rte_flow.h > @@ -995,7 +995,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, > > @@ -1104,6 +1104,30 @@ struct rte_flow_action_queue { > uint16_t index; /**< Queue index to use. */ > }; > > + Extraneous empty line. > +/** > + * RTE_FLOW_ACTION_TYPE_COUNT > + * > + * Add 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. > + * > + * 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. Please keep this documentation synchronized with rte_flow.rst. > + * Extraneous line. > + */ > +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) > * > @@ -1467,7 +1491,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 Missing "." > * @param[in, out] data > * Pointer to storage for the associated query data type. > * @param[out] error > @@ -1480,7 +1504,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 > -- Adrien Mazarguil 6WIND