Hi Andrew,
Apologies for the delay in getting back to you.
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> Sent: Friday, July 8, 2022 11:01 PM
> To: Wu, WenxuanX <wenxuanx...@intel.com>; tho...@monjalon.net; Li,
> Xiaoyun <xiaoyun...@intel.com>; ferruh.yi...@xilinx.com; Singh, Aman Deep
> <aman.deep.si...@intel.com>; dev@dpdk.org; Zhang, Yuying
> <yuying.zh...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>;
> jerinjac...@gmail.com
> Cc: step...@networkplumber.org
> Subject: Re: [PATCH v9 1/4] ethdev: introduce protocol header API
>
> On 6/13/22 13:25, wenxuanx...@intel.com wrote:
> > From: Wenxuan Wu <wenxuanx...@intel.com>
> >
> > This patch added new ethdev API to retrieve supported protocol header
> > mask
>
> This patch added -> Add
Thanks for your catch, will fix in the next version.
>
> > 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.
As Xuan replied in the patch 2, we think maybe RTE_PTYPE_* is enough.
Any insights are welcome.
>
> >
> > Signed-off-by: Wenxuan Wu <wenxuanx...@intel.com>
>
> [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.
Thank you for your suggestion, will fix in the next version.
>
> > /** 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.
Thanks for your catch, it should be in the patch 2, will fix in the next
version.
>
> > - 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'
Thanks for your catch, will fix in the next version.
>
> > }
> > 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
Thanks for your catch, will fix in the next version.
>
> > +
> > + 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.
Thank you for your suggestion, will fix in the next version.
>
> > + * 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?
Thank you for your suggestion, will fix in the next version.
>
> > + * 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
Thank you for your suggestion, will fix in the next version.
>
> > + * - (-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
Sure, it should be targeted for 22.11.
Thanks,
Yuan
>
> > + rte_eth_supported_hdrs_get;
> > };
> >
> > INTERNAL {
