[v2] net/ice: optimize protocol extraction by dynamic mbuf API
diff mbox series

Message ID 20191029073448.89373-1-haiyue.wang@intel.com
State Superseded, archived
Delegated to: xiaolong ye
Headers show
Series
  • [v2] net/ice: optimize protocol extraction by dynamic mbuf API
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-compilation success Compile Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation fail Compilation issues

Commit Message

Haiyue Wang Oct. 29, 2019, 7:34 a.m. UTC
The original design is to use rte_mbuf::udata64 to save the metadata of
protocol extraction which has network protocol data fields and type, an
private API is used to decode this metadata. It is not so generic.

Use the new dynamic mbuf field and flags to handle protocol extraction.
Then the metadata is very clean, only network protocol fields extracted
by defined offset, and its type will be indicated by related ol_flags.

Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
---

v2: - disable the protocol extraction if failed to register some ol_flags
    - rewrite the commit message

 doc/guides/nics/ice.rst       |  22 +++--
 drivers/net/ice/Makefile      |   3 -
 drivers/net/ice/ice_ethdev.c  |  67 ++++++++++++++-
 drivers/net/ice/ice_ethdev.h  |  18 ++++
 drivers/net/ice/ice_rxtx.c    |  82 +++++++++++-------
 drivers/net/ice/ice_rxtx.h    |   1 -
 drivers/net/ice/meson.build   |   2 -
 drivers/net/ice/rte_pmd_ice.h | 152 ----------------------------------
 8 files changed, 152 insertions(+), 195 deletions(-)
 delete mode 100644 drivers/net/ice/rte_pmd_ice.h

Comments

Olivier Matz Oct. 30, 2019, 4:56 p.m. UTC | #1
Hi Haiyue,

Please see some comments below.

On Tue, Oct 29, 2019 at 03:34:48PM +0800, Haiyue Wang wrote:
> The original design is to use rte_mbuf::udata64 to save the metadata of
> protocol extraction which has network protocol data fields and type, an

nit: an -> a

> private API is used to decode this metadata. It is not so generic.
> 
> Use the new dynamic mbuf field and flags to handle protocol extraction.
> Then the metadata is very clean, only network protocol fields extracted
> by defined offset, and its type will be indicated by related ol_flags.
> 
> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> ---

Before the patch, the result of a specific hw parsing was stored in
m->udata on Rx and this information is supposed to used by an
application or a library, is that correct?


> v2: - disable the protocol extraction if failed to register some ol_flags
>     - rewrite the commit message
> 
>  doc/guides/nics/ice.rst       |  22 +++--
>  drivers/net/ice/Makefile      |   3 -
>  drivers/net/ice/ice_ethdev.c  |  67 ++++++++++++++-
>  drivers/net/ice/ice_ethdev.h  |  18 ++++
>  drivers/net/ice/ice_rxtx.c    |  82 +++++++++++-------
>  drivers/net/ice/ice_rxtx.h    |   1 -
>  drivers/net/ice/meson.build   |   2 -
>  drivers/net/ice/rte_pmd_ice.h | 152 ----------------------------------
>  8 files changed, 152 insertions(+), 195 deletions(-)
>  delete mode 100644 drivers/net/ice/rte_pmd_ice.h
> 
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> index 933f63480..f460d1cfd 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -82,8 +82,8 @@ Runtime Config Options
>  
>  - ``Protocol extraction for per queue``
>  
> -  Configure the RX queues to do protocol extraction into ``rte_mbuf::udata64``
> -  for protocol handling acceleration, like checking the TCP SYN packets quickly.
> +  Configure the RX queues to do protocol extraction into mbuf for protocol
> +  handling acceleration, like checking the TCP SYN packets quickly.
>  
>    The argument format is::
>  
> @@ -111,7 +111,9 @@ Runtime Config Options
>    This setting means queues 1, 2-3, 8-9 are TCP extraction, queues 10-23 are
>    IPv6 extraction, other queues use the default VLAN extraction.
>  
> -  The extraction will be copied into the lower 32 bit of ``rte_mbuf::udata64``.
> +  The extraction metadata will be copied into the registered dynamic mbuf field
> +  with name: "proto-xtr-metadata", size: 4B, align: __alignof__(uint32_t), and
> +  the related dynamic mbuf flags in ``rte_mbuf::ol_flags`` will be set.
>  
>    .. table:: Protocol extraction : ``vlan``
>  
> @@ -125,6 +127,8 @@ Runtime Config Options
>  
>    VLAN2 - C-VLAN (second for QinQ).
>  
> +  Dynamic mbuf flag : "proto-xtr-ol-vlan"
> +
>    .. table:: Protocol extraction : ``ipv4``
>  
>     +----------------------------+----------------------------+
> @@ -137,6 +141,8 @@ Runtime Config Options
>  
>    IPHDR2 - IPv4 header word 0, "Ver", "Hdr Len" and "Type of Service" fields.
>  
> +  Dynamic mbuf flag : "proto-xtr-ol-ipv4"
> +
>    .. table:: Protocol extraction : ``ipv6``
>  
>     +----------------------------+----------------------------+
> @@ -150,6 +156,8 @@ Runtime Config Options
>    IPHDR2 - IPv6 header word 0, "Ver", "Traffic class" and high 4 bits of
>    "Flow Label" fields.
>  
> +  Dynamic mbuf flag : "proto-xtr-ol-ipv6"
> +
>    .. table:: Protocol extraction : ``ipv6_flow``
>  
>     +----------------------------+----------------------------+
> @@ -163,6 +171,8 @@ Runtime Config Options
>    IPHDR2 - IPv6 header word 0, "Ver", "Traffic class" and high 4 bits of
>    "Flow Label" fields.
>  
> +  Dynamic mbuf flag : "proto-xtr-ol-ipv6-flow"
> +
>    .. table:: Protocol extraction : ``tcp``
>  
>     +----------------------------+----------------------------+
> @@ -175,11 +185,7 @@ Runtime Config Options
>  
>    TCPHDR2 - Reserved
>  
> -  Use ``get_proto_xtr_flds(struct rte_mbuf *mb)`` to access the protocol
> -  extraction, do not use ``rte_mbuf::udata64`` directly.
> -
> -  The ``dump_proto_xtr_flds(struct rte_mbuf *mb)`` routine shows how to
> -  access the protocol extraction result in ``struct rte_mbuf``.
> +  Dynamic mbuf flag : "proto-xtr-ol-tcp"
>  
>  Driver compilation and testing
>  ------------------------------
> diff --git a/drivers/net/ice/Makefile b/drivers/net/ice/Makefile
> index 7c3b6a7ff..a1817e14c 100644
> --- a/drivers/net/ice/Makefile
> +++ b/drivers/net/ice/Makefile
> @@ -84,7 +84,4 @@ ifeq ($(CC_AVX2_SUPPORT), 1)
>  endif
>  SRCS-$(CONFIG_RTE_LIBRTE_ICE_PMD) += ice_generic_flow.c
>  
> -# install this header file
> -SYMLINK-$(CONFIG_RTE_LIBRTE_ICE_PMD)-include := rte_pmd_ice.h
> -
>  include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index d74675842..c68acdef0 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -30,6 +30,26 @@ static const char * const ice_valid_args[] = {
>  	NULL
>  };
>  
> +static const struct rte_mbuf_dynfield proto_xtr_metadata_param = {
> +	.name = "proto-xtr-metadata",
> +	.size = sizeof(uint32_t),
> +	.align = __alignof__(uint32_t),
> +	.flags = 0,
> +};
> +
> +struct proto_xtr_ol_flag {
> +	const struct rte_mbuf_dynflag param;
> +	uint64_t *flag;
> +};
> +
> +static const struct proto_xtr_ol_flag proto_xtr_ol_flag_params[] = {
> +	{ { "proto-xtr-ol-vlan", 0 },      &proto_xtr_ol_vlan },
> +	{ { "proto-xtr-ol-ipv4",  0 },     &proto_xtr_ol_ipv4 },
> +	{ { "proto-xtr-ol-ipv6", 0 },      &proto_xtr_ol_ipv6 },
> +	{ { "proto-xtr-ol-ipv6-flow", 0 }, &proto_xtr_ol_ipv6_flow },
> +	{ { "proto-xtr-ol-tcp", 0 },       &proto_xtr_ol_tcp },
> +};

If this fields are going to be used by an application, I think you need
to provide an api to access to the fields.

Solution 1, do the same than in Slava's patch (flow metadata):

- provide a function to do the registration (something like
  rte_pmd_ice_proto_xtr_register()), that sets global offset and masks.
- export these global offset and masks
- provide static inline helpers to access to the dyn fields/flags

Solution 2, without global variable export and helpers:

- define the rte_mbuf_dynfield and rte_mbuf_dynflag structures in
  the .h as static const, so they can be used by the application
- to access to the dynamic fields/flags, the application calls
  rte_mbuf_dynfield_register(&rte_pmd_ice_proto_xtr_metadata_param) to get
  the offset, and store it privately. Then it uses the generic macros
  to access to the field.


