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 3C415A00C5; Fri, 8 Jul 2022 17:00:48 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 169A0406B4; Fri, 8 Jul 2022 17:00:48 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 6576F4021E for ; Fri, 8 Jul 2022 17:00:45 +0200 (CEST) Received: from [192.168.1.38] (unknown [188.170.75.69]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 352C1B9; Fri, 8 Jul 2022 18:00:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 352C1B9 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1657292445; bh=lAHaZfdfQYXAnGt8wX/vGGrAyI5t/SG1kb1UeMnyOUA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=EuPfOlASm/+y+WO/VKLfCW3OOyNwqoNNS+S+pt3RJHioxar80BbPmK74z65/y0S2b DK708fLCVcER4KuReUEaCZv8Yzg5adIMoGcxFrzK2vesnaO31xhzr6DIXK/V+yh3fO bu7Gg7mppesobO/fEarz5sGx4hrGc/1nO5tsBuso= Message-ID: Date: Fri, 8 Jul 2022 18:00:37 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v9 1/4] ethdev: introduce protocol header API Content-Language: en-US To: wenxuanx.wu@intel.com, thomas@monjalon.net, xiaoyun.li@intel.com, ferruh.yigit@xilinx.com, aman.deep.singh@intel.com, dev@dpdk.org, yuying.zhang@intel.com, qi.z.zhang@intel.com, jerinjacobk@gmail.com Cc: stephen@networkplumber.org References: <20220303060136.36427-1-xuan.ding@intel.com> <20220613102550.241759-1-wenxuanx.wu@intel.com> <20220613102550.241759-2-wenxuanx.wu@intel.com> From: Andrew Rybchenko In-Reply-To: <20220613102550.241759-2-wenxuanx.wu@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 6/13/22 13:25, wenxuanx.wu@intel.com wrote: > From: Wenxuan Wu > > This patch added new ethdev API to retrieve supported protocol header mask This patch added -> Add > of a PMD, which helps to configure protocol header based buffer split. I'd like to see motivation why single mask is considered sufficient. I.e. why don't we follow ptypes approach which is move flexible, but a bit more complicated. Looking at RTE_PTYPE_* defines carefully it looks like below API simply cannot provide information that we can split after TCP or UDP. > > Signed-off-by: Wenxuan Wu [snip] > /** > * @internal > * Dump private info from device to a file. > @@ -1281,6 +1296,9 @@ struct eth_dev_ops { > /** Set IP reassembly configuration */ > eth_ip_reassembly_conf_set_t ip_reassembly_conf_set; > > + /** Get supported ptypes to split */ > + eth_buffer_split_hdr_ptype_get_t hdrs_supported_ptypes_get; > + It is better to be consistent with naming. I.e. just cut prefix "eth_" and suffix "_t". Also the type name sounds like it get current split configuration, not supported one. > /** Dump private info from device */ > eth_dev_priv_dump_t eth_dev_priv_dump; > }; > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c > index 29a3d80466..e1f2a0ffe3 100644 > --- a/lib/ethdev/rte_ethdev.c > +++ b/lib/ethdev/rte_ethdev.c > @@ -1636,9 +1636,10 @@ rte_eth_dev_is_removed(uint16_t port_id) > } > > static int > -rte_eth_rx_queue_check_split(const struct rte_eth_rxseg_split *rx_seg, > - uint16_t n_seg, uint32_t *mbp_buf_size, > - const struct rte_eth_dev_info *dev_info) > +rte_eth_rx_queue_check_split(uint16_t port_id, > + const struct rte_eth_rxseg_split *rx_seg, > + int16_t n_seg, uint32_t *mbp_buf_size, > + const struct rte_eth_dev_info *dev_info) > { > const struct rte_eth_rxseg_capa *seg_capa = &dev_info->rx_seg_capa; > struct rte_mempool *mp_first; > @@ -1694,13 +1695,7 @@ rte_eth_rx_queue_check_split(const struct rte_eth_rxseg_split *rx_seg, > } > offset += seg_idx != 0 ? 0 : RTE_PKTMBUF_HEADROOM; > *mbp_buf_size = rte_pktmbuf_data_room_size(mpl); > - length = length != 0 ? length : *mbp_buf_size; > - if (*mbp_buf_size < length + offset) { I don't understand why the check goes away completely. > - RTE_ETHDEV_LOG(ERR, > - "%s mbuf_data_room_size %u < %u (segment length=%u + segment offset=%u)\n", > - mpl->name, *mbp_buf_size, > - length + offset, length, offset); > - return -EINVAL; > + Unnecessary empty line > } Shouldn't the curly bracket go away as well together with its 'if' > } > return 0; > @@ -1779,7 +1774,7 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id, > n_seg = rx_conf->rx_nseg; > > if (rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) { > - ret = rte_eth_rx_queue_check_split(rx_seg, n_seg, > + ret = rte_eth_rx_queue_check_split(port_id, rx_seg, n_seg, > &mbp_buf_size, > &dev_info); > if (ret != 0) > @@ -5844,6 +5839,20 @@ rte_eth_ip_reassembly_conf_set(uint16_t port_id, > (*dev->dev_ops->ip_reassembly_conf_set)(dev, conf)); > } > > +int > +rte_eth_supported_hdrs_get(uint16_t port_id, uint32_t *ptypes) > +{ > + struct rte_eth_dev *dev; > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > + dev = &rte_eth_devices[port_id]; ptypes must be checked vs NULL > + > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->hdrs_supported_ptypes_get, > + -ENOTSUP); > + > + return eth_err(port_id, > + (*dev->dev_ops->hdrs_supported_ptypes_get)(dev, ptypes)); > +} > + > int > rte_eth_dev_priv_dump(uint16_t port_id, FILE *file) > { > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h > index 04cff8ee10..72cac1518e 100644 > --- a/lib/ethdev/rte_ethdev.h > +++ b/lib/ethdev/rte_ethdev.h > @@ -6152,6 +6152,28 @@ rte_eth_tx_buffer(uint16_t port_id, uint16_t queue_id, > return rte_eth_tx_buffer_flush(port_id, queue_id, buffer); > } > > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice > + * > + * Get supported header protocols to split supported by PMD. "supported" twice above. Get supported header protocols to split on Rx. > + * The API will return error if the device is not valid. Above sentence is obvious and does not add any value. Please, remove. > + * > + * @param port_id > + * The port identifier of the device. > + * @param ptype Why do you use out annotation for the callback description and does not use it here? > + * Supported protocol headers of driver. > + * @return > + * - (-ENOTSUP) if header protocol is not supported by device. > + * - (-ENODEV) if *port_id* invalid. EINVAL in the case of invalid ptypes argument > + * - (-EIO) if device is removed. > + * - (0) on success. > + */ > +__rte_experimental > +int rte_eth_supported_hdrs_get(uint16_t port_id, > + uint32_t *ptype); > + > #ifdef __cplusplus > } > #endif > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map > index 20391ab29e..7705c0364a 100644 > --- a/lib/ethdev/version.map > +++ b/lib/ethdev/version.map > @@ -279,6 +279,9 @@ EXPERIMENTAL { > rte_flow_async_action_handle_create; > rte_flow_async_action_handle_destroy; > rte_flow_async_action_handle_update; > + > + # added in 22.07 It hopefully will be in 22.11 > + rte_eth_supported_hdrs_get; > }; > > INTERNAL {