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 CE1EEA488 for ; Fri, 16 Feb 2018 12:17:27 +0100 (CET) Received: by mail-wr0-f196.google.com with SMTP id 34so2503298wre.13 for ; Fri, 16 Feb 2018 03:17:27 -0800 (PST) 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=/1BgUAgNot9waEQDMbI3ttNWnbeedXlN81wEHLEL23k=; b=XDkEujOgTgVi3GmtEmp6jWActH+x9lNhC+OC1BDDIrFjSv2KVlUiKfzswcEeeTMo0o iDYU9h6dd+huH2gK14GaTsymf4H28JJHGCaq8EFFFeb+ujniDUzDihqRYtDjrEaAHYtY VG1Do7fh4gVN10vUDtXzQCsCXRVzvAvQPp2+OA2I2v3pdHmetXT56lGk2PFkX159vutX w2uMIjti5qXMUZV5bs7HpAVWwqTQN48ngj3t3QSvWmUcGMyBwmBE547t+KkIQqaULkzM sUcn5Pv87Hi8KNCv1QdR5KjS1MCxr2L9opZsYV4GMGxlwOWMf6MwhVLCf/rjpB2stv4j gwGg== 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=/1BgUAgNot9waEQDMbI3ttNWnbeedXlN81wEHLEL23k=; b=tj6iREtmsCEOl63lCVxSJTywmT5Rb4/5QBSM/jMI9Y3qRjgMz1dFVxIgLI90iAIEIg as1GR2r3gvXbH0YD/T3z3IydCdl8UO6C8faBEJpeQ23F3AULldccRthzQ5lLoe7VM0CH +xT6ODbG9/4O3UcZdJc73incEmdpxCADw69gYWGZ/Ol/Htypc4P3ArxvRs1qWHjfLYtI hLCiUNyJB+MVcAmgewi+VBZ8rhxO8Cc+d6mYfovFvlNEIzQNJmt+LOtAcgL5elGGDLcf PdFhYZSyxLuaAXJOwHVD76m7qU2UG+MsawMtuki3Hj0kx5r3izsslmnz0To2yc2ikQlw F/Gw== X-Gm-Message-State: APf1xPBcPMLOcUVg268nvy0ZirbqOD4KpgOg4aE7/Y5OAhyA9nKl9XwF gQdZb/WtT5g+Cdc0DNEYvokkNw== X-Google-Smtp-Source: AH8x225P9q20+goI69XAX8EM7wCzIes/WhHXW5Md+m1wfhQEj0dQy3YJhHZmn5/Van9KJacOibSLLg== X-Received: by 10.223.196.129 with SMTP id m1mr5128946wrf.213.1518779847478; Fri, 16 Feb 2018 03:17:27 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 1sm25252766wru.19.2018.02.16.03.17.26 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 16 Feb 2018 03:17:26 -0800 (PST) Date: Fri, 16 Feb 2018 12:17:14 +0100 From: Adrien Mazarguil To: Nelio Laranjeiro Cc: dev@dpdk.org, Yongseok Koh Message-ID: <20180216111714.GD4256@6wind.com> References: <7aeb54018d1a8dd06638632076f25fbc88945afd.1518686582.git.nelio.laranjeiro@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7aeb54018d1a8dd06638632076f25fbc88945afd.1518686582.git.nelio.laranjeiro@6wind.com> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: change tunnel flow priority 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, 16 Feb 2018 11:17:27 -0000 A few commit log rewording suggestions for clarity and a couple of additional comments below, otherwise patch looks fine. On Thu, Feb 15, 2018 at 10:23:29AM +0100, Nelio Laranjeiro wrote: > Packet matching inner and outer flow rules are catch by the first one catch => caught > added in the device as both flows are configured with the same priority. > To avoid such situation, the inner flow can have an higher priority than in the device => to the device can => must? > the outer ones as their pattern matching will not collide. not => otherwise > > Signed-off-by: Nelio Laranjeiro > --- > drivers/net/mlx5/mlx5_flow.c | 72 +++++++++++++++++++++++++++----------------- > 1 file changed, 45 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c > index 26002c4b9..6223f1f94 100644 > --- a/drivers/net/mlx5/mlx5_flow.c > +++ b/drivers/net/mlx5/mlx5_flow.c > @@ -118,7 +118,7 @@ const struct hash_rxq_init hash_rxq_init[] = { > IBV_RX_HASH_SRC_PORT_TCP | > IBV_RX_HASH_DST_PORT_TCP), > .dpdk_rss_hf = ETH_RSS_NONFRAG_IPV4_TCP, > - .flow_priority = 0, > + .flow_priority = 1, > .ip_version = MLX5_IPV4, > }, > [HASH_RXQ_UDPV4] = { > @@ -127,7 +127,7 @@ const struct hash_rxq_init hash_rxq_init[] = { > IBV_RX_HASH_SRC_PORT_UDP | > IBV_RX_HASH_DST_PORT_UDP), > .dpdk_rss_hf = ETH_RSS_NONFRAG_IPV4_UDP, > - .flow_priority = 0, > + .flow_priority = 1, > .ip_version = MLX5_IPV4, > }, > [HASH_RXQ_IPV4] = { > @@ -135,7 +135,7 @@ const struct hash_rxq_init hash_rxq_init[] = { > IBV_RX_HASH_DST_IPV4), > .dpdk_rss_hf = (ETH_RSS_IPV4 | > ETH_RSS_FRAG_IPV4), > - .flow_priority = 1, > + .flow_priority = 2, > .ip_version = MLX5_IPV4, > }, > [HASH_RXQ_TCPV6] = { > @@ -144,7 +144,7 @@ const struct hash_rxq_init hash_rxq_init[] = { > IBV_RX_HASH_SRC_PORT_TCP | > IBV_RX_HASH_DST_PORT_TCP), > .dpdk_rss_hf = ETH_RSS_NONFRAG_IPV6_TCP, > - .flow_priority = 0, > + .flow_priority = 1, > .ip_version = MLX5_IPV6, > }, > [HASH_RXQ_UDPV6] = { > @@ -153,7 +153,7 @@ const struct hash_rxq_init hash_rxq_init[] = { > IBV_RX_HASH_SRC_PORT_UDP | > IBV_RX_HASH_DST_PORT_UDP), > .dpdk_rss_hf = ETH_RSS_NONFRAG_IPV6_UDP, > - .flow_priority = 0, > + .flow_priority = 1, > .ip_version = MLX5_IPV6, > }, > [HASH_RXQ_IPV6] = { > @@ -161,13 +161,13 @@ const struct hash_rxq_init hash_rxq_init[] = { > IBV_RX_HASH_DST_IPV6), > .dpdk_rss_hf = (ETH_RSS_IPV6 | > ETH_RSS_FRAG_IPV6), > - .flow_priority = 1, > + .flow_priority = 2, > .ip_version = MLX5_IPV6, > }, > [HASH_RXQ_ETH] = { > .hash_fields = 0, > .dpdk_rss_hf = 0, > - .flow_priority = 2, > + .flow_priority = 3, > }, > }; > > @@ -860,8 +860,6 @@ priv_flow_convert_items_validate(struct priv *priv, > * > * @param priv > * Pointer to private structure. > - * @param[in] priority > - * Flow priority. > * @param[in] size > * Amount of byte to allocate. > * @param[out] error > @@ -872,7 +870,6 @@ priv_flow_convert_items_validate(struct priv *priv, > */ > static struct ibv_flow_attr* > priv_flow_convert_allocate(struct priv *priv, > - unsigned int priority, > unsigned int size, > struct rte_flow_error *error) > { > @@ -887,11 +884,44 @@ priv_flow_convert_allocate(struct priv *priv, > "cannot allocate verbs spec attributes."); > return NULL; > } > - ibv_attr->priority = priority; > return ibv_attr; > } > > /** > + * Make Inner packet matching with an higher priority from the non Inner > + * matching. Inner => inner > + * > + * @param priv > + * Pointer to private structure. > + * @param[in, out] parser > + * Internal parser structure. > + * @param attr > + * User flow attribute. > + */ > +static void > +priv_flow_update_priority(struct priv *priv, struct mlx5_flow_parse *parser, > + const struct rte_flow_attr *attr) > +{ > + unsigned int i; > + > + (void)priv; I suggest dropping "priv" from everywhere in this function including its name. It's a static helper not even exposed, no need to be consistent with other priv* functions for the sake of consistency if the parameter is unnecessary. > + if (parser->drop) { > + parser->queue[HASH_RXQ_ETH].ibv_attr->priority = > + attr->priority + > + hash_rxq_init[HASH_RXQ_ETH].flow_priority; > + return; > + } > + for (i = 0; i != hash_rxq_init_n; ++i) { > + if (parser->queue[i].ibv_attr) { > + parser->queue[i].ibv_attr->priority = > + attr->priority + > + hash_rxq_init[i].flow_priority - > + (parser->inner ? 1 : 0); > + } > + } > +} On a personal note, I noticed how complex (convoluted?) mlx5 flow rule handling code turned lately due to all the added features, unforeseen issues and the resulting workarounds, partly caused by the limited number of available priorities on the Verbs side. I think we're nearing another major rewrite. > + > +/** > * Finalise verbs flow attributes. > * > * @param priv > @@ -1059,23 +1089,16 @@ priv_flow_convert(struct priv *priv, > * Allocate the memory space to store verbs specifications. > */ > if (parser->drop) { > - unsigned int priority = > - attr->priority + > - hash_rxq_init[HASH_RXQ_ETH].flow_priority; > unsigned int offset = parser->queue[HASH_RXQ_ETH].offset; > > parser->queue[HASH_RXQ_ETH].ibv_attr = > - priv_flow_convert_allocate(priv, priority, > - offset, error); > + priv_flow_convert_allocate(priv, offset, error); > if (!parser->queue[HASH_RXQ_ETH].ibv_attr) > return ENOMEM; > parser->queue[HASH_RXQ_ETH].offset = > sizeof(struct ibv_flow_attr); > } else { > for (i = 0; i != hash_rxq_init_n; ++i) { > - unsigned int priority = > - attr->priority + > - hash_rxq_init[i].flow_priority; > unsigned int offset; > > if (!(parser->rss_conf.rss_hf & > @@ -1084,8 +1107,7 @@ priv_flow_convert(struct priv *priv, > continue; > offset = parser->queue[i].offset; > parser->queue[i].ibv_attr = > - priv_flow_convert_allocate(priv, priority, > - offset, error); > + priv_flow_convert_allocate(priv, offset, error); > if (!parser->queue[i].ibv_attr) > goto exit_enomem; > parser->queue[i].offset = sizeof(struct ibv_flow_attr); > @@ -1120,13 +1142,9 @@ priv_flow_convert(struct priv *priv, > * Last step. Complete missing specification to reach the RSS > * configuration. > */ > - if (!parser->drop) { > + if (!parser->drop) > priv_flow_convert_finalise(priv, parser); > - } else { > - parser->queue[HASH_RXQ_ETH].ibv_attr->priority = > - attr->priority + > - hash_rxq_init[parser->layer].flow_priority; > - } > + priv_flow_update_priority(priv, parser, attr); > exit_free: > /* Only verification is expected, all resources should be released. */ > if (!parser->create) { > -- > 2.11.0 > -- Adrien Mazarguil 6WIND