>  #define ICE_DFLT_OUTER_TAG_TYPE ICE_AQ_VSI_OUTER_TAG_VLAN_9100
>  
>  /* DDP package search path */
> @@ -1385,6 +1405,9 @@ ice_init_proto_xtr(struct rte_eth_dev *dev)
>  			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
>  	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);
>  	struct ice_hw *hw = ICE_PF_TO_HW(pf);
> +	const struct proto_xtr_ol_flag *ol_flag;
> +	bool proto_xtr_enable = false;
> +	int offset;
>  	uint16_t i;
>  
>  	if (!ice_proto_xtr_support(hw)) {
> @@ -1398,10 +1421,52 @@ ice_init_proto_xtr(struct rte_eth_dev *dev)
>  		return;
>  	}
>  
> -	for (i = 0; i < pf->lan_nb_qps; i++)
> +	for (i = 0; i < pf->lan_nb_qps; i++) {
>  		pf->proto_xtr[i] = ad->devargs.proto_xtr[i] != PROTO_XTR_NONE ?
>  				   ad->devargs.proto_xtr[i] :
>  				   ad->devargs.proto_xtr_dflt;
> +
> +		if (pf->proto_xtr[i] != PROTO_XTR_NONE && !proto_xtr_enable)
> +			proto_xtr_enable = true;
> +	}
> +
> +	if (likely(!proto_xtr_enable))
> +		return;
> +
> +	offset = rte_mbuf_dynfield_register(&proto_xtr_metadata_param);
> +	if (unlikely(offset == -1)) {
> +		PMD_DRV_LOG(ERR,
> +			    "Protocol extraction metadata is disabled in mbuf with error %d",
> +			    -rte_errno);
> +		return;
> +	}

I think it is quite dangerous not to return an error in case the
registration fails. If the application makes use of a helper (in case of
solution 1), it may crash if there is no verification in the helper.

Another thing that can be a problem: from what I see, the registration
of the dynamic field will always be done as soon as the ice port is
configured. But maybe the user does not want to use this feature, and
prefers instead to keep space in mbuf for another purpose. I think this
should be an option.

A possibility is to let the application do the registration through the
registration helper (still solution 1). If the application wants to use
this feature, it will first register the fields/flags, and they will be
filled by the PMD on rx if they are registered.

> +
> +	PMD_DRV_LOG(DEBUG,
> +		    "Protocol extraction metadata offset in mbuf is : %d",
> +		    offset);
> +	proto_xtr_metadata = offset;
> +
> +	for (i = 0; i < RTE_DIM(proto_xtr_ol_flag_params); i++) {
> +		ol_flag = &proto_xtr_ol_flag_params[i];
> +
> +		offset = rte_mbuf_dynflag_register(&ol_flag->param);
> +		if (unlikely(offset == -1)) {
> +			PMD_DRV_LOG(ERR,
> +				    "Protocol extraction offload '%s' failed to register with error %d",
> +				    ol_flag->param.name, -rte_errno);
> +			break;
> +		}
> +
> +		PMD_DRV_LOG(DEBUG,
> +			    "Protocol extraction offload '%s' offset in mbuf is : %d",
> +			    ol_flag->param.name, offset);
> +		*ol_flag->flag = 1ULL << offset;
> +	}
> +
> +	if (i != RTE_DIM(proto_xtr_ol_flag_params)) {
> +		PMD_DRV_LOG(ERR, "Protocol extraction metadata is disabled");
> +		proto_xtr_metadata = -1;
> +	}
>  }
>  
>  /*  Initialize SW parameters of PF */
> diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
> index de67e5934..1ca00bda4 100644
> --- a/drivers/net/ice/ice_ethdev.h
> +++ b/drivers/net/ice/ice_ethdev.h
> @@ -8,6 +8,7 @@
>  #include <rte_kvargs.h>
>  
>  #include <rte_ethdev_driver.h>
> +#include <rte_mbuf_dyn.h>
>  
>  #include "base/ice_common.h"
>  #include "base/ice_adminq_cmd.h"
> @@ -241,6 +242,23 @@ struct ice_vsi {
>  	bool offset_loaded;
>  };
>  
> +enum proto_xtr_type {
> +	PROTO_XTR_NONE,
> +	PROTO_XTR_VLAN,
> +	PROTO_XTR_IPV4,
> +	PROTO_XTR_IPV6,
> +	PROTO_XTR_IPV6_FLOW,
> +	PROTO_XTR_TCP,
> +};
> +
> +extern int proto_xtr_metadata;
> +
> +extern uint64_t proto_xtr_ol_vlan;
> +extern uint64_t proto_xtr_ol_ipv4;
> +extern uint64_t proto_xtr_ol_ipv6;
> +extern uint64_t proto_xtr_ol_ipv6_flow;
> +extern uint64_t proto_xtr_ol_tcp;
> +
>  enum ice_fdir_tunnel_type {
>  	ICE_FDIR_TUNNEL_TYPE_NONE = 0,
>  	ICE_FDIR_TUNNEL_TYPE_VXLAN,
> diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> index 8d4820d3c..6493aa700 100644
> --- a/drivers/net/ice/ice_rxtx.c
> +++ b/drivers/net/ice/ice_rxtx.c
> @@ -13,18 +13,31 @@
>  		PKT_TX_TCP_SEG |		 \
>  		PKT_TX_OUTER_IP_CKSUM)
>  
> -static inline uint8_t
> -ice_rxdid_to_proto_xtr_type(uint8_t rxdid)
> -{
> -	static uint8_t xtr_map[] = {
> -		[ICE_RXDID_COMMS_AUX_VLAN]      = PROTO_XTR_VLAN,
> -		[ICE_RXDID_COMMS_AUX_IPV4]      = PROTO_XTR_IPV4,
> -		[ICE_RXDID_COMMS_AUX_IPV6]      = PROTO_XTR_IPV6,
> -		[ICE_RXDID_COMMS_AUX_IPV6_FLOW] = PROTO_XTR_IPV6_FLOW,
> -		[ICE_RXDID_COMMS_AUX_TCP]       = PROTO_XTR_TCP,
> +/* Offset of mbuf dynamic field for protocol extraction data */
> +int proto_xtr_metadata = -1;
> +
> +/* Mask of mbuf dynamic flags for protocol extraction type */
> +uint64_t proto_xtr_ol_vlan;
> +uint64_t proto_xtr_ol_ipv4;
> +uint64_t proto_xtr_ol_ipv6;
> +uint64_t proto_xtr_ol_ipv6_flow;
> +uint64_t proto_xtr_ol_tcp;
> +
> +static inline uint64_t
> +ice_rxdid_to_proto_xtr_ol_flag(uint8_t rxdid)
> +{
> +	static uint64_t *ol_flag_map[] = {
> +		[ICE_RXDID_COMMS_AUX_VLAN]      = &proto_xtr_ol_vlan,
> +		[ICE_RXDID_COMMS_AUX_IPV4]      = &proto_xtr_ol_ipv4,
> +		[ICE_RXDID_COMMS_AUX_IPV6]      = &proto_xtr_ol_ipv6,
> +		[ICE_RXDID_COMMS_AUX_IPV6_FLOW] = &proto_xtr_ol_ipv6_flow,
> +		[ICE_RXDID_COMMS_AUX_TCP]       = &proto_xtr_ol_tcp,
>  	};
> +	uint64_t *ol_flag;
>  
> -	return rxdid < RTE_DIM(xtr_map) ? xtr_map[rxdid] : PROTO_XTR_NONE;
> +	ol_flag = rxdid < RTE_DIM(ol_flag_map) ? ol_flag_map[rxdid] : NULL;
> +
> +	return ol_flag != NULL ? *ol_flag : 0;

Just an observation: I wonder if it wouldn't be more efficient to
register a 3 consecutive flags, and use it as a value: 0=none, 1=vlan,
2=ipv4, 3=ipv6, ...
Unfortunately, there is no 'count' parameter to the
rte_mbuf_dynflag_register() function, but I think is doable easily.


>  }
>  
>  static inline uint8_t
> @@ -1325,10 +1338,38 @@ ice_rxd_to_vlan_tci(struct rte_mbuf *mb, volatile union ice_rx_flex_desc *rxdp)
>  		   mb->vlan_tci, mb->vlan_tci_outer);
>  }
>  
> +#ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
>  #define ICE_RX_PROTO_XTR_VALID \
>  	((1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD4_VALID_S) | \
>  	 (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD5_VALID_S))
>  
> +static void
> +ice_rxd_to_proto_xtr(struct rte_mbuf *mb,
> +		     volatile struct ice_32b_rx_flex_desc_comms *desc)
> +{
> +	uint16_t stat_err = rte_le_to_cpu_16(desc->status_error1);
> +	uint32_t metadata;
> +	uint64_t ol_flag;
> +
> +	if (unlikely(!(stat_err & ICE_RX_PROTO_XTR_VALID)))
> +		return;
> +
> +	ol_flag = ice_rxdid_to_proto_xtr_ol_flag(desc->rxdid);
> +	if (unlikely(!ol_flag))
> +		return;
> +
> +	mb->ol_flags |= ol_flag;
> +
> +	metadata = stat_err & (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD4_VALID_S) ?
> +				rte_le_to_cpu_16(desc->flex_ts.flex.aux0) : 0;
> +
> +	if (likely(stat_err & (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD5_VALID_S)))
> +		metadata |= rte_le_to_cpu_16(desc->flex_ts.flex.aux1) << 16;
> +
> +	*RTE_MBUF_DYNFIELD(mb, proto_xtr_metadata, uint32_t *) = metadata;
> +}
> +#endif
> +
>  static inline void
>  ice_rxd_to_pkt_fields(struct rte_mbuf *mb,
>  		      volatile union ice_rx_flex_desc *rxdp)
> @@ -1344,28 +1385,13 @@ ice_rxd_to_pkt_fields(struct rte_mbuf *mb,
>  	}
>  
>  #ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
> -	init_proto_xtr_flds(mb);
> -
> -	stat_err = rte_le_to_cpu_16(desc->status_error1);
> -	if (stat_err & ICE_RX_PROTO_XTR_VALID) {
> -		struct proto_xtr_flds *xtr = get_proto_xtr_flds(mb);
> -
> -		if (stat_err & (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD4_VALID_S))
> -			xtr->u.raw.data0 =
> -				rte_le_to_cpu_16(desc->flex_ts.flex.aux0);
> -
> -		if (stat_err & (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD5_VALID_S))
> -			xtr->u.raw.data1 =
> -				rte_le_to_cpu_16(desc->flex_ts.flex.aux1);
> -
> -		xtr->type = ice_rxdid_to_proto_xtr_type(desc->rxdid);
> -		xtr->magic = PROTO_XTR_MAGIC_ID;
> -	}
> -
>  	if (desc->flow_id != 0xFFFFFFFF) {
>  		mb->ol_flags |= PKT_RX_FDIR | PKT_RX_FDIR_ID;
>  		mb->hash.fdir.hi = rte_le_to_cpu_32(desc->flow_id);
>  	}
> +
> +	if (unlikely(proto_xtr_metadata != -1))
> +		ice_rxd_to_proto_xtr(mb, desc);
>  #endif
>  }
>  
> diff --git a/drivers/net/ice/ice_rxtx.h b/drivers/net/ice/ice_rxtx.h
> index 5de618976..9e3d2cd07 100644
> --- a/drivers/net/ice/ice_rxtx.h
> +++ b/drivers/net/ice/ice_rxtx.h
> @@ -5,7 +5,6 @@
>  #ifndef _ICE_RXTX_H_
>  #define _ICE_RXTX_H_
>  
> -#include "rte_pmd_ice.h"
>  #include "ice_ethdev.h"
>  
>  #define ICE_ALIGN_RING_DESC  32
> diff --git a/drivers/net/ice/meson.build b/drivers/net/ice/meson.build
> index f9e897bbc..45e760063 100644
> --- a/drivers/net/ice/meson.build
> +++ b/drivers/net/ice/meson.build
> @@ -36,5 +36,3 @@ if arch_subdir == 'x86'
>  		objs += ice_avx2_lib.extract_objects('ice_rxtx_vec_avx2.c')
>  	endif
>  endif
> -
> -install_headers('rte_pmd_ice.h')
> diff --git a/drivers/net/ice/rte_pmd_ice.h b/drivers/net/ice/rte_pmd_ice.h
> deleted file mode 100644
> index 719487e1e..000000000
> --- a/drivers/net/ice/rte_pmd_ice.h
> +++ /dev/null
> @@ -1,152 +0,0 @@
> -/* SPDX-License-Identifier: BSD-3-Clause
> - * Copyright(c) 2019 Intel Corporation
> - */
> -
> -#ifndef _RTE_PMD_ICE_H_
> -#define _RTE_PMD_ICE_H_
> -
> -#include <stdio.h>
> -#include <rte_mbuf.h>
> -#include <rte_ethdev.h>
> -
> -#ifdef __cplusplus
> -extern "C" {
> -#endif
> -
> -enum proto_xtr_type {
> -	PROTO_XTR_NONE,
> -	PROTO_XTR_VLAN,
> -	PROTO_XTR_IPV4,
> -	PROTO_XTR_IPV6,
> -	PROTO_XTR_IPV6_FLOW,
> -	PROTO_XTR_TCP,
> -};
> -
> -struct proto_xtr_flds {
> -	union {
> -		struct {
> -			uint16_t data0;
> -			uint16_t data1;
> -		} raw;
> -		struct {
> -			uint16_t stag_vid:12,
> -				 stag_dei:1,
> -				 stag_pcp:3;
> -			uint16_t ctag_vid:12,
> -				 ctag_dei:1,
> -				 ctag_pcp:3;
> -		} vlan;
> -		struct {
> -			uint16_t protocol:8,
> -				 ttl:8;
> -			uint16_t tos:8,
> -				 ihl:4,
> -				 version:4;
> -		} ipv4;
> -		struct {
> -			uint16_t hoplimit:8,
> -				 nexthdr:8;
> -			uint16_t flowhi4:4,
> -				 tc:8,
> -				 version:4;
> -		} ipv6;
> -		struct {
> -			uint16_t flowlo16;
> -			uint16_t flowhi4:4,
> -				 tc:8,
> -				 version:4;
> -		} ipv6_flow;
> -		struct {
> -			uint16_t fin:1,
> -				 syn:1,
> -				 rst:1,
> -				 psh:1,
> -				 ack:1,
> -				 urg:1,
> -				 ece:1,
> -				 cwr:1,
> -				 res1:4,
> -				 doff:4;
> -			uint16_t rsvd;
> -		} tcp;
> -	} u;
> -
> -	uint16_t rsvd;
> -
> -	uint8_t type;
> -
> -#define PROTO_XTR_MAGIC_ID	0xCE
> -	uint8_t magic;
> -};
> -
> -static inline void
> -init_proto_xtr_flds(struct rte_mbuf *mb)
> -{
> -	mb->udata64 = 0;
> -}
> -
> -static inline struct proto_xtr_flds *
> -get_proto_xtr_flds(struct rte_mbuf *mb)
> -{
> -	RTE_BUILD_BUG_ON(sizeof(struct proto_xtr_flds) > sizeof(mb->udata64));
> -
> -	return (struct proto_xtr_flds *)&mb->udata64;
> -}
> -
> -static inline void
> -dump_proto_xtr_flds(struct rte_mbuf *mb)
> -{
> -	struct proto_xtr_flds *xtr = get_proto_xtr_flds(mb);
> -
> -	if (xtr->magic != PROTO_XTR_MAGIC_ID || xtr->type == PROTO_XTR_NONE)
> -		return;
> -
> -	printf(" - Protocol Extraction:[0x%04x:0x%04x],",
> -	       xtr->u.raw.data0, xtr->u.raw.data1);
> -
> -	if (xtr->type == PROTO_XTR_VLAN)
> -		printf("vlan,stag=%u:%u:%u,ctag=%u:%u:%u ",
> -		       xtr->u.vlan.stag_pcp,
> -		       xtr->u.vlan.stag_dei,
> -		       xtr->u.vlan.stag_vid,
> -		       xtr->u.vlan.ctag_pcp,
> -		       xtr->u.vlan.ctag_dei,
> -		       xtr->u.vlan.ctag_vid);
> -	else if (xtr->type == PROTO_XTR_IPV4)
> -		printf("ipv4,ver=%u,hdrlen=%u,tos=%u,ttl=%u,proto=%u ",
> -		       xtr->u.ipv4.version,
> -		       xtr->u.ipv4.ihl,
> -		       xtr->u.ipv4.tos,
> -		       xtr->u.ipv4.ttl,
> -		       xtr->u.ipv4.protocol);
> -	else if (xtr->type == PROTO_XTR_IPV6)
> -		printf("ipv6,ver=%u,tc=%u,flow_hi4=0x%x,nexthdr=%u,hoplimit=%u ",
> -		       xtr->u.ipv6.version,
> -		       xtr->u.ipv6.tc,
> -		       xtr->u.ipv6.flowhi4,
> -		       xtr->u.ipv6.nexthdr,
> -		       xtr->u.ipv6.hoplimit);
> -	else if (xtr->type == PROTO_XTR_IPV6_FLOW)
> -		printf("ipv6_flow,ver=%u,tc=%u,flow=0x%x%04x ",
> -		       xtr->u.ipv6_flow.version,
> -		       xtr->u.ipv6_flow.tc,
> -		       xtr->u.ipv6_flow.flowhi4,
> -		       xtr->u.ipv6_flow.flowlo16);
> -	else if (xtr->type == PROTO_XTR_TCP)
> -		printf("tcp,doff=%u,flags=%s%s%s%s%s%s%s%s ",
> -		       xtr->u.tcp.doff,
> -		       xtr->u.tcp.cwr ? "C" : "",
> -		       xtr->u.tcp.ece ? "E" : "",
> -		       xtr->u.tcp.urg ? "U" : "",
> -		       xtr->u.tcp.ack ? "A" : "",
> -		       xtr->u.tcp.psh ? "P" : "",
> -		       xtr->u.tcp.rst ? "R" : "",
> -		       xtr->u.tcp.syn ? "S" : "",
> -		       xtr->u.tcp.fin ? "F" : "");
> -}
> -
> -#ifdef __cplusplus
> -}
> -#endif
> -
> -#endif /* _RTE_PMD_ICE_H_ */
> -- 
> 2.17.1
>
Haiyue Wang Oct. 31, 2019, 1:16 a.m. UTC | #2
Hi Olivier,

