RE: [PATCH v9 1/4] ethdev: introduce protocol header API

["Wang, YuanX" <yuanx.wang@intel.com>]

Hi Andrew,

Apologies for the delay in getting back to you.

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Friday, July 8, 2022 11:01 PM
> To: Wu, WenxuanX <wenxuanx.wu@intel.com>; thomas@monjalon.net; Li,
> Xiaoyun <xiaoyun.li@intel.com>; ferruh.yigit@xilinx.com; Singh, Aman Deep
> <aman.deep.singh@intel.com>; dev@dpdk.org; Zhang, Yuying
> <yuying.zhang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> jerinjacobk@gmail.com
> Cc: stephen@networkplumber.org
> Subject: Re: [PATCH v9 1/4] ethdev: introduce protocol header API
> 
> On 6/13/22 13:25, wenxuanx.wu@intel.com wrote:
> > From: Wenxuan Wu <wenxuanx.wu@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.wu@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 {