RE: [PATCH 1/2] ethdev: add Rx packet type offload control flag

[Chaoyong He <chaoyong.he@corigine.com>]

> On 9/24/2024 3:03 AM, Chaoyong He wrote:
> >> On 6/19/2024 11:11 AM, Chaoyong He wrote:
> >>> From: Long Wu <long.wu@corigine.com>
> >>>
> >>> The Rx packet type offload feature may affect the performance, so
> >>> add a control flag for applications to turn it on or off.
> >>>
> >>> Signed-off-by: Long Wu <long.wu@corigine.com>
> >>> ---
> >>>  lib/ethdev/rte_ethdev.h | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
> >>> 548fada1c7..be86983e24 100644
> >>> --- a/lib/ethdev/rte_ethdev.h
> >>> +++ b/lib/ethdev/rte_ethdev.h
> >>> @@ -1555,6 +1555,7 @@ struct rte_eth_conf {  #define
> >>> RTE_ETH_RX_OFFLOAD_OUTER_UDP_CKSUM  RTE_BIT64(18)
> >>>  #define RTE_ETH_RX_OFFLOAD_RSS_HASH         RTE_BIT64(19)
> >>>  #define RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT     RTE_BIT64(20)
> >>> +#define RTE_ETH_RX_OFFLOAD_PTYPES           RTE_BIT64(21)
> >>>
> >>>  #define RTE_ETH_RX_OFFLOAD_CHECKSUM
> >> (RTE_ETH_RX_OFFLOAD_IPV4_CKSUM | \
> >>>  				 RTE_ETH_RX_OFFLOAD_UDP_CKSUM | \
> >>
> >> Hi Chaoyong,
> >>
> >> Instead of having an offload for ptypes, we have APIs for this,
> >>
> >> First one is 'rte_eth_dev_get_supported_ptypes()' that application
> >> can learn the supported packet types.
> >>
> >> Second one is more related to above flag, it is 'rte_eth_dev_set_ptypes()'
> >> which application can set which pytpes is required, it can be set to
> >> disable all packet type parsing, can be similar to not requesting
> >> 'RTE_ETH_RX_OFFLOAD_PTYPES'.
> >>
> >> With above two APIs, do we still need the offload flag?
> >>
> >
> > At present, the purpose of the ops 'rte_eth_dev_set_ptypes()' is to set the
> range of packet types to handle.
> >
> 
> Yes, and setting 'ptype_mask' to zero should disable packet type parsing.
> 
> Packet type parsing is an offload, but when we have an API that has finer
> grained control to what packet type to parse, why not use it instead of having
> offload flag, which is all on or off configuration.
> 
> > Of course, we can maintain a flag for each application and driver
> > based on the return value of this ops; but this is a bit troublesome.
> >
> 
> I didn't get your point, why maintain a flag?
> 
> > So, we hope to follow the example of RSS, in addition to
> > 'rte_eth_dev_rss_hash_update()' and 'rte_eth_dev_rss_hash_conf_get()',
> > we also want to set a flag for the ptype function similar to
> RTE_ETH_RX_OFFLOAD_RSS_HASH.
> >
> >>
> >> Another concern with adding new offload flag is backward
> >> compatibility, all existing drivers that support packet type parsing
> >> should be updated to list this offload flag as capability. Also they
> >> need to be updated to configure packet parsing based on user requested
> offload configuration.
> >>
> >
> > If you agree with this design suggestion, we will adapt all the related code to
> ptypes for each PMDs and 'test-pmd' applications in the next patch.
> > Do you think this okay?
> >
> >> Briefly, we can't just introduce a new offload flag for an existing
> >> capability and update only one driver, all drivers needs to be updated.

Hi Ferruh,
Thanks for your explanation, we understand what you mean now.

We'll send a new version patch to drop the 'RTE_ETH_RX_OFFLOAD_PTYPES'.