Very appreciate your response, please see reply below.

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Thursday, October 31, 2019 00:56
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/ice: optimize protocol extraction by dynamic mbuf API
> 
> Hi Haiyue,
> 
> Please see some comments below.
> 
> On Tue, Oct 29, 2019 at 03:34:48PM +0800, Haiyue Wang wrote:
> > The original design is to use rte_mbuf::udata64 to save the metadata of
> > protocol extraction which has network protocol data fields and type, an
> 
> nit: an -> a

Fix next version, thanks.

> 
> > private API is used to decode this metadata. It is not so generic.
> >
> > Use the new dynamic mbuf field and flags to handle protocol extraction.
> > Then the metadata is very clean, only network protocol fields extracted
> > by defined offset, and its type will be indicated by related ol_flags.
> >
> > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> > ---
> 
> Before the patch, the result of a specific hw parsing was stored in
> m->udata on Rx and this information is supposed to used by an
> application or a library, is that correct?
> 

Yes.

> 
> > v2: - disable the protocol extraction if failed to register some ol_flags
> >     - rewrite the commit message
> >
> >  doc/guides/nics/ice.rst       |  22 +++--
> >  drivers/net/ice/Makefile      |   3 -
> >  drivers/net/ice/ice_ethdev.c  |  67 ++++++++++++++-
> >  drivers/net/ice/ice_ethdev.h  |  18 ++++
> >  drivers/net/ice/ice_rxtx.c    |  82 +++++++++++-------
> >  drivers/net/ice/ice_rxtx.h    |   1 -
> >  drivers/net/ice/meson.build   |   2 -
> >  drivers/net/ice/rte_pmd_ice.h | 152 ----------------------------------
> >  8 files changed, 152 insertions(+), 195 deletions(-)
> >  delete mode 100644 drivers/net/ice/rte_pmd_ice.h
> >
> > diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> > index 933f63480..f460d1cfd 100644
> > --- a/doc/guides/nics/ice.rst
> > +++ b/doc/guides/nics/ice.rst
> > @@ -82,8 +82,8 @@ Runtime Config Options
> >
> >  - ``Protocol extraction for per queue``
> >
> > -  Configure the RX queues to do protocol extraction into ``rte_mbuf::udata64``
> > -  for protocol handling acceleration, like checking the TCP SYN packets quickly.
> > +  Configure the RX queues to do protocol extraction into mbuf for protocol
> > +  handling acceleration, like checking the TCP SYN packets quickly.
> >
> >    The argument format is::
> >
> > @@ -111,7 +111,9 @@ Runtime Config Options
> >    This setting means queues 1, 2-3, 8-9 are TCP extraction, queues 10-23 are
> >    IPv6 extraction, other queues use the default VLAN extraction.
> >
> > -  The extraction will be copied into the lower 32 bit of ``rte_mbuf::udata64``.
> > +  The extraction metadata will be copied into the registered dynamic mbuf field
> > +  with name: "proto-xtr-metadata", size: 4B, align: __alignof__(uint32_t), and
> > +  the related dynamic mbuf flags in ``rte_mbuf::ol_flags`` will be set.
> >
> >    .. table:: Protocol extraction : ``vlan``
> >
> > @@ -125,6 +127,8 @@ Runtime Config Options
> >
> >    VLAN2 - C-VLAN (second for QinQ).
> >
> > +  Dynamic mbuf flag : "proto-xtr-ol-vlan"
> > +
> >    .. table:: Protocol extraction : ``ipv4``
> >
> >     +----------------------------+----------------------------+
> > @@ -137,6 +141,8 @@ Runtime Config Options
> >
> >    IPHDR2 - IPv4 header word 0, "Ver", "Hdr Len" and "Type of Service" fields.
> >
> > +  Dynamic mbuf flag : "proto-xtr-ol-ipv4"
> > +
> >    .. table:: Protocol extraction : ``ipv6``
> >
> >     +----------------------------+----------------------------+
> > @@ -150,6 +156,8 @@ Runtime Config Options
> >    IPHDR2 - IPv6 header word 0, "Ver", "Traffic class" and high 4 bits of
> >    "Flow Label" fields.
> >
> > +  Dynamic mbuf flag : "proto-xtr-ol-ipv6"
> > +
> >    .. table:: Protocol extraction : ``ipv6_flow``
> >
> >     +----------------------------+----------------------------+
> > @@ -163,6 +171,8 @@ Runtime Config Options
> >    IPHDR2 - IPv6 header word 0, "Ver", "Traffic class" and high 4 bits of
> >    "Flow Label" fields.
> >
> > +  Dynamic mbuf flag : "proto-xtr-ol-ipv6-flow"
> > +
> >    .. table:: Protocol extraction : ``tcp``
> >
> >     +----------------------------+----------------------------+
> > @@ -175,11 +185,7 @@ Runtime Config Options
> >
> >    TCPHDR2 - Reserved
> >
> > -  Use ``get_proto_xtr_flds(struct rte_mbuf *mb)`` to access the protocol
> > -  extraction, do not use ``rte_mbuf::udata64`` directly.
> > -
> > -  The ``dump_proto_xtr_flds(struct rte_mbuf *mb)`` routine shows how to
> > -  access the protocol extraction result in ``struct rte_mbuf``.
> > +  Dynamic mbuf flag : "proto-xtr-ol-tcp"
> >
> >  Driver compilation and testing
> >  ------------------------------
> > diff --git a/drivers/net/ice/Makefile b/drivers/net/ice/Makefile
> > index 7c3b6a7ff..a1817e14c 100644
> > --- a/drivers/net/ice/Makefile
> > +++ b/drivers/net/ice/Makefile
> > @@ -84,7 +84,4 @@ ifeq ($(CC_AVX2_SUPPORT), 1)
> >  endif
> >  SRCS-$(CONFIG_RTE_LIBRTE_ICE_PMD) += ice_generic_flow.c
> >
> > -# install this header file
> > -SYMLINK-$(CONFIG_RTE_LIBRTE_ICE_PMD)-include := rte_pmd_ice.h
> > -
> >  include $(RTE_SDK)/mk/rte.lib.mk
> > diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> > index d74675842..c68acdef0 100644
> > --- a/drivers/net/ice/ice_ethdev.c
> > +++ b/drivers/net/ice/ice_ethdev.c
> > @@ -30,6 +30,26 @@ static const char * const ice_valid_args[] = {
> >  	NULL
> >  };
> >
> > +static const struct rte_mbuf_dynfield proto_xtr_metadata_param = {
> > +	.name = "proto-xtr-metadata",
> > +	.size = sizeof(uint32_t),
> > +	.align = __alignof__(uint32_t),
> > +	.flags = 0,
> > +};
> > +
> > +struct proto_xtr_ol_flag {
> > +	const struct rte_mbuf_dynflag param;
> > +	uint64_t *flag;
> > +};
> > +
> > +static const struct proto_xtr_ol_flag proto_xtr_ol_flag_params[] = {
> > +	{ { "proto-xtr-ol-vlan", 0 },      &proto_xtr_ol_vlan },
> > +	{ { "proto-xtr-ol-ipv4",  0 },     &proto_xtr_ol_ipv4 },
> > +	{ { "proto-xtr-ol-ipv6", 0 },      &proto_xtr_ol_ipv6 },
> > +	{ { "proto-xtr-ol-ipv6-flow", 0 }, &proto_xtr_ol_ipv6_flow },
> > +	{ { "proto-xtr-ol-tcp", 0 },       &proto_xtr_ol_tcp },
> > +};
> 
> If this fields are going to be used by an application, I think you need
> to provide an api to access to the fields.
> 
> Solution 1, do the same than in Slava's patch (flow metadata):
> 
> - provide a function to do the registration (something like
>   rte_pmd_ice_proto_xtr_register()), that sets global offset and masks.
> - export these global offset and masks
> - provide static inline helpers to access to the dyn fields/flags
> 
> Solution 2, without global variable export and helpers:
> 
> - define the rte_mbuf_dynfield and rte_mbuf_dynflag structures in
>   the .h as static const, so they can be used by the application
> - to access to the dynamic fields/flags, the application calls
>   rte_mbuf_dynfield_register(&rte_pmd_ice_proto_xtr_metadata_param) to get
>   the offset, and store it privately. Then it uses the generic macros
>   to access to the field.
> 
> 

