From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f169.google.com (mail-wr0-f169.google.com [209.85.128.169]) by dpdk.org (Postfix) with ESMTP id 002F11094 for ; Thu, 6 Jul 2017 19:11:10 +0200 (CEST) Received: by mail-wr0-f169.google.com with SMTP id c11so11346331wrc.3 for ; Thu, 06 Jul 2017 10:11:10 -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:content-transfer-encoding:in-reply-to :user-agent; bh=JFIowhwx2TBbW/FwyVCYz8aFEYAXbG0hgSOETtm+W+k=; b=zDf6I89M1vYpXquzisAlqhmJaWif3QX6JzjEGZm8CYbSZCq9xv8QxufvdCNqJt83ei wmrNP9CipmobFF2JqwaFVJNgMSzUnkwWzovhm6PxU3r/zuSgAMyIMhi23xjaWCbiOxZh Jigr3OQVFSlGOTu1OAiCRE0fIfQWr+qBfh7el1wrjr7tCAHkQ0x8249AEuUzhOLOCxOz 7xvDHTT44tNyCyswscHOJlfiLXRzDUNRkVtzu88O2NAYifMaqMadI6togQ8nZdz7GTIt anf40EBvs890y7oENBL3gRTHH9OPfUJlZ7DAfhmJtuAjRw+sp6uuE5pJMu1xEJsABYtQ jYDA== 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:content-transfer-encoding :in-reply-to:user-agent; bh=JFIowhwx2TBbW/FwyVCYz8aFEYAXbG0hgSOETtm+W+k=; b=dtYKeVMbMjsvJC+nWFHdWYlt4JLd/ayc7lqGyLNPo26pRRZgn+QaUrbtKv/yl/YwNo 2Hq52ycdPObGO/pSkSuC10OvyC6ZfRa8+sRuukKLv54u0b/AITeljwf25y+gs1mRVuc8 S2atPYCH/qwSq5qq+xvGf8/oJyMYBNahmRlBOuvKwx2kFt9fCxrGa4QCcuOZjjJ2LDEm ASt8BWwAyr3zMOXDSRDABvmefrUqaGfSQSmr0XfygY8072Cw/TXT1KKAj15rQ7Nhq+TZ Ap3giGLmfBVdyEEnSQJcGvLpnTTXmlS1tuhihftJXSY7m2Xz3RHOWeU2tsKlbXYJCXKz 355A== X-Gm-Message-State: AIVw110GKWUsinnv3aurKzAqGkqS5hhTZOu3v6DQn/LNkBIAyw4duxYb /TjZ5DVdMMmR8d50 X-Received: by 10.28.153.81 with SMTP id b78mr126627wme.85.1499361070354; Thu, 06 Jul 2017 10:11:10 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id q70sm819926wrb.3.2017.07.06.10.11.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 06 Jul 2017 10:11:09 -0700 (PDT) Date: Thu, 6 Jul 2017 19:11:01 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Adrien Mazarguil Cc: Thomas Monjalon , dev@dpdk.org Message-ID: <20170706171101.GN11154@bidouze.vm.6wind.com> References: <4695f0b4603308f6e27a0f2518945172989165fd.1499360068.git.adrien.mazarguil@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4695f0b4603308f6e27a0f2518945172989165fd.1499360068.git.adrien.mazarguil@6wind.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v3] ethdev: add flow API rule copy function 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, 06 Jul 2017 17:11:11 -0000 Hi Adrien, On Thu, Jul 06, 2017 at 06:56:54PM +0200, Adrien Mazarguil wrote: > From: Gaetan Rivet > > This allows PMDs and applications to save flow rules in their generic > format for later processing. This is useful when rules cannot be applied > immediately, such as when the device is not properly initialized. > > Signed-off-by: Gaetan Rivet > Signed-off-by: Adrien Mazarguil > > --- > > Gaetan, sorry for taking over and modifying your patch, took me a while to > review it and given there is not much time left before RC1, here is my > suggestion in the form of an updated patch for reasons described below. > > I'm convinced exposing rte_flow_copy() is useful and mandatory not only for > the fail-safe PMD, but also to avoid code duplication later. > > However since you took this code from testpmd, you've inherited its main > drawback which is rte_flow_desc_item[] and rte_flow_desc_action[] are a > pain to maintain. Every time new flow item/action are added, one has to > update these arrays as well. > > Moreover it forces you to expose and version a bunch of additional symbols > so far only useful to testpmd. > > Therefore I'm thinking about generating these arrays automatically at > compilation time from enum definitions and not expose extra symbols at the > same time. I'll try to submit these changes before the next RC. > That would be easier to maintain but I'd be curious to see an elegant solution to this issue. > So in the meantime, here's a v3 in order not to break existing series that > depend on this patch and not introduce unnecessary symbols. > Thanks, however E_TAG and NVGRE are not covered (I should have updated my version after the remark from Thomas). I will send a v4 shortly including those. > v2 -> v3: > > * Revert testpmd changes. > * Do not expose extra symbols (rte_flow_desc_action, rte_flow_desc_item, > rte_flow_nb_action and rte_flow_nb_item). > * Do not expose struct rte_flow_desc_data. > * Add missing #include directives to rte_flow.c. > > v1 -> v2: > > * fix checkpatch warnings > --- > lib/librte_ether/rte_ether_version.map | 1 + > lib/librte_ether/rte_flow.c | 225 ++++++++++++++++++++++++++++ > lib/librte_ether/rte_flow.h | 40 +++++ > 3 files changed, 266 insertions(+) > > diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map > index 019a93d..6f65f83 100644 > --- a/lib/librte_ether/rte_ether_version.map > +++ b/lib/librte_ether/rte_ether_version.map > @@ -152,6 +152,7 @@ DPDK_17.08 { > global: > > _rte_eth_dev_callback_process; > + rte_flow_copy; > rte_flow_isolate; > > } DPDK_17.05; > diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c > index c1de31b..59ca85a 100644 > --- a/lib/librte_ether/rte_flow.c > +++ b/lib/librte_ether/rte_flow.c > @@ -31,14 +31,79 @@ > * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > */ > > +#include > +#include > #include > +#include > > +#include > #include > #include > #include "rte_ethdev.h" > #include "rte_flow_driver.h" > #include "rte_flow.h" > > +/** > + * Flow elements description tables. > + */ > +struct rte_flow_desc_data { > + const char *name; > + size_t size; > +}; > + > +/** Generate flow_item[] entry. */ > +#define MK_FLOW_ITEM(t, s) \ > + [RTE_FLOW_ITEM_TYPE_ ## t] = { \ > + .name = # t, \ > + .size = s, \ > + } > + > +/** Information about known flow pattern items. */ > +static const struct rte_flow_desc_data rte_flow_desc_item[] = { > + MK_FLOW_ITEM(END, 0), > + MK_FLOW_ITEM(VOID, 0), > + MK_FLOW_ITEM(INVERT, 0), > + MK_FLOW_ITEM(ANY, sizeof(struct rte_flow_item_any)), > + MK_FLOW_ITEM(PF, 0), > + MK_FLOW_ITEM(VF, sizeof(struct rte_flow_item_vf)), > + MK_FLOW_ITEM(PORT, sizeof(struct rte_flow_item_port)), > + MK_FLOW_ITEM(RAW, sizeof(struct rte_flow_item_raw)), /* +pattern[] */ > + MK_FLOW_ITEM(ETH, sizeof(struct rte_flow_item_eth)), > + MK_FLOW_ITEM(VLAN, sizeof(struct rte_flow_item_vlan)), > + MK_FLOW_ITEM(IPV4, sizeof(struct rte_flow_item_ipv4)), > + MK_FLOW_ITEM(IPV6, sizeof(struct rte_flow_item_ipv6)), > + MK_FLOW_ITEM(ICMP, sizeof(struct rte_flow_item_icmp)), > + MK_FLOW_ITEM(UDP, sizeof(struct rte_flow_item_udp)), > + MK_FLOW_ITEM(TCP, sizeof(struct rte_flow_item_tcp)), > + MK_FLOW_ITEM(SCTP, sizeof(struct rte_flow_item_sctp)), > + MK_FLOW_ITEM(VXLAN, sizeof(struct rte_flow_item_vxlan)), > + MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)), > + MK_FLOW_ITEM(GRE, sizeof(struct rte_flow_item_gre)), > +}; > + > +/** Generate flow_action[] entry. */ > +#define MK_FLOW_ACTION(t, s) \ > + [RTE_FLOW_ACTION_TYPE_ ## t] = { \ > + .name = # t, \ > + .size = s, \ > + } > + > +/** Information about known flow actions. */ > +static const struct rte_flow_desc_data rte_flow_desc_action[] = { > + MK_FLOW_ACTION(END, 0), > + MK_FLOW_ACTION(VOID, 0), > + MK_FLOW_ACTION(PASSTHRU, 0), > + MK_FLOW_ACTION(MARK, sizeof(struct rte_flow_action_mark)), > + MK_FLOW_ACTION(FLAG, 0), > + MK_FLOW_ACTION(QUEUE, sizeof(struct rte_flow_action_queue)), > + MK_FLOW_ACTION(DROP, 0), > + MK_FLOW_ACTION(COUNT, 0), > + MK_FLOW_ACTION(DUP, sizeof(struct rte_flow_action_dup)), > + MK_FLOW_ACTION(RSS, sizeof(struct rte_flow_action_rss)), /* +queue[] */ > + MK_FLOW_ACTION(PF, 0), > + MK_FLOW_ACTION(VF, sizeof(struct rte_flow_action_vf)), > +}; > + > /* Get generic flow operations structure from a port. */ > const struct rte_flow_ops * > rte_flow_ops_get(uint8_t port_id, struct rte_flow_error *error) > @@ -175,3 +240,163 @@ rte_flow_isolate(uint8_t port_id, > RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > NULL, rte_strerror(ENOSYS)); > } > + > +/** Compute storage space needed by item specification. */ > +static void > +flow_item_spec_size(const struct rte_flow_item *item, > + size_t *size, size_t *pad) > +{ > + if (!item->spec) > + goto empty; > + switch (item->type) { > + union { > + const struct rte_flow_item_raw *raw; > + } spec; > + > + /* Not a fall-through */ > + case RTE_FLOW_ITEM_TYPE_RAW: > + spec.raw = item->spec; > + *size = offsetof(struct rte_flow_item_raw, pattern) + > + spec.raw->length * sizeof(*spec.raw->pattern); > + break; > + default: > +empty: > + *size = 0; > + break; > + } > + *pad = RTE_ALIGN_CEIL(*size, sizeof(double)) - *size; > +} > + > +/** Compute storage space needed by action configuration. */ > +static void > +flow_action_conf_size(const struct rte_flow_action *action, > + size_t *size, size_t *pad) > +{ > + if (!action->conf) > + goto empty; > + switch (action->type) { > + union { > + const struct rte_flow_action_rss *rss; > + } conf; > + > + /* Not a fall-through. */ > + case RTE_FLOW_ACTION_TYPE_RSS: > + conf.rss = action->conf; > + *size = offsetof(struct rte_flow_action_rss, queue) + > + conf.rss->num * sizeof(*conf.rss->queue); > + break; > + default: > +empty: > + *size = 0; > + break; > + } > + *pad = RTE_ALIGN_CEIL(*size, sizeof(double)) - *size; > +} > + > +/** Store a full rte_flow description. */ > +size_t > +rte_flow_copy(struct rte_flow_desc *desc, size_t len, > + const struct rte_flow_attr *attr, > + const struct rte_flow_item *items, > + const struct rte_flow_action *actions) > +{ > + struct rte_flow_desc *fd = NULL; > + size_t tmp; > + size_t pad; > + size_t off1 = 0; > + size_t off2 = 0; > + size_t size = 0; > + > +store: > + if (items) { > + const struct rte_flow_item *item; > + > + item = items; > + if (fd) > + fd->items = (void *)&fd->data[off1]; > + do { > + struct rte_flow_item *dst = NULL; > + > + if ((size_t)item->type >= > + RTE_DIM(rte_flow_desc_item) || > + !rte_flow_desc_item[item->type].name) { > + rte_errno = ENOTSUP; > + return 0; > + } > + if (fd) > + dst = memcpy(fd->data + off1, item, > + sizeof(*item)); > + off1 += sizeof(*item); > + flow_item_spec_size(item, &tmp, &pad); > + if (item->spec) { > + if (fd) > + dst->spec = memcpy(fd->data + off2, > + item->spec, tmp); > + off2 += tmp + pad; > + } > + if (item->last) { > + if (fd) > + dst->last = memcpy(fd->data + off2, > + item->last, tmp); > + off2 += tmp + pad; > + } > + if (item->mask) { > + if (fd) > + dst->mask = memcpy(fd->data + off2, > + item->mask, tmp); > + off2 += tmp + pad; > + } > + off2 = RTE_ALIGN_CEIL(off2, sizeof(double)); > + } while ((item++)->type != RTE_FLOW_ITEM_TYPE_END); > + off1 = RTE_ALIGN_CEIL(off1, sizeof(double)); > + } > + if (actions) { > + const struct rte_flow_action *action; > + > + action = actions; > + if (fd) > + fd->actions = (void *)&fd->data[off1]; > + do { > + struct rte_flow_action *dst = NULL; > + > + if ((size_t)action->type >= > + RTE_DIM(rte_flow_desc_action) || > + !rte_flow_desc_action[action->type].name) { > + rte_errno = ENOTSUP; > + return 0; > + } > + if (fd) > + dst = memcpy(fd->data + off1, action, > + sizeof(*action)); > + off1 += sizeof(*action); > + flow_action_conf_size(action, &tmp, &pad); > + if (action->conf) { > + if (fd) > + dst->conf = memcpy(fd->data + off2, > + action->conf, tmp); > + off2 += tmp + pad; > + } > + off2 = RTE_ALIGN_CEIL(off2, sizeof(double)); > + } while ((action++)->type != RTE_FLOW_ACTION_TYPE_END); > + } > + if (fd != NULL) > + return size; > + off1 = RTE_ALIGN_CEIL(off1, sizeof(double)); > + tmp = RTE_ALIGN_CEIL(offsetof(struct rte_flow_desc, data), > + sizeof(double)); > + size = tmp + off1 + off2; > + if (size > len) > + return size; > + fd = desc; > + if (fd != NULL) { > + *fd = (const struct rte_flow_desc) { > + .size = size, > + .attr = *attr, > + }; > + tmp -= offsetof(struct rte_flow_desc, data); > + off2 = tmp + off1; > + off1 = tmp; > + goto store; > + } > + return 0; > +} > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > index cfbed30..6ac7cdb 100644 > --- a/lib/librte_ether/rte_flow.h > +++ b/lib/librte_ether/rte_flow.h > @@ -1266,6 +1266,46 @@ rte_flow_query(uint8_t port_id, > int > rte_flow_isolate(uint8_t port_id, int set, struct rte_flow_error *error); > > +/** > + * Generic flow representation. > + * > + * This form is sufficient to describe an rte_flow independently from any > + * PMD implementation and allows for replayability and identification. > + */ > +struct rte_flow_desc { > + size_t size; /**< Allocated space including data[]. */ > + struct rte_flow_attr attr; /**< Attributes. */ > + struct rte_flow_item *items; /**< Items. */ > + struct rte_flow_action *actions; /**< Actions. */ > + uint8_t data[]; /**< Storage for items/actions. */ > +}; > + > +/** > + * Copy an rte_flow rule description. > + * > + * @param[in] fd > + * Flow rule description. > + * @param[in] len > + * Total size of allocated data for the flow description. > + * @param[in] attr > + * Flow rule attributes. > + * @param[in] items > + * Pattern specification (list terminated by the END pattern item). > + * @param[in] actions > + * Associated actions (list terminated by the END action). > + * > + * @return > + * If len is greater or equal to the size of the flow, the total size of the > + * flow description and its data. > + * If len is lower than the size of the flow, the number of bytes that would > + * have been written to desc had it been sufficient. Nothing is written. > + */ > +size_t > +rte_flow_copy(struct rte_flow_desc *fd, size_t len, > + const struct rte_flow_attr *attr, > + const struct rte_flow_item *items, > + const struct rte_flow_action *actions); > + > #ifdef __cplusplus > } > #endif > -- > 2.1.4 > -- Gaëtan Rivet 6WIND