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 3207A5F44 for ; Thu, 28 Jun 2018 14:33:38 +0200 (CEST) Received: by mail-wr0-f196.google.com with SMTP id p1-v6so5345289wrs.9 for ; Thu, 28 Jun 2018 05:33:38 -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=XKQF6xDt5/BiSvqh3+0pz2EbDbQiU+zszVqDGcRT2RA=; b=O7KnptsmP72dq5MhznzUlKNhGmHYMmF7oSTyWlCo+rY6DJslklsT9+0fHY32mFUDLG YcX9onYw+59ZCAXWNSp9o5dvLwuIRrwwrh2+tEKv1K3RSTcPjgCmnBN9Kr4qBd0wXIsg LrGTMJclXzQ/T5JFV/MPqQ6Poi+QWKf6ojzZGfgV+gbrn9ZK47Q2XBdKcmEoBdZgPnyY 1pWYZZs+TqXiIgkR+D1WmdMj+yS3YtoWvo9RWKITXX7x+SJYL7LmkzwARm2Apgud0ebj PtkzEXp+KLXVt4Z0wirUDtQhtGSzUnVIhrY4qFqa8CjN37PDBDlzhS+8lFGDE9ybrr3d sjpA== 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=XKQF6xDt5/BiSvqh3+0pz2EbDbQiU+zszVqDGcRT2RA=; b=rD5qcOSZ4vFqCfTsPRneGgwtT3keyiJpUH+3gb2+/csB0YzWgR7f6W2Tws3vn7TO26 anxJlftY0qiyo/yWFsRnXAOAs5Me7ApybY6JLY+ibxy6CPnMpq/7+szlZn7Ot9MKspw1 MR8ifs7n+wkrsAlr9Xmsixczkx0Dkak4sULCd8FAped0prWEv7rhP12pMo+FzggvPomF 5+7GkQO3cELYQTG3/7+gDBToNoDaz4m3Hfwp4N9YuyfUQy3UkwHux9S4cAFlzBgDOmFC J8HPI98fPPbhQg8ysZR19kLFweI/FAyVww2g7DMYjp4jqBkQ5HOEN9Pe0F+fnR8eXpMc hxfg== X-Gm-Message-State: APt69E0BujKd3kUlCNXsAlNAgNVOzEepb4tuKj6q7CTXfq2hps3Qv6gg teacRJZjG0FiWbDb6TH257mVe01S X-Google-Smtp-Source: AAOMgpd49mpOWZzmgM1vssJ6eJFczD0kmzQvUsRkxqVKNVbN11VubgJ0bQdUA1BXC/2NlvP8e/tokg== X-Received: by 2002:adf:ec4d:: with SMTP id w13-v6mr8378824wrn.222.1530189217768; Thu, 28 Jun 2018 05:33:37 -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 m58-v6sm12611473wrf.61.2018.06.28.05.33.36 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 28 Jun 2018 05:33:36 -0700 (PDT) Date: Thu, 28 Jun 2018 14:33:20 +0200 From: Adrien Mazarguil To: Nelio Laranjeiro Cc: dev@dpdk.org Message-ID: <20180628123320.GD4025@6wind.com> References: <15e22f72e9b4c56e79809c413ce3001e4f6067d8.1529565844.git.nelio.laranjeiro@6wind.com> <20180627145525.8659-1-nelio.laranjeiro@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180627145525.8659-1-nelio.laranjeiro@6wind.com> Subject: Re: [dpdk-dev] [PATCH v4] ethdev: add flow API to expand RSS flows 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, 28 Jun 2018 12:33:38 -0000 On Wed, Jun 27, 2018 at 04:55:25PM +0200, Nelio Laranjeiro wrote: > Introduce an helper for PMD to expand easily flows items list with RSS > action into multiple flow items lists with priority information. > > For instance a user items list being "eth / end" with rss action types > "ipv4-udp ipv6-udp end" needs to be expanded into three items lists: > > - eth > - eth / ipv4 / udp > - eth / ipv6 / udp > > to match the user request. Some drivers are unable to reach such > request without this expansion, this API is there to help those. > Only PMD should use such API for their internal cooking, the application > will still handle a single flow. > > Signed-off-by: Nelio Laranjeiro > --- > > Changes in v4: > > - Replace the expanded algorithm with a graph to support also tunnel > pattern matching. Looks like a useful helper, nice to see that it's much shorter than the previous versions. A few nits below, mainly suggestions for documentation. > + > +int > +rte_flow_expand_rss(struct rte_flow_expand_rss *buf, size_t size, > + const struct rte_flow_item *pat, uint64_t types, > + const struct rte_flow_expand_node nodes[], > + const int node_entry_index) Please add short documentation reminder on top of this definition (i.e. a copy of the first sentence from rte_flow_driver.h). > +{ > + const int elt_n = 8; > + const struct rte_flow_item *item; > + const struct rte_flow_expand_node *node = &nodes[node_entry_index]; > + const int *next_node; > + const int *stack[elt_n]; > + int stack_n = 0; > + struct rte_flow_item flow_items[elt_n]; > + unsigned int i; > + size_t lsize; > + size_t user_pattern_size = 0; > + void *addr = NULL; > + > + lsize = sizeof(*buf) + > + /* Size for the list of patterns. */ > + sizeof(*buf->patterns) + > + RTE_ALIGN_CEIL(elt_n * sizeof(*item), sizeof(void *)) + > + /* Size for priorities. */ > + sizeof(*buf->priority) + > + RTE_ALIGN_CEIL(elt_n * sizeof(uint32_t), sizeof(void *)); > + if (lsize <= size) { > + buf->priority = (void *)(buf + 1); > + buf->priority[0] = 0; > + buf->patterns = (void *)&buf->priority[elt_n]; > + buf->patterns[0] = (void *)&buf->patterns[elt_n]; > + buf->entries = 0; > + addr = buf->patterns[0]; > + } > + for (item = pat; item->type != RTE_FLOW_ITEM_TYPE_END; item++) { > + const struct rte_flow_expand_node *next = NULL; > + > + for (i = 0; node->next && node->next[i]; ++i) { > + next = &nodes[node->next[i]]; > + if (next->type == item->type) > + break; > + } > + if (next) > + node = next; > + user_pattern_size += sizeof(*item); > + } > + user_pattern_size += sizeof(*item); /**< Handle END item. */ This comment shouldn't be in Doxygen format. > + lsize += user_pattern_size; > + /* Copy the user pattern in the first entry of the buffer. */ > + if (lsize <= size) { > + rte_memcpy(addr, pat, user_pattern_size); > + addr = (void *)(((uintptr_t)addr) + user_pattern_size); > + buf->priority[buf->entries] = 0; > + buf->entries = 1; > + } > + /* Start expanding. */ > + memset(flow_items, 0, sizeof(flow_items)); > + user_pattern_size -= sizeof(*item); > + next_node = node->next; > + stack[stack_n] = next_node; > + node = next_node ? &nodes[*next_node] : NULL; > + while (node) { > + flow_items[stack_n].type = node->type; > + if ((node->rss_types & types) == node->rss_types) { > + /* > + * compute the number of items to copy from the > + * expansion and copy it. > + * When the stack_n is 0, there are 1 element in it, > + * plus the addition END item. > + */ > + int elt = stack_n + 2; > + > + flow_items[stack_n + 1].type = RTE_FLOW_ITEM_TYPE_END; > + lsize += elt * sizeof(*item) + user_pattern_size; > + if (lsize <= size) { > + size_t n = elt * sizeof(*item); > + > + buf->priority[buf->entries] = stack_n + 1; > + buf->patterns[buf->entries++] = addr; > + rte_memcpy(addr, buf->patterns[0], > + user_pattern_size); > + addr = (void *)(((uintptr_t)addr) + > + user_pattern_size); > + rte_memcpy(addr, flow_items, n); > + addr = (void *) (((uintptr_t)addr) + n); Extra space after (void *). > + } > + } > + /* Go deeper. */ > + if (node->next) { > + next_node = node->next; > + stack[++stack_n] = next_node; Since stack[] contains at most elt_n (8) elements, even assuming it's always plenty enough, I think it would be wise to check for any overflow before incrementing stack_n and return an error if so. > + } else if (*(next_node + 1)) { > + /* Follow up with the next possibility. */ > + ++next_node; > + } else { > + /* Move to the next path. */ > + if (stack_n) > + next_node = stack[--stack_n]; > + next_node++; > + stack[stack_n] = next_node; > + } > + node = *next_node ? &nodes[*next_node] : NULL; > + }; > + return lsize; > +} > diff --git a/lib/librte_ethdev/rte_flow_driver.h b/lib/librte_ethdev/rte_flow_driver.h > index 1c90c600d..538b46e54 100644 > --- a/lib/librte_ethdev/rte_flow_driver.h > +++ b/lib/librte_ethdev/rte_flow_driver.h > @@ -114,6 +114,54 @@ struct rte_flow_ops { > const struct rte_flow_ops * > rte_flow_ops_get(uint16_t port_id, struct rte_flow_error *error); > > +/** Macro to create the expansion graph. */ => Helper macro to build input graph for rte_flow_expand_rss(). Mostly rewording suggestions from here on. > +#define RTE_FLOW_EXPAND_ITEMS(...) \ You should keep the same prefix for all objects associated with this function. Everything should start with "rte_flow_expand_rss" for consistency. => RTE_FLOW_EXPAND_RSS_NEXT(...) > + (const int []){ \ > + __VA_ARGS__, 0, \ > + } > + > +/** structure to generate the expansion graph. */ => Node object of input graph for rte_flow_expand_rss(). > +struct rte_flow_expand_node { => struct rte_flow_expand_rss_node > + const int *const next; /**< list of items finalised by 0. */ => List of next node indexes. Index 0 is interpreted as a terminator. > + const enum rte_flow_item_type type; /**< Item type to add. */ => Pattern item type of current node. > + uint64_t rss_types; /**< RSS bit-field value. */ => RSS types bit-field associated with this node (see ETH_RSS_* definitions). > +}; > + > +/** > + * Expansion structure for RSS flows. > + */ => Object returned by rte_flow_expand_rss(). This block could fit a single line by the way. > +struct rte_flow_expand_rss { > + uint32_t entries; /**< Number of entries in the following arrays. */ => Number of entries in @p patterns and @p priorities. > + struct rte_flow_item **patterns; /**< Expanded pattern array. */ > + uint32_t *priority; /**< Priority offset for each expansion. */ How about priority => priorities since there are as many of them as patterns? > +}; Another suggestion regarding this structure definition, since it's entirely written by rte_flow_expand_rss() based on an arbitrarily-sized user-provided buffer, and given it's not a public API, a flexible array member could make sense. This would highlight the link between patterns and priorities and make the result more convenient to use thanks to fewer pointers: struct rte_flow_expand_rss { uint32_t entries; struct { struct rte_flow_item *pattern; uint32_t priority; } entry[]; }; > + > +/** > + * Expand RSS flows into several possible flows according to the RSS hash > + * fields requested and the driver capabilities. > + * > + * @param[in,out] buf Since this buffer is only written to: @param[in,out] => @param[out] > + * Buffer to store the result expansion. => Buffer for expansion result. May be truncated if @p size is not large enough. > + * @param[in] size > + * Size in octets of the buffer. => Buffer size in bytes. If 0, @p buf can be NULL. > + * @param[in] pat pat => pattern > + * User flow pattern. > + * @param[in] types > + * RSS types expected (see ETH_RSS_*). => RSS types to expand (see ETH_RSS_* definitions). > + * @param[in] nodes. nodes => graph > + * Expansion graph of possibilities for the RSS. => Input graph to expand @p pattern according to @p types. > + * @param[in] node_entry_index "[in]" is unnecessary since it's not a pointer. node_entry_index => graph_root_index > + * The index in the \p nodes array as start point. Let's keep "@" instead of "\" for directives. => Index of root node in @p graph, typically 0. > + * > + * @return > + * The size in octets used to expand. => A positive value representing the size of @p buf in bytes regardless of @p size on success, a negative errno value otherwise and rte_errno is set. > + */ > +int > +rte_flow_expand_rss(struct rte_flow_expand_rss *buf, size_t size, > + const struct rte_flow_item *pat, uint64_t types, pat => pattern > + const struct rte_flow_expand_node nodes[], > + const int node_entry_index); No need for node_entry_index to be const. > + > #ifdef __cplusplus > } > #endif > -- > 2.18.0 > -- Adrien Mazarguil 6WIND