Good practice for reducing the application's effort, I will refer
to Slava's design.

> >  #define ICE_DFLT_OUTER_TAG_TYPE ICE_AQ_VSI_OUTER_TAG_VLAN_9100
> >
> >  /* DDP package search path */
> > @@ -1385,6 +1405,9 @@ ice_init_proto_xtr(struct rte_eth_dev *dev)
> >  			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> >  	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> >  	struct ice_hw *hw = ICE_PF_TO_HW(pf);
> > +	const struct proto_xtr_ol_flag *ol_flag;
> > +	bool proto_xtr_enable = false;
> > +	int offset;
> >  	uint16_t i;
> >
> >  	if (!ice_proto_xtr_support(hw)) {
> > @@ -1398,10 +1421,52 @@ ice_init_proto_xtr(struct rte_eth_dev *dev)
> >  		return;
> >  	}
> >
> > -	for (i = 0; i < pf->lan_nb_qps; i++)
> > +	for (i = 0; i < pf->lan_nb_qps; i++) {
> >  		pf->proto_xtr[i] = ad->devargs.proto_xtr[i] != PROTO_XTR_NONE ?
> >  				   ad->devargs.proto_xtr[i] :
> >  				   ad->devargs.proto_xtr_dflt;
> > +
> > +		if (pf->proto_xtr[i] != PROTO_XTR_NONE && !proto_xtr_enable)
> > +			proto_xtr_enable = true;
> > +	}
> > +
> > +	if (likely(!proto_xtr_enable))
> > +		return;
> > +
> > +	offset = rte_mbuf_dynfield_register(&proto_xtr_metadata_param);
> > +	if (unlikely(offset == -1)) {
> > +		PMD_DRV_LOG(ERR,
> > +			    "Protocol extraction metadata is disabled in mbuf with error %d",
> > +			    -rte_errno);
> > +		return;
> > +	}
> 
> I think it is quite dangerous not to return an error in case the
> registration fails. If the application makes use of a helper (in case of
> solution 1), it may crash if there is no verification in the helper.
> 
> Another thing that can be a problem: from what I see, the registration
> of the dynamic field will always be done as soon as the ice port is
> configured. But maybe the user does not want to use this feature, and
> prefers instead to keep space in mbuf for another purpose. I think this
> should be an option.
> 
> A possibility is to let the application do the registration through the
> registration helper (still solution 1). If the application wants to use
> this feature, it will first register the fields/flags, and they will be
> filled by the PMD on rx if they are registered.
> 

In fact, this feature is only enabled explicitly by "-w 18:00.0,proto_xtr=vlan",
by default, it is disabled. In other words, if the application pass the 'proto_xtr'
dev_args, the ice PMD will call 'rte_mbuf_dynXXX_register' instead, the application
jus needs to call 'rte_mbuf_dynXXX_lookup' to get the offset.

So looks like Solution 2 is better ? Since the application just needs to know
the name, size, align.

	for (i = 0; i < pf->lan_nb_qps; i++) {
		pf->proto_xtr[i] = ad->devargs.proto_xtr[i] != PROTO_XTR_NONE ?
				   ad->devargs.proto_xtr[i] :
				   ad->devargs.proto_xtr_dflt;

		if (pf->proto_xtr[i] != PROTO_XTR_NONE && !proto_xtr_enable)
			proto_xtr_enable = true;
	}

	if (likely(!proto_xtr_enable)) <---- Will return gracefully if no 'proto_xtr' dev_args.
		return;

	offset = rte_mbuf_dynfield_register(&proto_xtr_metadata_param);

