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 C59D7A034E; Mon, 21 Feb 2022 15:54:01 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 85C4A4068C; Mon, 21 Feb 2022 15:54:01 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id D65C54013F for ; Mon, 21 Feb 2022 15:53:58 +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 5242638; Mon, 21 Feb 2022 17:53:58 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 5242638 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1645455238; bh=A3xPMRSOpPDlH+35fU74K4AXz3IKZwB110v8xUgudts=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=KcXLv/DlegwPfwcB4CL6hjNvxaxBDz1Rk1pZ/I6p09PaF53fUWSigztR8BPbZIUIr SGOXHAfKTr9/3QjM6bMl31xVR9G4SAtXdTDf8+Aj9uGPmHXYQQ3DWTX1/+sDgKy68/ /HcKJTrh0JCuqHKtKt6Z76dDzTfpVaQ4rm1S9zm4= Message-ID: Date: Mon, 21 Feb 2022 17:53:57 +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 To: Ori Kam , Alexander Kozyrev , "dev@dpdk.org" Cc: "NBU-Contact-Thomas Monjalon (EXTERNAL)" , "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> <5ab364fc-8ba0-bad8-3aed-5810cce4bde7@oktetlabs.ru> From: Andrew Rybchenko 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 15:53, Ori Kam wrote: > Hi Andrew and Alexander, > >> -----Original Message----- >> From: Andrew Rybchenko >> Sent: Monday, February 21, 2022 11:53 AM >> Subject: Re: [PATCH v8 01/11] ethdev: introduce flow engine configuration >> >> 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. >>> > +1 >>>> +        /** >>>> +         * 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? >>> > > See my commets below, > I can see two ways or remove this member or check in each control function > that the configuration function was done. > >>>>       /** 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) >>> > > Agree, > >>>> +    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; > > I don't think there is meaning to add this set here. > I would remove this field. > Unless you want to check it for all control functions. > >>>> +    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? >> > > That’s dependes on PMD. > PMD may support reconfiguring, partial reconfigure (for example only number of objects > but not changing the number of queues) or it will not support any reconfigure. > It may also be dependent if the port is started or not. > Currently we don't plan to support reconfigure but in future we may support changing the > number of objects. But we should define behaviour and say what application should expect. Above sounds like: Flow engine configuration persists across device reconfigure. > >>>> +    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 >>> > I'm not sure, until now we didn't have any errors values defined in RTE flow. > I don't want to enforce PMD with the error types. > If PMD can say that it can give better error code or add a case that may result in > error, I don't want to change the API. > So I think we better leave the error codes out of documentation unless they are final and can only > be resulted from the rte_level. It is not helpful for application. If so, application don't know how to interpret and handle various error codes. >>>> + */ >>>> +__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. >>> > Same as above. > >>> [snip] > > Best, > ORi