From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8DB4FA034F; Fri, 8 Oct 2021 12:23:51 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 514C140E25; Fri, 8 Oct 2021 12:23:51 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id BC94140DF8 for ; Fri, 8 Oct 2021 12:23:49 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 374A57F514; Fri, 8 Oct 2021 13:23:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 374A57F514 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1633688629; bh=m0qitftNONucbwQ9BeK88TNwz916COpX5dzkD7i63h0=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=fGHYkr54MOJeJhX/zSCJR/DcNvbLeOfSRf8EpfXNC+5FNZWT5P0Pb237HKUiJbphz Uj39sdrZx7gThGZEV1i7KMwiwRARGur0R0IH/QaIm5PSndKJsz6Azg8lkI30CVOs0m bueXZ2HsTAOLyKEpTef5h6rtn7spO6xukfYm3yxA= To: Matan Azrad , Ori Kam , Xiaoyun Li , Ray Kinsella , Ajit Khaparde , Somnath Kotur , Nithin Dabilpuram , Kiran Kumar K , Sunil Kumar Kori , Satha Rao , "Min Hu (Connor)" , Yisen Zhuang , Lijun Ou , Qiming Yang , Qi Zhang , Slava Ovsiienko , Jerin Jacob , Jasvinder Singh , Cristian Dumitrescu , NBU-Contact-Thomas Monjalon , Ferruh Yigit Cc: "dev@dpdk.org" References: <20210928152300.989961-1-andrew.rybchenko@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <935b89c5-250e-93a6-eecd-fcc9a34961f4@oktetlabs.ru> Date: Fri, 8 Oct 2021 13:23:48 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Matan, Many thanks for review. See below. Andrew. On 10/6/21 12:45 PM, Matan Azrad wrote: > Hi Andrew > > Thank you for the big effort to adjust mlx5. > I left some comments inside. > If you feel it is too much, we can do a later patch to improve. > > Matan > > >> From: Andrew Rybchenko >> Indirect actions should be used to do shared counters. >> >> Signed-off-by: Andrew Rybchenko [snip] >> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c >> b/drivers/net/mlx5/mlx5_flow_dv.c index b610ad3ef4..91314d5ea5 100644 >> --- a/drivers/net/mlx5/mlx5_flow_dv.c >> +++ b/drivers/net/mlx5/mlx5_flow_dv.c >> @@ -3308,33 +3308,11 @@ flow_dv_validate_action_set_tag(struct >> rte_eth_dev *dev, >> return 0; >> } >> >> -/** >> - * Check if action counter is shared by either old or new mechanism. >> - * >> - * @param[in] action >> - * Pointer to the action structure. >> - * >> - * @return >> - * True when counter is shared, false otherwise. >> - */ >> -static inline bool >> -is_shared_action_count(const struct rte_flow_action *action) -{ >> - const struct rte_flow_action_count *count = >> - (const struct rte_flow_action_count *)action->conf; >> - >> - if ((int)action->type == MLX5_RTE_FLOW_ACTION_TYPE_COUNT) >> - return true; So, it is shared if and only if type is MLX5_RTE_FLOW_ACTION_TYPE_COUNT. >> - return !!(count && count->shared); >> -} >> - >> /** >> * Validate count action. >> * >> * @param[in] dev >> * Pointer to rte_eth_dev structure. >> - * @param[in] shared >> - * Indicator if action is shared. >> * @param[in] action_flags >> * Holds the actions detected until now. >> * @param[out] error >> @@ -3344,7 +3322,7 @@ is_shared_action_count(const struct >> rte_flow_action *action) >> * 0 on success, a negative errno value otherwise and rte_errno is set. >> */ >> static int >> -flow_dv_validate_action_count(struct rte_eth_dev *dev, bool shared, >> +flow_dv_validate_action_count(struct rte_eth_dev *dev, >> uint64_t action_flags, >> struct rte_flow_error *error) { @@ -3356,11 +3334,6 @@ >> flow_dv_validate_action_count(struct rte_eth_dev *dev, bool shared, >> return rte_flow_error_set(error, EINVAL, >> RTE_FLOW_ERROR_TYPE_ACTION, NULL, >> "duplicate count actions set"); >> - if (shared && (action_flags & MLX5_FLOW_ACTION_AGE) && > > This check is relevant to the indirect action case in some places, see below. Yes, this if should be preserved. Thanks. > > >> - !priv->sh->flow_hit_aso_en) >> - return rte_flow_error_set(error, EINVAL, >> - RTE_FLOW_ERROR_TYPE_ACTION, NULL, >> - "old age and shared count combination is not >> supported"); >> #ifdef HAVE_IBV_FLOW_DEVX_COUNTERS >> return 0; >> #endif >> @@ -5658,9 +5631,7 @@ flow_dv_validate_action_sample(uint64_t >> *action_flags, >> break; >> case RTE_FLOW_ACTION_TYPE_COUNT: >> ret = flow_dv_validate_action_count >> - (dev, is_shared_action_count(act), >> - *action_flags | sub_action_flags, >> - error); >> + (dev, *action_flags | sub_action_flags, >> + error); >> if (ret < 0) >> return ret; >> *count = act->conf; @@ -6230,60 +6201,6 @@ >> flow_dv_counter_alloc(struct rte_eth_dev *dev, uint32_t age) >> return 0; >> } >> >> -/** >> - * Allocate a shared flow counter. >> - * >> - * @param[in] ctx >> - * Pointer to the shared counter configuration. >> - * @param[in] data >> - * Pointer to save the allocated counter index. >> - * >> - * @return >> - * Index to flow counter on success, 0 otherwise and rte_errno is set. >> - */ >> - >> -static int32_t >> -flow_dv_counter_alloc_shared_cb(void *ctx, union mlx5_l3t_data *data) -{ >> - struct mlx5_shared_counter_conf *conf = ctx; >> - struct rte_eth_dev *dev = conf->dev; >> - struct mlx5_flow_counter *cnt; >> - >> - data->dword = flow_dv_counter_alloc(dev, 0); >> - data->dword |= MLX5_CNT_SHARED_OFFSET; >> - cnt = flow_dv_counter_get_by_idx(dev, data->dword, NULL); >> - cnt->shared_info.id = conf->id; >> - return 0; >> -} >> - >> -/** >> - * Get a shared flow counter. >> - * >> - * @param[in] dev >> - * Pointer to the Ethernet device structure. >> - * @param[in] id >> - * Counter identifier. >> - * >> - * @return >> - * Index to flow counter on success, 0 otherwise and rte_errno is set. >> - */ >> -static uint32_t >> -flow_dv_counter_get_shared(struct rte_eth_dev *dev, uint32_t id) -{ >> - struct mlx5_priv *priv = dev->data->dev_private; >> - struct mlx5_shared_counter_conf conf = { >> - .dev = dev, >> - .id = id, >> - }; >> - union mlx5_l3t_data data = { >> - .dword = 0, >> - }; >> - >> - mlx5_l3t_prepare_entry(priv->sh->cnt_id_tbl, id, &data, >> - flow_dv_counter_alloc_shared_cb, &conf); >> - return data.dword; >> -} >> - > > Need to remove cnt_id_tbl and from sh and all of its management, no? Looks. Will do in v2. > >> /** >> * Get age param from counter index. >> * >> @@ -6366,16 +6283,6 @@ flow_dv_counter_free(struct rte_eth_dev *dev, >> uint32_t counter) >> if (pool->is_aged) { >> flow_dv_counter_remove_from_age(dev, counter, cnt); >> } else { >> - /* >> - * If the counter action is shared by ID, the l3t_clear_entry >> - * function reduces its references counter. If after the >> - * reduction the action is still referenced, the function >> - * returns here and does not release it. >> - */ >> - if (IS_LEGACY_SHARED_CNT(counter) && >> - mlx5_l3t_clear_entry(priv->sh->cnt_id_tbl, >> - cnt->shared_info.id)) >> - return; >> /* >> * If the counter action is shared by indirect action API, >> * the atomic function reduces its references counter. >> @@ -6385,8 +6292,7 @@ flow_dv_counter_free(struct rte_eth_dev *dev, >> uint32_t counter) > > You can remove the ID sharing notice in this comment. OK, will do. > >> * indirect action API, shared info is 1 before the reduction, >> * so this condition is failed and function doesn't return here. >> */ >> - if (!IS_LEGACY_SHARED_CNT(counter) && >> - __atomic_sub_fetch(&cnt->shared_info.refcnt, 1, >> + if (__atomic_sub_fetch(&cnt->shared_info.refcnt, 1, >> __ATOMIC_RELAXED)) >> return; >> } >> @@ -7275,7 +7181,6 @@ flow_dv_validate(struct rte_eth_dev *dev, const >> struct rte_flow_attr *attr, >> } >> for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { >> int type = actions->type; >> - bool shared_count = false; >> >> if (!mlx5_flow_os_action_supported(type)) >> return rte_flow_error_set(error, ENOTSUP, @@ -7427,8 +7332,7 >> @@ flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr >> *attr, >> break; >> case MLX5_RTE_FLOW_ACTION_TYPE_COUNT: >> case RTE_FLOW_ACTION_TYPE_COUNT: >> - shared_count = is_shared_action_count(actions); > > We need shared_count for the indirect count case(which can be shared with different flows), > So, if the action is MLX5_RTE_FLOW_ACTION_TYPE_COUNT, shared_count should be true. > It is relevant to the validate function below and for the check at the end of the function. Thanks, will fix in v2. > > >> - ret = flow_dv_validate_action_count(dev, shared_count, >> + ret = flow_dv_validate_action_count(dev, >> action_flags, >> error); >> if (ret < 0) >> @@ -17469,19 +17364,7 @@ flow_dv_action_validate(struct rte_eth_dev *dev, >> "Indirect age action not supported"); >> return flow_dv_validate_action_age(0, action, dev, err); >> case RTE_FLOW_ACTION_TYPE_COUNT: >> - /* >> - * There are two mechanisms to share the action count. >> - * The old mechanism uses the shared field to share, while the >> - * new mechanism uses the indirect action API. >> - * This validation comes to make sure that the two mechanisms >> - * are not combined. >> - */ >> - if (is_shared_action_count(action)) >> - return rte_flow_error_set(err, ENOTSUP, >> - RTE_FLOW_ERROR_TYPE_ACTION, >> - NULL, >> - "Mix shared and indirect counter is not supported"); >> - return flow_dv_validate_action_count(dev, true, 0, err); >> + return flow_dv_validate_action_count(dev, 0, err); > > > Did you also consider improving struct mlx5_flow_counter_shared? Then, maybe we don't need it anymore. Comments say that reference counters there are used for indirect actions as well. So, I'll keep it for follow up cleanups. [snip]