> > +
> > +	PMD_DRV_LOG(DEBUG,
> > +		    "Protocol extraction metadata offset in mbuf is : %d",
> > +		    offset);
> > +	proto_xtr_metadata = offset;
> > +
> > +	for (i = 0; i < RTE_DIM(proto_xtr_ol_flag_params); i++) {
> > +		ol_flag = &proto_xtr_ol_flag_params[i];
> > +
> > +		offset = rte_mbuf_dynflag_register(&ol_flag->param);
> > +		if (unlikely(offset == -1)) {
> > +			PMD_DRV_LOG(ERR,
> > +				    "Protocol extraction offload '%s' failed to register with error %d",
> > +				    ol_flag->param.name, -rte_errno);
> > +			break;
> > +		}
> > +
> > +		PMD_DRV_LOG(DEBUG,
> > +			    "Protocol extraction offload '%s' offset in mbuf is : %d",
> > +			    ol_flag->param.name, offset);
> > +		*ol_flag->flag = 1ULL << offset;
> > +	}
> > +
> > +	if (i != RTE_DIM(proto_xtr_ol_flag_params)) {
> > +		PMD_DRV_LOG(ERR, "Protocol extraction metadata is disabled");
> > +		proto_xtr_metadata = -1;
> > +	}
> >  }
> >
> >  /*  Initialize SW parameters of PF */
> > diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
> > index de67e5934..1ca00bda4 100644
> > --- a/drivers/net/ice/ice_ethdev.h
> > +++ b/drivers/net/ice/ice_ethdev.h
> > @@ -8,6 +8,7 @@
> >  #include <rte_kvargs.h>
> >
> >  #include <rte_ethdev_driver.h>
> > +#include <rte_mbuf_dyn.h>
> >
> >  #include "base/ice_common.h"
> >  #include "base/ice_adminq_cmd.h"
> > @@ -241,6 +242,23 @@ struct ice_vsi {
> >  	bool offset_loaded;
> >  };
> >
> > +enum proto_xtr_type {
> > +	PROTO_XTR_NONE,
> > +	PROTO_XTR_VLAN,
> > +	PROTO_XTR_IPV4,
> > +	PROTO_XTR_IPV6,
> > +	PROTO_XTR_IPV6_FLOW,
> > +	PROTO_XTR_TCP,
> > +};
> > +
> > +extern int proto_xtr_metadata;
> > +
> > +extern uint64_t proto_xtr_ol_vlan;
> > +extern uint64_t proto_xtr_ol_ipv4;
> > +extern uint64_t proto_xtr_ol_ipv6;
> > +extern uint64_t proto_xtr_ol_ipv6_flow;
> > +extern uint64_t proto_xtr_ol_tcp;
> > +
> >  enum ice_fdir_tunnel_type {
> >  	ICE_FDIR_TUNNEL_TYPE_NONE = 0,
> >  	ICE_FDIR_TUNNEL_TYPE_VXLAN,
> > diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> > index 8d4820d3c..6493aa700 100644
> > --- a/drivers/net/ice/ice_rxtx.c
> > +++ b/drivers/net/ice/ice_rxtx.c
> > @@ -13,18 +13,31 @@
> >  		PKT_TX_TCP_SEG |		 \
> >  		PKT_TX_OUTER_IP_CKSUM)
> >
> > -static inline uint8_t
> > -ice_rxdid_to_proto_xtr_type(uint8_t rxdid)
> > -{
> > -	static uint8_t xtr_map[] = {
> > -		[ICE_RXDID_COMMS_AUX_VLAN]      = PROTO_XTR_VLAN,
> > -		[ICE_RXDID_COMMS_AUX_IPV4]      = PROTO_XTR_IPV4,
> > -		[ICE_RXDID_COMMS_AUX_IPV6]      = PROTO_XTR_IPV6,
> > -		[ICE_RXDID_COMMS_AUX_IPV6_FLOW] = PROTO_XTR_IPV6_FLOW,
> > -		[ICE_RXDID_COMMS_AUX_TCP]       = PROTO_XTR_TCP,
> > +/* Offset of mbuf dynamic field for protocol extraction data */
> > +int proto_xtr_metadata = -1;
> > +
> > +/* Mask of mbuf dynamic flags for protocol extraction type */
> > +uint64_t proto_xtr_ol_vlan;
> > +uint64_t proto_xtr_ol_ipv4;
> > +uint64_t proto_xtr_ol_ipv6;
> > +uint64_t proto_xtr_ol_ipv6_flow;
> > +uint64_t proto_xtr_ol_tcp;
> > +
> > +static inline uint64_t
> > +ice_rxdid_to_proto_xtr_ol_flag(uint8_t rxdid)
> > +{
> > +	static uint64_t *ol_flag_map[] = {
> > +		[ICE_RXDID_COMMS_AUX_VLAN]      = &proto_xtr_ol_vlan,
> > +		[ICE_RXDID_COMMS_AUX_IPV4]      = &proto_xtr_ol_ipv4,
> > +		[ICE_RXDID_COMMS_AUX_IPV6]      = &proto_xtr_ol_ipv6,
> > +		[ICE_RXDID_COMMS_AUX_IPV6_FLOW] = &proto_xtr_ol_ipv6_flow,
> > +		[ICE_RXDID_COMMS_AUX_TCP]       = &proto_xtr_ol_tcp,
> >  	};
> > +	uint64_t *ol_flag;
> >
> > -	return rxdid < RTE_DIM(xtr_map) ? xtr_map[rxdid] : PROTO_XTR_NONE;
> > +	ol_flag = rxdid < RTE_DIM(ol_flag_map) ? ol_flag_map[rxdid] : NULL;
> > +
> > +	return ol_flag != NULL ? *ol_flag : 0;
> 
> Just an observation: I wonder if it wouldn't be more efficient to
> register a 3 consecutive flags, and use it as a value: 0=none, 1=vlan,
> 2=ipv4, 3=ipv6, ...
> Unfortunately, there is no 'count' parameter to the
> rte_mbuf_dynflag_register() function, but I think is doable easily.
> 

Yes, then we can save the flags bits if the count is BIGGER, but no
more resources, and for the registered flags can't be freed now, return
error directly will make other less flags bits request fit. And more,
no need to loop like this:

	for (i = 0; i < RTE_DIM(proto_xtr_ol_flag_params); i++) {
		ol_flag = &proto_xtr_ol_flag_params[i];

		offset = rte_mbuf_dynflag_register(&ol_flag->param);
		if (unlikely(offset == -1)) {
			break;
		}

A new API may be born. ;-)

> 
> >  }
> >
> >  static inline uint8_t
> > @@ -1325,10 +1338,38 @@ ice_rxd_to_vlan_tci(struct rte_mbuf *mb, volatile union ice_rx_flex_desc
> *rxdp)
> >  		   mb->vlan_tci, mb->vlan_tci_outer);
> >  }
> >
> > +#ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
> >  #define ICE_RX_PROTO_XTR_VALID \
> >  	((1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD4_VALID_S) | \
> >  	 (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD5_VALID_S))
> >
> > +static void
> > +ice_rxd_to_proto_xtr(struct rte_mbuf *mb,
> > +		     volatile struct ice_32b_rx_flex_desc_comms *desc)
> > +{
> > +	uint16_t stat_err = rte_le_to_cpu_16(desc->status_error1);
> > +	uint32_t metadata;
> > +	uint64_t ol_flag;
> > +
> > +	if (unlikely(!(stat_err & ICE_RX_PROTO_XTR_VALID)))
> > +		return;
> > +
> > +	ol_flag = ice_rxdid_to_proto_xtr_ol_flag(desc->rxdid);
> > +	if (unlikely(!ol_flag))
> > +		return;
> > +
> > +	mb->ol_flags |= ol_flag;
> > +
> > +	metadata = stat_err & (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD4_VALID_S) ?
> > +				rte_le_to_cpu_16(desc->flex_ts.flex.aux0) : 0;
> > +
> > +	if (likely(stat_err & (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD5_VALID_S)))
> > +		metadata |= rte_le_to_cpu_16(desc->flex_ts.flex.aux1) << 16;
> > +
> > +	*RTE_MBUF_DYNFIELD(mb, proto_xtr_metadata, uint32_t *) = metadata;
> > +}
> > +#endif
> > +
> >  static inline void
> >  ice_rxd_to_pkt_fields(struct rte_mbuf *mb,
> >  		      volatile union ice_rx_flex_desc *rxdp)
> > @@ -1344,28 +1385,13 @@ ice_rxd_to_pkt_fields(struct rte_mbuf *mb,
> >  	}
> >
> >  #ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
> > -	init_proto_xtr_flds(mb);
> > -
> > -	stat_err = rte_le_to_cpu_16(desc->status_error1);
> > -	if (stat_err & ICE_RX_PROTO_XTR_VALID) {
> > -		struct proto_xtr_flds *xtr = get_proto_xtr_flds(mb);
> > -
> > -		if (stat_err & (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD4_VALID_S))
> > -			xtr->u.raw.data0 =
> > -				rte_le_to_cpu_16(desc->flex_ts.flex.aux0);
> > -
> > -		if (stat_err & (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD5_VALID_S))
> > -			xtr->u.raw.data1 =
> > -				rte_le_to_cpu_16(desc->flex_ts.flex.aux1);
> > -
> > -		xtr->type = ice_rxdid_to_proto_xtr_type(desc->rxdid);
> > -		xtr->magic = PROTO_XTR_MAGIC_ID;
> > -	}
> > -
> >  	if (desc->flow_id != 0xFFFFFFFF) {
> >  		mb->ol_flags |= PKT_RX_FDIR | PKT_RX_FDIR_ID;
> >  		mb->hash.fdir.hi = rte_le_to_cpu_32(desc->flow_id);
> >  	}
> > +
> > +	if (unlikely(proto_xtr_metadata != -1))
> > +		ice_rxd_to_proto_xtr(mb, desc);
> >  #endif
> >  }
> >
> > diff --git a/drivers/net/ice/ice_rxtx.h b/drivers/net/ice/ice_rxtx.h
> > index 5de618976..9e3d2cd07 100644
> > --- a/drivers/net/ice/ice_rxtx.h
> > +++ b/drivers/net/ice/ice_rxtx.h
> > @@ -5,7 +5,6 @@
> >  #ifndef _ICE_RXTX_H_
> >  #define _ICE_RXTX_H_
> >
> > -#include "rte_pmd_ice.h"
> >  #include "ice_ethdev.h"
> >
> >  #define ICE_ALIGN_RING_DESC  32
> > diff --git a/drivers/net/ice/meson.build b/drivers/net/ice/meson.build
> > index f9e897bbc..45e760063 100644
> > --- a/drivers/net/ice/meson.build
> > +++ b/drivers/net/ice/meson.build
> > @@ -36,5 +36,3 @@ if arch_subdir == 'x86'
> >  		objs += ice_avx2_lib.extract_objects('ice_rxtx_vec_avx2.c')
> >  	endif
> >  endif
> > -
> > -install_headers('rte_pmd_ice.h')
> > diff --git a/drivers/net/ice/rte_pmd_ice.h b/drivers/net/ice/rte_pmd_ice.h
> > deleted file mode 100644
> > index 719487e1e..000000000
> > --- a/drivers/net/ice/rte_pmd_ice.h
> > +++ /dev/null
> > @@ -1,152 +0,0 @@
> > -/* SPDX-License-Identifier: BSD-3-Clause
> > - * Copyright(c) 2019 Intel Corporation
> > - */
> > -
> > -#ifndef _RTE_PMD_ICE_H_
> > -#define _RTE_PMD_ICE_H_
> > -
> > -#include <stdio.h>
> > -#include <rte_mbuf.h>
> > -#include <rte_ethdev.h>
> > -
> > -#ifdef __cplusplus
> > -extern "C" {
> > -#endif
> > -
> > -enum proto_xtr_type {
> > -	PROTO_XTR_NONE,
> > -	PROTO_XTR_VLAN,
> > -	PROTO_XTR_IPV4,
> > -	PROTO_XTR_IPV6,
> > -	PROTO_XTR_IPV6_FLOW,
> > -	PROTO_XTR_TCP,
> > -};
> > -
> > -struct proto_xtr_flds {
> > -	union {
> > -		struct {
> > -			uint16_t data0;
> > -			uint16_t data1;
> > -		} raw;
> > -		struct {
> > -			uint16_t stag_vid:12,
> > -				 stag_dei:1,
> > -				 stag_pcp:3;
> > -			uint16_t ctag_vid:12,
> > -				 ctag_dei:1,
> > -				 ctag_pcp:3;
> > -		} vlan;
> > -		struct {
> > -			uint16_t protocol:8,
> > -				 ttl:8;
> > -			uint16_t tos:8,
> > -				 ihl:4,
> > -				 version:4;
> > -		} ipv4;
> > -		struct {
> > -			uint16_t hoplimit:8,
> > -				 nexthdr:8;
> > -			uint16_t flowhi4:4,
> > -				 tc:8,
> > -				 version:4;
> > -		} ipv6;
> > -		struct {
> > -			uint16_t flowlo16;
> > -			uint16_t flowhi4:4,
> > -				 tc:8,
> > -				 version:4;
> > -		} ipv6_flow;
> > -		struct {
> > -			uint16_t fin:1,
> > -				 syn:1,
> > -				 rst:1,
> > -				 psh:1,
> > -				 ack:1,
> > -				 urg:1,
> > -				 ece:1,
> > -				 cwr:1,
> > -				 res1:4,
> > -				 doff:4;
> > -			uint16_t rsvd;
> > -		} tcp;
> > -	} u;
> > -
> > -	uint16_t rsvd;
> > -
> > -	uint8_t type;
> > -
> > -#define PROTO_XTR_MAGIC_ID	0xCE
> > -	uint8_t magic;
> > -};
> > -
> > -static inline void
> > -init_proto_xtr_flds(struct rte_mbuf *mb)
> > -{
> > -	mb->udata64 = 0;
> > -}
> > -
> > -static inline struct proto_xtr_flds *
> > -get_proto_xtr_flds(struct rte_mbuf *mb)
> > -{
> > -	RTE_BUILD_BUG_ON(sizeof(struct proto_xtr_flds) > sizeof(mb->udata64));
> > -
> > -	return (struct proto_xtr_flds *)&mb->udata64;
> > -}
> > -
> > -static inline void
> > -dump_proto_xtr_flds(struct rte_mbuf *mb)
> > -{
> > -	struct proto_xtr_flds *xtr = get_proto_xtr_flds(mb);
> > -
> > -	if (xtr->magic != PROTO_XTR_MAGIC_ID || xtr->type == PROTO_XTR_NONE)
> > -		return;
> > -
> > -	printf(" - Protocol Extraction:[0x%04x:0x%04x],",
> > -	       xtr->u.raw.data0, xtr->u.raw.data1);
> > -
> > -	if (xtr->type == PROTO_XTR_VLAN)
> > -		printf("vlan,stag=%u:%u:%u,ctag=%u:%u:%u ",
> > -		       xtr->u.vlan.stag_pcp,
> > -		       xtr->u.vlan.stag_dei,
> > -		       xtr->u.vlan.stag_vid,
> > -		       xtr->u.vlan.ctag_pcp,
> > -		       xtr->u.vlan.ctag_dei,
> > -		       xtr->u.vlan.ctag_vid);
> > -	else if (xtr->type == PROTO_XTR_IPV4)
> > -		printf("ipv4,ver=%u,hdrlen=%u,tos=%u,ttl=%u,proto=%u ",
> > -		       xtr->u.ipv4.version,
> > -		       xtr->u.ipv4.ihl,
> > -		       xtr->u.ipv4.tos,
> > -		       xtr->u.ipv4.ttl,
> > -		       xtr->u.ipv4.protocol);
> > -	else if (xtr->type == PROTO_XTR_IPV6)
> > -		printf("ipv6,ver=%u,tc=%u,flow_hi4=0x%x,nexthdr=%u,hoplimit=%u ",
> > -		       xtr->u.ipv6.version,
> > -		       xtr->u.ipv6.tc,
> > -		       xtr->u.ipv6.flowhi4,
> > -		       xtr->u.ipv6.nexthdr,
> > -		       xtr->u.ipv6.hoplimit);
> > -	else if (xtr->type == PROTO_XTR_IPV6_FLOW)
> > -		printf("ipv6_flow,ver=%u,tc=%u,flow=0x%x%04x ",
> > -		       xtr->u.ipv6_flow.version,
> > -		       xtr->u.ipv6_flow.tc,
> > -		       xtr->u.ipv6_flow.flowhi4,
> > -		       xtr->u.ipv6_flow.flowlo16);
> > -	else if (xtr->type == PROTO_XTR_TCP)
> > -		printf("tcp,doff=%u,flags=%s%s%s%s%s%s%s%s ",
> > -		       xtr->u.tcp.doff,
> > -		       xtr->u.tcp.cwr ? "C" : "",
> > -		       xtr->u.tcp.ece ? "E" : "",
> > -		       xtr->u.tcp.urg ? "U" : "",
> > -		       xtr->u.tcp.ack ? "A" : "",
> > -		       xtr->u.tcp.psh ? "P" : "",
> > -		       xtr->u.tcp.rst ? "R" : "",
> > -		       xtr->u.tcp.syn ? "S" : "",
> > -		       xtr->u.tcp.fin ? "F" : "");
> > -}
> > -
> > -#ifdef __cplusplus
> > -}
> > -#endif
> > -
> > -#endif /* _RTE_PMD_ICE_H_ */
> > --
> > 2.17.1
> >

