From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f68.google.com (mail-wm0-f68.google.com [74.125.82.68]) by dpdk.org (Postfix) with ESMTP id CC79C1C39C for ; Thu, 5 Jul 2018 10:47:37 +0200 (CEST) Received: by mail-wm0-f68.google.com with SMTP id p11-v6so9440090wmc.4 for ; Thu, 05 Jul 2018 01:47:37 -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=+DNVQBoU+N84nOn7yZLJ7wH6dn6UEL7Q+s9J2HDe/nU=; b=n6CntZj6mLir7k0nn9TVhkw//imhPKMiPUtSqXoGFmrCLXXZDEqCAuSnq1JevmUM84 ZQtUCjoM/NcBdzaeo8oq2HPvJoqwiFNdbpTNX01WkeRCeNJhukuwu9Qq+nNkSvE4srUI EGd4BWI8b5wfppp83RH1VVCxpbs2oBw3ZZfKjUYHS3dbEkL5lwIHxwFF9piXxVcR2f1f t5xT5IxegqCJK9drH4D9PrH7r3xuOwkMUI6tImD/BzqhKRUji8gnJk4/1XgkJMRDUitI zveKc1MZCaCrApfpkIkYu0tFKLLw35ZDSATYGiJRoZwHiEvIFojWWnn1W/o8UOBFouhp jnDw== 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=+DNVQBoU+N84nOn7yZLJ7wH6dn6UEL7Q+s9J2HDe/nU=; b=ryLaJ12y62PsM9sXPVdK/HF5kSYcXqs5R2pxT1+hNV6Qx7931W/f7+0QA0CLmPS9a3 T25vBzE2bYI018g3qWJRxanTzqCtGtHANUrfwYJhdEC1uIrLVYS5FAHGTThmGq0zn6TL UvonJgueXaCuiGjDoKwkG2eatssR/dyLC720J2CuMX9qAEZmDE3S9WdGT2dn8fOKH4s1 mbBTlDAWMGFBsvVDkdjvrEYhbNNmzl6iALi3vCPOVcM+M+dG3si0o+YM6Pns3KDiPLeg asHGwEmsMBothRLU0Uc6N/vMFDl9YQdSwrBUvXyq/FyeEtAdhhm+VSZo7KAfYm2B/JDx FfVQ== X-Gm-Message-State: APt69E04YV8a/r/xkqiIxhOpoPVezVwzir5boq/I6BMgHHKKj2G7MI4p syf5JlpaUudetqyLA2Ypb4GbnAsWcw== X-Google-Smtp-Source: AAOMgpds7CyUmbo0L4+H6vqHrKsFuj8n5Ak037W2NIePQIgkzRQnJJlRBYjXxWBHh5sjkkfP6rGfBg== X-Received: by 2002:adf:fa45:: with SMTP id y5-v6mr4181957wrr.138.1530780457381; Thu, 05 Jul 2018 01:47:37 -0700 (PDT) Received: from laranjeiro-vm.dev.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id t1-v6sm7450756wmt.40.2018.07.05.01.47.36 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 05 Jul 2018 01:47:36 -0700 (PDT) Date: Thu, 5 Jul 2018 10:47:35 +0200 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro To: Yongseok Koh Cc: dev@dpdk.org, Adrien Mazarguil Message-ID: <20180705084735.rrgqf3v7dpmo5col@laranjeiro-vm.dev.6wind.com> References: <90a73b5f33e147ffa3a668f5d19410de17f96045.1530111623.git.nelio.laranjeiro@6wind.com> <20180704083418.GA45405@minint-98vp2qg> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180704083418.GA45405@minint-98vp2qg> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v2 12/20] net/mlx5: add mark/flag flow action 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, 05 Jul 2018 08:47:37 -0000 On Wed, Jul 04, 2018 at 01:34:19AM -0700, Yongseok Koh wrote: > On Wed, Jun 27, 2018 at 05:07:44PM +0200, Nelio Laranjeiro wrote: > > Signed-off-by: Nelio Laranjeiro > > --- > > drivers/net/mlx5/mlx5_flow.c | 209 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 209 insertions(+) > > > > diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c > > index 57f072c03..a39157533 100644 > > --- a/drivers/net/mlx5/mlx5_flow.c > > +++ b/drivers/net/mlx5/mlx5_flow.c > > @@ -52,6 +52,10 @@ extern const struct eth_dev_ops mlx5_dev_ops_isolate; > > #define MLX5_FLOW_FATE_DROP (1u << 0) > > #define MLX5_FLOW_FATE_QUEUE (1u << 1) > > > > +/* Modify a packet. */ > > +#define MLX5_FLOW_MOD_FLAG (1u << 0) > > +#define MLX5_FLOW_MOD_MARK (1u << 1) > > + > > /** Handles information leading to a drop fate. */ > > struct mlx5_flow_verbs { > > unsigned int size; /**< Size of the attribute. */ > > @@ -70,6 +74,8 @@ struct rte_flow { > > struct rte_flow_attr attributes; /**< User flow attribute. */ > > uint32_t layers; > > /**< Bit-fields of present layers see MLX5_FLOW_ITEMS_*. */ > > + uint32_t modifier; > > + /**< Bit-fields of present modifier see MLX5_FLOW_MOD_*. */ > > Why do you think flag and mark modify a packet? I don't think modifier is an > appropriate name. API terminology: "Actions that modify matching traffic contents or its properties. This includes adding/removing encapsulation, encryption, compression and marks." > > uint32_t fate; > > /**< Bit-fields of present fate see MLX5_FLOW_FATE_*. */ > > struct mlx5_flow_verbs verbs; /* Verbs flow. */ > > @@ -954,6 +960,12 @@ mlx5_flow_action_drop(const struct rte_flow_action *actions, > > actions, > > "multiple fate actions are not" > > " supported"); > > + if (flow->modifier & (MLX5_FLOW_MOD_FLAG | MLX5_FLOW_MOD_MARK)) > > + return rte_flow_error_set(error, ENOTSUP, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > + actions, > > + "drop is not compatible with" > > + " flag/mark action"); > > if (size < flow_size) > > mlx5_flow_spec_verbs_add(flow, &drop, size); > > flow->fate |= MLX5_FLOW_FATE_DROP; > > @@ -1007,6 +1019,144 @@ mlx5_flow_action_queue(struct rte_eth_dev *dev, > > return 0; > > } > > > > +/** > > + * Validate action flag provided by the user. > > + * > > + * @param actions > > + * Pointer to flow actions array. > > + * @param flow > > + * Pointer to the rte_flow structure. > > + * @param flow_size > > + * Size in bytes of the available space for to store the flow information. > > + * @param error > > + * Pointer to error structure. > > + * > > + * @return > > + * size in bytes necessary for the conversion, a negative errno value > > + * otherwise and rte_errno is set. > > Like I asked for the previous patches, please be more verbose for function > description and explanation of args and return value. I've update the documentation of all patches it would be strange to see some with correct comments and some without :) > > + */ > > +static int > > +mlx5_flow_action_flag(const struct rte_flow_action *actions, > > + struct rte_flow *flow, const size_t flow_size, > > + struct rte_flow_error *error) > > +{ > > + unsigned int size = sizeof(struct ibv_flow_spec_action_tag); > > + struct ibv_flow_spec_action_tag tag = { > > + .type = IBV_FLOW_SPEC_ACTION_TAG, > > + .size = size, > > + .tag_id = mlx5_flow_mark_set(MLX5_FLOW_MARK_DEFAULT), > > + }; > > + > > + if (flow->modifier & MLX5_FLOW_MOD_FLAG) > > + return rte_flow_error_set(error, ENOTSUP, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > + actions, > > + "flag action already present"); > > + if (flow->fate & MLX5_FLOW_FATE_DROP) > > + return rte_flow_error_set(error, ENOTSUP, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > + actions, > > + "flag is not compatible with drop" > > + " action"); > > + if (flow->modifier & MLX5_FLOW_MOD_MARK) > > + return 0; > > + flow->modifier |= MLX5_FLOW_MOD_FLAG; > > + if (size <= flow_size) > > + mlx5_flow_spec_verbs_add(flow, &tag, size); > > + return size; > > +} > > + > > +/** > > + * Update verbs specification to modify the flag to mark. > > + * > > + * @param flow > > + * Pointer to the rte_flow structure. > > + * @param mark_id > > + * Mark identifier to replace the flag. > > + */ > > +static void > > +mlx5_flow_verbs_mark_update(struct rte_flow *flow, uint32_t mark_id) > > +{ > > + struct ibv_spec_header *hdr; > > + int i; > > + > > + /* Update Verbs specification. */ > > + hdr = (struct ibv_spec_header *)flow->verbs.specs; > > + for (i = 0; i != flow->verbs.attr->num_of_specs; ++i) { > > flow->verbs.attr/specs can be null in case of validation call. But you don't > need to fix it because it is anyway changed and fixed when you add RSS action. You are right, but it still need to be fixed, if for some reason a bisect is used this may break the bug research. > > + if (hdr->type == IBV_FLOW_SPEC_ACTION_TAG) { > > + struct ibv_flow_spec_action_tag *t = > > + (struct ibv_flow_spec_action_tag *)hdr; > > + > > + t->tag_id = mlx5_flow_mark_set(mark_id); > > + } > > + hdr = (struct ibv_spec_header *)((uintptr_t)hdr + hdr->size); > > + } > > +} > > + > > +/** > > + * Validate action mark provided by the user. > > + * > > + * @param actions > > + * Pointer to flow actions array. > > + * @param flow > > + * Pointer to the rte_flow structure. > > + * @param flow_size[in] > > + * Size in bytes of the available space for to store the flow information. > > + * @param error > > + * Pointer to error structure. > > + * > > + * @return > > + * size in bytes necessary for the conversion, a negative errno value > > + * otherwise and rte_errno is set. > > + */ > > +static int > > +mlx5_flow_action_mark(const struct rte_flow_action *actions, > > + struct rte_flow *flow, const size_t flow_size, > > + struct rte_flow_error *error) > > +{ > > + const struct rte_flow_action_mark *mark = actions->conf; > > + unsigned int size = sizeof(struct ibv_flow_spec_action_tag); > > + struct ibv_flow_spec_action_tag tag = { > > + .type = IBV_FLOW_SPEC_ACTION_TAG, > > + .size = size, > > + }; > > + > > + if (!mark) > > + return rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > + actions, > > + "configuration cannot be null"); > > + if (mark->id >= MLX5_FLOW_MARK_MAX) > > + return rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ACTION_CONF, > > + &mark->id, > > + "mark must be between 0 and" > > + " 16777199"); > > Use %d and (MLX5_FLOW_MARK_MAX - 1), instead of fixed string. It needs an snprintf, rte_flow_error_set() does not accept formatting strings. >[...] > > +/** > > + * Mark the Rx queues mark flag if the flow has a mark or flag modifier. > > + * > > + * @param dev > > + * Pointer to Ethernet device. > > + * @param flow > > + * Pointer to flow structure. > > + */ > > +static void > > +mlx5_flow_rxq_mark(struct rte_eth_dev *dev, struct rte_flow *flow) > > +{ > > + struct priv *priv = dev->data->dev_private; > > + > > + (*priv->rxqs)[flow->queue]->mark |= > > + flow->modifier & (MLX5_FLOW_MOD_FLAG | MLX5_FLOW_MOD_MARK); > > This has to be !!(...) as rxq->mark has only 1 bit. But, it is also fixed by > coming RSS patches. Not sure what's benefit of splitting patches in this way. Same answer as above, even if fixed after, it still need a fix here. > > +} > > + > > /** > > * Validate a flow supported by the NIC. > > * > > @@ -1281,6 +1456,7 @@ mlx5_flow_list_create(struct rte_eth_dev *dev, > > if (ret < 0) > > goto error; > > } > > + mlx5_flow_rxq_mark(dev, flow); > > TAILQ_INSERT_TAIL(list, flow, next); > > return flow; > > error: > > @@ -1323,8 +1499,31 @@ static void > > mlx5_flow_list_destroy(struct rte_eth_dev *dev, struct mlx5_flows *list, > > struct rte_flow *flow) > > { > > + struct priv *priv = dev->data->dev_private; > > + struct rte_flow *rflow; > > + const uint32_t mask = MLX5_FLOW_MOD_FLAG & MLX5_FLOW_MOD_MARK; > > + int mark = 0; > > + > > mlx5_flow_fate_remove(dev, flow); > > TAILQ_REMOVE(list, flow, next); > > + if (!(flow->modifier & mask)) { > > + rte_free(flow); > > + return; > > + } > > + /* > > + * When a flow is removed and this flow has a flag/mark modifier, all > > + * flows needs to be parse to verify if the Rx queue use by the flow > > + * still need to track the flag/mark request. > > + */ > > When a flow is created, mlx5_flow_rxq_mark() is called. Is there a specific > reason for not writing a separate function in order to drop rxq->mark bit? > > > + TAILQ_FOREACH(rflow, &priv->flows, next) { > > + if (!(rflow->modifier & mask)) > > + continue; > > + if (flow->queue == rflow->queue) { > > + mark = 1; > > + break; > > + } > > + } > > + (*priv->rxqs)[flow->queue]->mark = !!mark; > > mark can be either 0 or 1, then !!mark == mark anyway. > > > rte_free(flow); > > } > > > > @@ -1358,10 +1557,19 @@ mlx5_flow_list_flush(struct rte_eth_dev *dev, struct mlx5_flows *list) > > void > > mlx5_flow_stop(struct rte_eth_dev *dev, struct mlx5_flows *list) > > { > > + struct priv *priv = dev->data->dev_private; > > struct rte_flow *flow; > > + unsigned int i; > > + unsigned int idx; > > > > TAILQ_FOREACH_REVERSE(flow, list, mlx5_flows, next) > > mlx5_flow_fate_remove(dev, flow); > > + for (idx = 0, i = 0; idx != priv->rxqs_n; ++i) { > > + if (!(*priv->rxqs)[idx]) > > + continue; > > + (*priv->rxqs)[idx]->mark = 0; > > + ++idx; > > + } > > Same question here but looks like this part is being moved to > mlx5_flow_rxqs_clear() in the future. Addressing both question here, for the flow_stop() and flow_destroy() the process is different, for the stop, the flow remains with the mark bit set but all queues must me cleared, there is no comparison to make. As you can see, it don't even get a flow, it directly unset the mask bit in the Rx queues. For the destroy the issue is different, several flows may be using the same Rx queues, if one of them will remains and has a mark, then the associated queues must keep their mark bit set. As the process is different, it would end in two distinct functions and each one used by a single function. For the mlx5_flow_rxq_mark(), the situation is different, the same process is make when a flow is created and the flow are started. > > } > > > > /** > > @@ -1386,6 +1594,7 @@ mlx5_flow_start(struct rte_eth_dev *dev, struct mlx5_flows *list) > > ret = mlx5_flow_fate_apply(dev, flow, &error); > > if (ret < 0) > > goto error; > > + mlx5_flow_rxq_mark(dev, flow); > > } > > return 0; > > error: > > -- > > 2.18.0 Thanks, -- Nélio Laranjeiro 6WIND