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 CE56EA034E; Mon, 21 Feb 2022 10:53:03 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6DB964068C; Mon, 21 Feb 2022 10:53:03 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 645754013F for ; Mon, 21 Feb 2022 10:53:02 +0100 (CET) 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)) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 3CF7240; Mon, 21 Feb 2022 12:53:00 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 3CF7240 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1645437180; bh=dIXT/jnMotMdA2xXjJCZznb1BcXUeT7uOQHkI8z+rb0=; h=Date:Subject:From:To:Cc:References:In-Reply-To:From; b=ZUArUmVlYqRadg8WZqx7EIlQBYq6bZ6q5ivczesRrta5Zr2Dgqdn419B9Zmjta29P KgJ4PRCLyLJFUhsV6sEn4L9YTUOp3aNoENAuOa/xNkHnPOEBwOjTj0ZkoE5puo/I5F QDmu8tigjJgiAnNGzb3/OhuugTC0LN39dTvebGZo= Message-ID: <5ab364fc-8ba0-bad8-3aed-5810cce4bde7@oktetlabs.ru> Date: Mon, 21 Feb 2022 12:52:59 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH v8 01/11] ethdev: introduce flow engine configuration Content-Language: en-US From: Andrew Rybchenko To: Alexander Kozyrev , dev@dpdk.org Cc: orika@nvidia.com, thomas@monjalon.net, ivan.malov@oktetlabs.ru, ferruh.yigit@intel.com, mohammad.abdul.awal@intel.com, qi.z.zhang@intel.com, jerinj@marvell.com, ajit.khaparde@broadcom.com, bruce.richardson@intel.com References: <20220219041144.2145380-1-akozyrev@nvidia.com> <20220220034409.2226860-1-akozyrev@nvidia.com> <20220220034409.2226860-2-akozyrev@nvidia.com> Organization: OKTET Labs In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 On 2/21/22 12:47, Andrew Rybchenko wrote: > On 2/20/22 06:43, Alexander Kozyrev wrote: >> The flow rules creation/destruction at a large scale incurs a performance >> penalty and may negatively impact the packet processing when used >> as part of the datapath logic. This is mainly because software/hardware >> resources are allocated and prepared during the flow rule creation. >> >> In order to optimize the insertion rate, PMD may use some hints provided >> by the application at the initialization phase. The rte_flow_configure() >> function allows to pre-allocate all the needed resources beforehand. >> These resources can be used at a later stage without costly allocations. >> Every PMD may use only the subset of hints and ignore unused ones or >> fail in case the requested configuration is not supported. >> >> The rte_flow_info_get() is available to retrieve the information about >> supported pre-configurable resources. Both these functions must be called >> before any other usage of the flow API engine. >> >> Signed-off-by: Alexander Kozyrev >> Acked-by: Ori Kam > > [snip] > >> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h >> index 6d697a879a..06f0896e1e 100644 >> --- a/lib/ethdev/ethdev_driver.h >> +++ b/lib/ethdev/ethdev_driver.h >> @@ -138,7 +138,12 @@ struct rte_eth_dev_data { >>            * Indicates whether the device is configured: >>            * CONFIGURED(1) / NOT CONFIGURED(0) >>            */ >> -        dev_configured : 1; >> +        dev_configured:1, > > Above is unrelated to the patch. Moreover, it breaks style used > few lines above. > >> +        /** >> +         * Indicates whether the flow engine is configured: >> +         * CONFIGURED(1) / NOT CONFIGURED(0) >> +         */ >> +        flow_configured:1; > > I'd like to understand why we need the information. It is > unclear from the patch. Right now it is write-only. Nobody > checks it. Is flow engine configuration become a mandatory > step? Always? Just in some cases? > >>       /** Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0) */ >>       uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT]; >> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c >> index 7f93900bc8..ffd48e40d5 100644 >> --- a/lib/ethdev/rte_flow.c >> +++ b/lib/ethdev/rte_flow.c >> @@ -1392,3 +1392,72 @@ rte_flow_flex_item_release(uint16_t port_id, >>       ret = ops->flex_item_release(dev, handle, error); >>       return flow_err(port_id, ret, error); >>   } >> + >> +int >> +rte_flow_info_get(uint16_t port_id, >> +          struct rte_flow_port_info *port_info, >> +          struct rte_flow_error *error) >> +{ >> +    struct rte_eth_dev *dev = &rte_eth_devices[port_id]; >> +    const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error); >> + >> +    if (port_info == NULL) { >> +        RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id); >> +        return -EINVAL; >> +    } >> +    if (dev->data->dev_configured == 0) { >> +        RTE_FLOW_LOG(INFO, >> +            "Device with port_id=%"PRIu16" is not configured.\n", >> +            port_id); >> +        return -EINVAL; >> +    } >> +    if (unlikely(!ops)) >> +        return -rte_errno; > > Order of checks is not always obvious, but requires at > least some rules to follow. When there is no any good > reason to do otherwise, I'd suggest to check arguments > in there order. I.e. check port_id and its direct > derivatives first: > 1. ops (since it is NULL if port_id is invalid) > 2. dev_configured (since only port_id is required to check it) > 3. port_info (since it goes after port_id) > >> +    if (likely(!!ops->info_get)) { >> +        return flow_err(port_id, >> +                ops->info_get(dev, port_info, error), >> +                error); >> +    } >> +    return rte_flow_error_set(error, ENOTSUP, >> +                  RTE_FLOW_ERROR_TYPE_UNSPECIFIED, >> +                  NULL, rte_strerror(ENOTSUP)); >> +} >> + >> +int >> +rte_flow_configure(uint16_t port_id, >> +           const struct rte_flow_port_attr *port_attr, >> +           struct rte_flow_error *error) >> +{ >> +    struct rte_eth_dev *dev = &rte_eth_devices[port_id]; >> +    const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error); >> +    int ret; >> + >> +    dev->data->flow_configured = 0; >> +    if (port_attr == NULL) { >> +        RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id); >> +        return -EINVAL; >> +    } >> +    if (dev->data->dev_configured == 0) { >> +        RTE_FLOW_LOG(INFO, >> +            "Device with port_id=%"PRIu16" is not configured.\n", >> +            port_id); >> +        return -EINVAL; >> +    } In fact there is one more interesting question related to device states. Necessity to call flow info and flow configure in configured state allows configure to rely on device configuration. The question is: what should happen with the device flow engine configuration if the device is reconfigured? >> +    if (dev->data->dev_started != 0) { >> +        RTE_FLOW_LOG(INFO, >> +            "Device with port_id=%"PRIu16" already started.\n", >> +            port_id); >> +        return -EINVAL; >> +    } >> +    if (unlikely(!ops)) >> +        return -rte_errno; > > Same logic here: > 1. ops > 2. dev_configured > 3. dev_started > 4. port_attr > 5. ops->configure since we want to be sure that state and input >    arguments are valid before calling it > >> +    if (likely(!!ops->configure)) { >> +        ret = ops->configure(dev, port_attr, error); >> +        if (ret == 0) >> +            dev->data->flow_configured = 1; >> +        return flow_err(port_id, ret, error); >> +    } >> +    return rte_flow_error_set(error, ENOTSUP, >> +                  RTE_FLOW_ERROR_TYPE_UNSPECIFIED, >> +                  NULL, rte_strerror(ENOTSUP)); >> +} > > [snip] > >> +/** >> + * @warning >> + * @b EXPERIMENTAL: this API may change without prior notice. >> + * >> + * Get information about flow engine resources. >> + * >> + * @param port_id >> + *   Port identifier of Ethernet device. >> + * @param[out] port_info >> + *   A pointer to a structure of type *rte_flow_port_info* >> + *   to be filled with the resources information of the port. >> + * @param[out] error >> + *   Perform verbose error reporting if not NULL. >> + *   PMDs initialize this structure in case of error only. >> + * >> + * @return >> + *   0 on success, a negative errno value otherwise and rte_errno is >> set. > > If I'm not mistakes we should be explicit with > negative result values menting > >> + */ >> +__rte_experimental >> +int >> +rte_flow_info_get(uint16_t port_id, >> +          struct rte_flow_port_info *port_info, >> +          struct rte_flow_error *error); > > [snip] > >> +/** >> + * @warning >> + * @b EXPERIMENTAL: this API may change without prior notice. >> + * >> + * Configure the port's flow API engine. >> + * >> + * This API can only be invoked before the application >> + * starts using the rest of the flow library functions. >> + * >> + * The API can be invoked multiple times to change the >> + * settings. The port, however, may reject the changes. >> + * >> + * Parameters in configuration attributes must not exceed >> + * numbers of resources returned by the rte_flow_info_get API. >> + * >> + * @param port_id >> + *   Port identifier of Ethernet device. >> + * @param[in] port_attr >> + *   Port configuration attributes. >> + * @param[out] error >> + *   Perform verbose error reporting if not NULL. >> + *   PMDs initialize this structure in case of error only. >> + * >> + * @return >> + *   0 on success, a negative errno value otherwise and rte_errno is >> set. > > Same here. > > [snip]