Patch
diff mbox series

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index 933f63480..f460d1cfd 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -82,8 +82,8 @@  Runtime Config Options
 
 - ``Protocol extraction for per queue``
 
-  Configure the RX queues to do protocol extraction into ``rte_mbuf::udata64``
-  for protocol handling acceleration, like checking the TCP SYN packets quickly.
+  Configure the RX queues to do protocol extraction into mbuf for protocol
+  handling acceleration, like checking the TCP SYN packets quickly.
 
   The argument format is::
 
@@ -111,7 +111,9 @@  Runtime Config Options
   This setting means queues 1, 2-3, 8-9 are TCP extraction, queues 10-23 are
   IPv6 extraction, other queues use the default VLAN extraction.
 
-  The extraction will be copied into the lower 32 bit of ``rte_mbuf::udata64``.
+  The extraction metadata will be copied into the registered dynamic mbuf field
+  with name: "proto-xtr-metadata", size: 4B, align: __alignof__(uint32_t), and
+  the related dynamic mbuf flags in ``rte_mbuf::ol_flags`` will be set.
 
   .. table:: Protocol extraction : ``vlan``
 
@@ -125,6 +127,8 @@  Runtime Config Options
 
   VLAN2 - C-VLAN (second for QinQ).
 
+  Dynamic mbuf flag : "proto-xtr-ol-vlan"
+
   .. table:: Protocol extraction : ``ipv4``
 
    +----------------------------+----------------------------+
@@ -137,6 +141,8 @@  Runtime Config Options
 
   IPHDR2 - IPv4 header word 0, "Ver", "Hdr Len" and "Type of Service" fields.
 
+  Dynamic mbuf flag : "proto-xtr-ol-ipv4"
+
   .. table:: Protocol extraction : ``ipv6``
 
    +----------------------------+----------------------------+
@@ -150,6 +156,8 @@  Runtime Config Options
   IPHDR2 - IPv6 header word 0, "Ver", "Traffic class" and high 4 bits of
   "Flow Label" fields.
 
+  Dynamic mbuf flag : "proto-xtr-ol-ipv6"
+
   .. table:: Protocol extraction : ``ipv6_flow``
 
    +----------------------------+----------------------------+
@@ -163,6 +171,8 @@  Runtime Config Options
   IPHDR2 - IPv6 header word 0, "Ver", "Traffic class" and high 4 bits of
   "Flow Label" fields.
 
+  Dynamic mbuf flag : "proto-xtr-ol-ipv6-flow"
+
   .. table:: Protocol extraction : ``tcp``
 
    +----------------------------+----------------------------+
@@ -175,11 +185,7 @@  Runtime Config Options
 
   TCPHDR2 - Reserved
 
-  Use ``get_proto_xtr_flds(struct rte_mbuf *mb)`` to access the protocol
-  extraction, do not use ``rte_mbuf::udata64`` directly.
-
-  The ``dump_proto_xtr_flds(struct rte_mbuf *mb)`` routine shows how to
-  access the protocol extraction result in ``struct rte_mbuf``.
+  Dynamic mbuf flag : "proto-xtr-ol-tcp"
 
 Driver compilation and testing
 ------------------------------
diff --git a/drivers/net/ice/Makefile b/drivers/net/ice/Makefile
index 7c3b6a7ff..a1817e14c 100644
--- a/drivers/net/ice/Makefile
+++ b/drivers/net/ice/Makefile
@@ -84,7 +84,4 @@  ifeq ($(CC_AVX2_SUPPORT), 1)
 endif
 SRCS-$(CONFIG_RTE_LIBRTE_ICE_PMD) += ice_generic_flow.c
 
-# install this header file
-SYMLINK-$(CONFIG_RTE_LIBRTE_ICE_PMD)-include := rte_pmd_ice.h
-
 include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index d74675842..c68acdef0 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -30,6 +30,26 @@  static const char * const ice_valid_args[] = {
 	NULL
 };
 
+static const struct rte_mbuf_dynfield proto_xtr_metadata_param = {
+	.name = "proto-xtr-metadata",
+	.size = sizeof(uint32_t),
+	.align = __alignof__(uint32_t),
+	.flags = 0,
+};
+
+struct proto_xtr_ol_flag {
+	const struct rte_mbuf_dynflag param;
+	uint64_t *flag;
+};
+
+static const struct proto_xtr_ol_flag proto_xtr_ol_flag_params[] = {
+	{ { "proto-xtr-ol-vlan", 0 },      &proto_xtr_ol_vlan },
+	{ { "proto-xtr-ol-ipv4",  0 },     &proto_xtr_ol_ipv4 },
+	{ { "proto-xtr-ol-ipv6", 0 },      &proto_xtr_ol_ipv6 },
+	{ { "proto-xtr-ol-ipv6-flow", 0 }, &proto_xtr_ol_ipv6_flow },
+	{ { "proto-xtr-ol-tcp", 0 },       &proto_xtr_ol_tcp },
+};
+
 #define ICE_DFLT_OUTER_TAG_TYPE ICE_AQ_VSI_OUTER_TAG_VLAN_9100
 
 /* DDP package search path */
@@ -1385,6 +1405,9 @@  ice_init_proto_xtr(struct rte_eth_dev *dev)
 			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);
 	struct ice_hw *hw = ICE_PF_TO_HW(pf);
+	const struct proto_xtr_ol_flag *ol_flag;
+	bool proto_xtr_enable = false;
+	int offset;
 	uint16_t i;
 
 	if (!ice_proto_xtr_support(hw)) {
@@ -1398,10 +1421,52 @@  ice_init_proto_xtr(struct rte_eth_dev *dev)
 		return;
 	}
 
-	for (i = 0; i < pf->lan_nb_qps; i++)
+	for (i = 0; i < pf->lan_nb_qps; i++) {
 		pf->proto_xtr[i] = ad->devargs.proto_xtr[i] != PROTO_XTR_NONE ?
 				   ad->devargs.proto_xtr[i] :
 				   ad->devargs.proto_xtr_dflt;
+
+		if (pf->proto_xtr[i] != PROTO_XTR_NONE && !proto_xtr_enable)
+			proto_xtr_enable = true;
+	}
+
+	if (likely(!proto_xtr_enable))
+		return;
+
+	offset = rte_mbuf_dynfield_register(&proto_xtr_metadata_param);
+	if (unlikely(offset == -1)) {
+		PMD_DRV_LOG(ERR,
+			    "Protocol extraction metadata is disabled in mbuf with error %d",
+			    -rte_errno);
+		return;
+	}
+
+	PMD_DRV_LOG(DEBUG,
+		    "Protocol extraction metadata offset in mbuf is : %d",
+		    offset);
+	proto_xtr_metadata = offset;
+
+	for (i = 0; i < RTE_DIM(proto_xtr_ol_flag_params); i++) {
+		ol_flag = &proto_xtr_ol_flag_params[i];
+
+		offset = rte_mbuf_dynflag_register(&ol_flag->param);
+		if (unlikely(offset == -1)) {
+			PMD_DRV_LOG(ERR,
+				    "Protocol extraction offload '%s' failed to register with error %d",
+				    ol_flag->param.name, -rte_errno);
+			break;
+		}
+
+		PMD_DRV_LOG(DEBUG,
+			    "Protocol extraction offload '%s' offset in mbuf is : %d",
+			    ol_flag->param.name, offset);
+		*ol_flag->flag = 1ULL << offset;
+	}
+
+	if (i != RTE_DIM(proto_xtr_ol_flag_params)) {
+		PMD_DRV_LOG(ERR, "Protocol extraction metadata is disabled");
+		proto_xtr_metadata = -1;
+	}
 }
 
 /*  Initialize SW parameters of PF */
diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
index de67e5934..1ca00bda4 100644
--- a/drivers/net/ice/ice_ethdev.h
+++ b/drivers/net/ice/ice_ethdev.h
@@ -8,6 +8,7 @@ 
 #include <rte_kvargs.h>
 
 #include <rte_ethdev_driver.h>
+#include <rte_mbuf_dyn.h>
 
 #include "base/ice_common.h"
 #include "base/ice_adminq_cmd.h"
@@ -241,6 +242,23 @@  struct ice_vsi {
 	bool offset_loaded;
 };
 
+enum proto_xtr_type {
+	PROTO_XTR_NONE,
+	PROTO_XTR_VLAN,
+	PROTO_XTR_IPV4,
+	PROTO_XTR_IPV6,
+	PROTO_XTR_IPV6_FLOW,
+	PROTO_XTR_TCP,
+};
+
+extern int proto_xtr_metadata;
+
+extern uint64_t proto_xtr_ol_vlan;
+extern uint64_t proto_xtr_ol_ipv4;
+extern uint64_t proto_xtr_ol_ipv6;
+extern uint64_t proto_xtr_ol_ipv6_flow;
+extern uint64_t proto_xtr_ol_tcp;
+
 enum ice_fdir_tunnel_type {
 	ICE_FDIR_TUNNEL_TYPE_NONE = 0,
 	ICE_FDIR_TUNNEL_TYPE_VXLAN,
diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
index 8d4820d3c..6493aa700 100644
--- a/drivers/net/ice/ice_rxtx.c
+++ b/drivers/net/ice/ice_rxtx.c
@@ -13,18 +13,31 @@ 
 		PKT_TX_TCP_SEG |		 \
 		PKT_TX_OUTER_IP_CKSUM)
 
-static inline uint8_t
-ice_rxdid_to_proto_xtr_type(uint8_t rxdid)
-{
-	static uint8_t xtr_map[] = {
-		[ICE_RXDID_COMMS_AUX_VLAN]      = PROTO_XTR_VLAN,
-		[ICE_RXDID_COMMS_AUX_IPV4]      = PROTO_XTR_IPV4,
-		[ICE_RXDID_COMMS_AUX_IPV6]      = PROTO_XTR_IPV6,
-		[ICE_RXDID_COMMS_AUX_IPV6_FLOW] = PROTO_XTR_IPV6_FLOW,
-		[ICE_RXDID_COMMS_AUX_TCP]       = PROTO_XTR_TCP,
+/* Offset of mbuf dynamic field for protocol extraction data */
+int proto_xtr_metadata = -1;
+
+/* Mask of mbuf dynamic flags for protocol extraction type */
+uint64_t proto_xtr_ol_vlan;
+uint64_t proto_xtr_ol_ipv4;
+uint64_t proto_xtr_ol_ipv6;
+uint64_t proto_xtr_ol_ipv6_flow;
+uint64_t proto_xtr_ol_tcp;
+
+static inline uint64_t
+ice_rxdid_to_proto_xtr_ol_flag(uint8_t rxdid)
+{
+	static uint64_t *ol_flag_map[] = {
+		[ICE_RXDID_COMMS_AUX_VLAN]      = &proto_xtr_ol_vlan,
+		[ICE_RXDID_COMMS_AUX_IPV4]      = &proto_xtr_ol_ipv4,
+		[ICE_RXDID_COMMS_AUX_IPV6]      = &proto_xtr_ol_ipv6,
+		[ICE_RXDID_COMMS_AUX_IPV6_FLOW] = &proto_xtr_ol_ipv6_flow,
+		[ICE_RXDID_COMMS_AUX_TCP]       = &proto_xtr_ol_tcp,
 	};
+	uint64_t *ol_flag;
 
-	return rxdid < RTE_DIM(xtr_map) ? xtr_map[rxdid] : PROTO_XTR_NONE;
+	ol_flag = rxdid < RTE_DIM(ol_flag_map) ? ol_flag_map[rxdid] : NULL;
+
+	return ol_flag != NULL ? *ol_flag : 0;
 }
 
 static inline uint8_t
@@ -1325,10 +1338,38 @@  ice_rxd_to_vlan_tci(struct rte_mbuf *mb, volatile union ice_rx_flex_desc *rxdp)
 		   mb->vlan_tci, mb->vlan_tci_outer);
 }
 
+#ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
 #define ICE_RX_PROTO_XTR_VALID \
 	((1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD4_VALID_S) | \
 	 (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD5_VALID_S))
 
+static void
+ice_rxd_to_proto_xtr(struct rte_mbuf *mb,
+		     volatile struct ice_32b_rx_flex_desc_comms *desc)
+{
+	uint16_t stat_err = rte_le_to_cpu_16(desc->status_error1);
+	uint32_t metadata;
+	uint64_t ol_flag;
+
+	if (unlikely(!(stat_err & ICE_RX_PROTO_XTR_VALID)))
+		return;
+
+	ol_flag = ice_rxdid_to_proto_xtr_ol_flag(desc->rxdid);
+	if (unlikely(!ol_flag))
+		return;
+
+	mb->ol_flags |= ol_flag;
+
+	metadata = stat_err & (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD4_VALID_S) ?
+				rte_le_to_cpu_16(desc->flex_ts.flex.aux0) : 0;
+
+	if (likely(stat_err & (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD5_VALID_S)))
+		metadata |= rte_le_to_cpu_16(desc->flex_ts.flex.aux1) << 16;
+
+	*RTE_MBUF_DYNFIELD(mb, proto_xtr_metadata, uint32_t *) = metadata;
+}
+#endif
+
 static inline void
 ice_rxd_to_pkt_fields(struct rte_mbuf *mb,
 		      volatile union ice_rx_flex_desc *rxdp)
@@ -1344,28 +1385,13 @@  ice_rxd_to_pkt_fields(struct rte_mbuf *mb,
 	}
 
 #ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
-	init_proto_xtr_flds(mb);
-
-	stat_err = rte_le_to_cpu_16(desc->status_error1);
-	if (stat_err & ICE_RX_PROTO_XTR_VALID) {
-		struct proto_xtr_flds *xtr = get_proto_xtr_flds(mb);
-
-		if (stat_err & (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD4_VALID_S))
-			xtr->u.raw.data0 =
-				rte_le_to_cpu_16(desc->flex_ts.flex.aux0);
-
-		if (stat_err & (1 << ICE_RX_FLEX_DESC_STATUS1_XTRMD5_VALID_S))
-			xtr->u.raw.data1 =
-				rte_le_to_cpu_16(desc->flex_ts.flex.aux1);
-
-		xtr->type = ice_rxdid_to_proto_xtr_type(desc->rxdid);
-		xtr->magic = PROTO_XTR_MAGIC_ID;
-	}
-
 	if (desc->flow_id != 0xFFFFFFFF) {
 		mb->ol_flags |= PKT_RX_FDIR | PKT_RX_FDIR_ID;
 		mb->hash.fdir.hi = rte_le_to_cpu_32(desc->flow_id);
 	}
+
+	if (unlikely(proto_xtr_metadata != -1))
+		ice_rxd_to_proto_xtr(mb, desc);
 #endif
 }
 
diff --git a/drivers/net/ice/ice_rxtx.h b/drivers/net/ice/ice_rxtx.h
index 5de618976..9e3d2cd07 100644
--- a/drivers/net/ice/ice_rxtx.h
+++ b/drivers/net/ice/ice_rxtx.h
@@ -5,7 +5,6 @@ 
 #ifndef _ICE_RXTX_H_
 #define _ICE_RXTX_H_
 
-#include "rte_pmd_ice.h"
 #include "ice_ethdev.h"
 
 #define ICE_ALIGN_RING_DESC  32
diff --git a/drivers/net/ice/meson.build b/drivers/net/ice/meson.build
index f9e897bbc..45e760063 100644
--- a/drivers/net/ice/meson.build
+++ b/drivers/net/ice/meson.build
@@ -36,5 +36,3 @@  if arch_subdir == 'x86'
 		objs += ice_avx2_lib.extract_objects('ice_rxtx_vec_avx2.c')
 	endif
 endif
-
-install_headers('rte_pmd_ice.h')
diff --git a/drivers/net/ice/rte_pmd_ice.h b/drivers/net/ice/rte_pmd_ice.h
deleted file mode 100644
index 719487e1e..000000000
--- a/drivers/net/ice/rte_pmd_ice.h
+++ /dev/null
@@ -1,152 +0,0 @@ 
-/* SPDX-License-Identifier: BSD-3-Clause
- * Copyright(c) 2019 Intel Corporation
- */
-
-#ifndef _RTE_PMD_ICE_H_
-#define _RTE_PMD_ICE_H_
-
-#include <stdio.h>
-#include <rte_mbuf.h>
-#include <rte_ethdev.h>
-
-#ifdef __cplusplus
-extern "C" {
-#endif
-
-enum proto_xtr_type {
-	PROTO_XTR_NONE,
-	PROTO_XTR_VLAN,
-	PROTO_XTR_IPV4,
-	PROTO_XTR_IPV6,
-	PROTO_XTR_IPV6_FLOW,
-	PROTO_XTR_TCP,
-};
-
-struct proto_xtr_flds {
-	union {
-		struct {
-			uint16_t data0;
-			uint16_t data1;
-		} raw;
-		struct {
-			uint16_t stag_vid:12,
-				 stag_dei:1,
-				 stag_pcp:3;
-			uint16_t ctag_vid:12,
-				 ctag_dei:1,
-				 ctag_pcp:3;
-		} vlan;
-		struct {
-			uint16_t protocol:8,
-				 ttl:8;
-			uint16_t tos:8,
-				 ihl:4,
-				 version:4;
-		} ipv4;
-		struct {
-			uint16_t hoplimit:8,
-				 nexthdr:8;
-			uint16_t flowhi4:4,
-				 tc:8,
-				 version:4;
-		} ipv6;
-		struct {
-			uint16_t flowlo16;
-			uint16_t flowhi4:4,
-				 tc:8,
-				 version:4;
-		} ipv6_flow;
-		struct {
-			uint16_t fin:1,
-				 syn:1,
-				 rst:1,
-				 psh:1,
-				 ack:1,
-				 urg:1,
-				 ece:1,
-				 cwr:1,
-				 res1:4,
-				 doff:4;
-			uint16_t rsvd;
-		} tcp;
-	} u;
-
-	uint16_t rsvd;
-
-	uint8_t type;
-
-#define PROTO_XTR_MAGIC_ID	0xCE
-	uint8_t magic;
-};
-
-static inline void
-init_proto_xtr_flds(struct rte_mbuf *mb)
-{
-	mb->udata64 = 0;
-}
-
-static inline struct proto_xtr_flds *
-get_proto_xtr_flds(struct rte_mbuf *mb)
-{
-	RTE_BUILD_BUG_ON(sizeof(struct proto_xtr_flds) > sizeof(mb->udata64));
-
-	return (struct proto_xtr_flds *)&mb->udata64;
-}
-
-static inline void
-dump_proto_xtr_flds(struct rte_mbuf *mb)
-{
-	struct proto_xtr_flds *xtr = get_proto_xtr_flds(mb);
-
-	if (xtr->magic != PROTO_XTR_MAGIC_ID || xtr->type == PROTO_XTR_NONE)
-		return;
-
-	printf(" - Protocol Extraction:[0x%04x:0x%04x],",
-	       xtr->u.raw.data0, xtr->u.raw.data1);
-
-	if (xtr->type == PROTO_XTR_VLAN)
-		printf("vlan,stag=%u:%u:%u,ctag=%u:%u:%u ",
-		       xtr->u.vlan.stag_pcp,
-		       xtr->u.vlan.stag_dei,
-		       xtr->u.vlan.stag_vid,
-		       xtr->u.vlan.ctag_pcp,
-		       xtr->u.vlan.ctag_dei,
-		       xtr->u.vlan.ctag_vid);
-	else if (xtr->type == PROTO_XTR_IPV4)
-		printf("ipv4,ver=%u,hdrlen=%u,tos=%u,ttl=%u,proto=%u ",
-		       xtr->u.ipv4.version,
-		       xtr->u.ipv4.ihl,
-		       xtr->u.ipv4.tos,
-		       xtr->u.ipv4.ttl,
-		       xtr->u.ipv4.protocol);
-	else if (xtr->type == PROTO_XTR_IPV6)
-		printf("ipv6,ver=%u,tc=%u,flow_hi4=0x%x,nexthdr=%u,hoplimit=%u ",
-		       xtr->u.ipv6.version,
-		       xtr->u.ipv6.tc,
-		       xtr->u.ipv6.flowhi4,
-		       xtr->u.ipv6.nexthdr,
-		       xtr->u.ipv6.hoplimit);
-	else if (xtr->type == PROTO_XTR_IPV6_FLOW)
-		printf("ipv6_flow,ver=%u,tc=%u,flow=0x%x%04x ",
-		       xtr->u.ipv6_flow.version,
-		       xtr->u.ipv6_flow.tc,
-		       xtr->u.ipv6_flow.flowhi4,
-		       xtr->u.ipv6_flow.flowlo16);
-	else if (xtr->type == PROTO_XTR_TCP)
-		printf("tcp,doff=%u,flags=%s%s%s%s%s%s%s%s ",
-		       xtr->u.tcp.doff,
-		       xtr->u.tcp.cwr ? "C" : "",
-		       xtr->u.tcp.ece ? "E" : "",
-		       xtr->u.tcp.urg ? "U" : "",
-		       xtr->u.tcp.ack ? "A" : "",
-		       xtr->u.tcp.psh ? "P" : "",
-		       xtr->u.tcp.rst ? "R" : "",
-		       xtr->u.tcp.syn ? "S" : "",
-		       xtr->u.tcp.fin ? "F" : "");
-}
-
-#ifdef __cplusplus
-}
-#endif
-
-#endif /* _RTE_PMD_ICE_H_ */