[v1] net/ice: refactor dynamic mbuf in data extraction

  Current dynamic mbuf design is that the driver will register the needed
field and flags at the device probing time, this will make iavf PMD use
different names to register the dynamic mbuf field and flags, but both
of them use the exactly same protocol extraction metadata.

This will run out of the limited dynamic mbuf resource, meanwhile, the
application has to handle the dynamic mbuf separately.

For making things simple and consistent, refactor dynamic mbuf in data
extraction handling: the PMD just lookups the same name at the queue
setup time after the application registers it.

In other words, make the dynamic mbuf string name as API, not the data
object which is defined in each PMD.

Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
---
 drivers/net/ice/ice_ethdev.c  | 104 ++--------------
 drivers/net/ice/ice_ethdev.h  |   1 +
 drivers/net/ice/ice_rxtx.c    |  49 ++++----
 drivers/net/ice/ice_rxtx.h    |   1 +
 drivers/net/ice/rte_pmd_ice.h | 219 +++++-----------------------------
 drivers/net/ice/version.map   |  13 --
 6 files changed, 68 insertions(+), 319 deletions(-)
  

Hi Haiyue,

On Sun, Oct 25, 2020 at 03:13:52PM +0800, Haiyue Wang wrote:
> Current dynamic mbuf design is that the driver will register the needed
> field and flags at the device probing time, this will make iavf PMD use
> different names to register the dynamic mbuf field and flags, but both
> of them use the exactly same protocol extraction metadata.
> 
> This will run out of the limited dynamic mbuf resource, meanwhile, the
> application has to handle the dynamic mbuf separately.
> 
> For making things simple and consistent, refactor dynamic mbuf in data
> extraction handling: the PMD just lookups the same name at the queue
> setup time after the application registers it.
> 
> In other words, make the dynamic mbuf string name as API, not the data
> object which is defined in each PMD.

In case the dynamic mbuf field is shared by several PMDs, it seems to
be indeed a better solution.

Currently, the "union rte_pmd_proto_xtr_metadata" is still defined in
rte_pmd_ice.h. Will it be the same for iavf, and will it be factorized
somewhere? However I don't know where could be a good place.

There is already lib/librte_mbuf/rte_mbuf_dyn.h which is the place to
centralize the name and description of dynamic fields/flags used in
libraries. But I think neither structure definitions nor PMD-specific
flags should go there too. I'd prefer to have them in
drivers/net/<common-something>, but I'm not sure it is possible.

Also, it is difficult from the patch to see the impact it has on an
application that was using these metadata. Should we have an example
of use?

Thanks,
Olivier


> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> ---
>  drivers/net/ice/ice_ethdev.c  | 104 ++--------------
>  drivers/net/ice/ice_ethdev.h  |   1 +
>  drivers/net/ice/ice_rxtx.c    |  49 ++++----
>  drivers/net/ice/ice_rxtx.h    |   1 +
>  drivers/net/ice/rte_pmd_ice.h | 219 +++++-----------------------------
>  drivers/net/ice/version.map   |  13 --
>  6 files changed, 68 insertions(+), 319 deletions(-)
> 
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 51b99c6506..ec27089cfa 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -32,42 +32,6 @@ static const char * const ice_valid_args[] = {
>  	NULL
>  };
>  
> -static const struct rte_mbuf_dynfield ice_proto_xtr_metadata_param = {
> -	.name = "ice_dynfield_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 *ol_flag;
> -	bool required;
> -};
> -
> -static bool ice_proto_xtr_hw_support[PROTO_XTR_MAX];
> -
> -static struct proto_xtr_ol_flag ice_proto_xtr_ol_flag_params[] = {
> -	[PROTO_XTR_VLAN] = {
> -		.param = { .name = "ice_dynflag_proto_xtr_vlan" },
> -		.ol_flag = &rte_net_ice_dynflag_proto_xtr_vlan_mask },
> -	[PROTO_XTR_IPV4] = {
> -		.param = { .name = "ice_dynflag_proto_xtr_ipv4" },
> -		.ol_flag = &rte_net_ice_dynflag_proto_xtr_ipv4_mask },
> -	[PROTO_XTR_IPV6] = {
> -		.param = { .name = "ice_dynflag_proto_xtr_ipv6" },
> -		.ol_flag = &rte_net_ice_dynflag_proto_xtr_ipv6_mask },
> -	[PROTO_XTR_IPV6_FLOW] = {
> -		.param = { .name = "ice_dynflag_proto_xtr_ipv6_flow" },
> -		.ol_flag = &rte_net_ice_dynflag_proto_xtr_ipv6_flow_mask },
> -	[PROTO_XTR_TCP] = {
> -		.param = { .name = "ice_dynflag_proto_xtr_tcp" },
> -		.ol_flag = &rte_net_ice_dynflag_proto_xtr_tcp_mask },
> -	[PROTO_XTR_IP_OFFSET] = {
> -		.param = { .name = "ice_dynflag_proto_xtr_ip_offset" },
> -		.ol_flag = &rte_net_ice_dynflag_proto_xtr_ip_offset_mask },
> -};
> -
>  #define ICE_DFLT_OUTER_TAG_TYPE ICE_AQ_VSI_OUTER_TAG_VLAN_9100
>  
>  #define ICE_OS_DEFAULT_PKG_NAME		"ICE OS Default Package"
> @@ -542,7 +506,7 @@ handle_proto_xtr_arg(__rte_unused const char *key, const char *value,
>  }
>  
>  static void
> -ice_check_proto_xtr_support(struct ice_hw *hw)
> +ice_check_proto_xtr_support(struct ice_pf *pf, struct ice_hw *hw)
>  {
>  #define FLX_REG(val, fld, idx) \
>  	(((val) & GLFLXP_RXDID_FLX_WRD_##idx##_##fld##_M) >> \
> @@ -587,7 +551,7 @@ ice_check_proto_xtr_support(struct ice_hw *hw)
>  
>  			if (FLX_REG(v, PROT_MDID, 4) == xtr_sets[i].protid_0 &&
>  			    FLX_REG(v, RXDID_OPCODE, 4) == xtr_sets[i].opcode)
> -				ice_proto_xtr_hw_support[i] = true;
> +				pf->hw_proto_xtr_ena[i] = 1;
>  		}
>  
>  		if (xtr_sets[i].protid_1 != ICE_PROT_ID_INVAL) {
> @@ -595,7 +559,7 @@ ice_check_proto_xtr_support(struct ice_hw *hw)
>  
>  			if (FLX_REG(v, PROT_MDID, 5) == xtr_sets[i].protid_1 &&
>  			    FLX_REG(v, RXDID_OPCODE, 5) == xtr_sets[i].opcode)
> -				ice_proto_xtr_hw_support[i] = true;
> +				pf->hw_proto_xtr_ena[i] = 1;
>  		}
>  	}
>  }
> @@ -1429,9 +1393,6 @@ 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;
>  
>  	pf->proto_xtr = rte_zmalloc(NULL, pf->lan_nb_qps, 0);
> @@ -1440,65 +1401,20 @@ ice_init_proto_xtr(struct rte_eth_dev *dev)
>  		return;
>  	}
>  
> +	ice_check_proto_xtr_support(pf, hw);
> +
>  	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) {
> -			uint8_t type = pf->proto_xtr[i];
> -
> -			ice_proto_xtr_ol_flag_params[type].required = true;
> -			proto_xtr_enable = true;
> -		}
> -	}
> -
> -	if (likely(!proto_xtr_enable))
> -		return;
> -
> -	ice_check_proto_xtr_support(hw);
> -
> -	offset = rte_mbuf_dynfield_register(&ice_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);
> -	rte_net_ice_dynfield_proto_xtr_metadata_offs = offset;
> -
> -	for (i = 0; i < RTE_DIM(ice_proto_xtr_ol_flag_params); i++) {
> -		ol_flag = &ice_proto_xtr_ol_flag_params[i];
> -
> -		if (!ol_flag->required)
> -			continue;
> -
> -		if (!ice_proto_xtr_hw_support[i]) {
> +		if (pf->proto_xtr[i] != PROTO_XTR_NONE &&
> +		    !pf->hw_proto_xtr_ena[pf->proto_xtr[i]]) {
>  			PMD_DRV_LOG(ERR,
> -				    "Protocol extraction type %u is not supported in hardware",
> -				    i);
> -			rte_net_ice_dynfield_proto_xtr_metadata_offs = -1;
> -			break;
> +				    "The Rx queue %u doesn't support protocol extraction type %u\n",
> +				    i, pf->proto_xtr[i]);
> +			pf->proto_xtr[i] = PROTO_XTR_NONE;
>  		}
> -
> -		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);
> -
> -			rte_net_ice_dynfield_proto_xtr_metadata_offs = -1;
> -			break;
> -		}
> -
> -		PMD_DRV_LOG(DEBUG,
> -			    "Protocol extraction offload '%s' offset in mbuf is : %d",
> -			    ol_flag->param.name, offset);
> -		*ol_flag->ol_flag = 1ULL << offset;
>  	}
>  }
>  
> diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
> index 05218af05e..a675eac209 100644
> --- a/drivers/net/ice/ice_ethdev.h
> +++ b/drivers/net/ice/ice_ethdev.h
> @@ -457,6 +457,7 @@ struct ice_pf {
>  	uint64_t old_rx_bytes;
>  	uint64_t old_tx_bytes;
>  	uint64_t supported_rxdid; /* bitmap for supported RXDID */
> +	uint8_t hw_proto_xtr_ena[PROTO_XTR_MAX];
>  };
>  
>  #define ICE_MAX_QUEUE_NUM  2048
> diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> index f6291894cd..27f04a432f 100644
> --- a/drivers/net/ice/ice_rxtx.c
> +++ b/drivers/net/ice/ice_rxtx.c
> @@ -15,16 +15,8 @@
>  		PKT_TX_TCP_SEG |		 \
>  		PKT_TX_OUTER_IP_CKSUM)
>  
> -/* Offset of mbuf dynamic field for protocol extraction data */
> -int rte_net_ice_dynfield_proto_xtr_metadata_offs = -1;
> -
> -/* Mask of mbuf dynamic flags for protocol extraction type */
> -uint64_t rte_net_ice_dynflag_proto_xtr_vlan_mask;
> -uint64_t rte_net_ice_dynflag_proto_xtr_ipv4_mask;
> -uint64_t rte_net_ice_dynflag_proto_xtr_ipv6_mask;
> -uint64_t rte_net_ice_dynflag_proto_xtr_ipv6_flow_mask;
> -uint64_t rte_net_ice_dynflag_proto_xtr_tcp_mask;
> -uint64_t rte_net_ice_dynflag_proto_xtr_ip_offset_mask;
> +#define ICE_DYNF_PROTO_XTR_METADATA(m) \
> +	RTE_MBUF_DYNFIELD((m), rxq->xtr_metadata_off, uint32_t *)
>  
>  static inline uint8_t
>  ice_proto_xtr_type_to_rxdid(uint8_t xtr_type)
> @@ -104,7 +96,7 @@ ice_rxd_to_pkt_fields_by_comms_aux_v1(struct ice_rx_queue *rxq,
>  		if (metadata) {
>  			mb->ol_flags |= rxq->xtr_ol_flag;
>  
> -			*RTE_NET_ICE_DYNF_PROTO_XTR_METADATA(mb) = metadata;
> +			*ICE_DYNF_PROTO_XTR_METADATA(mb) = metadata;
>  		}
>  	}
>  #endif
> @@ -142,7 +134,7 @@ ice_rxd_to_pkt_fields_by_comms_aux_v2(struct ice_rx_queue *rxq,
>  		if (metadata) {
>  			mb->ol_flags |= rxq->xtr_ol_flag;
>  
> -			*RTE_NET_ICE_DYNF_PROTO_XTR_METADATA(mb) = metadata;
> +			*ICE_DYNF_PROTO_XTR_METADATA(mb) = metadata;
>  		}
>  	}
>  #endif
> @@ -151,34 +143,38 @@ ice_rxd_to_pkt_fields_by_comms_aux_v2(struct ice_rx_queue *rxq,
>  static void
>  ice_select_rxd_to_pkt_fields_handler(struct ice_rx_queue *rxq, uint32_t rxdid)
>  {
> +	struct rte_mbuf_dynfield metadata_param;
> +	const char *flag_name = NULL;
> +	int flag_off, metadata_off;
> +
>  	switch (rxdid) {
>  	case ICE_RXDID_COMMS_AUX_VLAN:
> -		rxq->xtr_ol_flag = rte_net_ice_dynflag_proto_xtr_vlan_mask;
> +		flag_name = RTE_PMD_DYNFLAG_PROTO_XTR_VLAN_NAME;
>  		rxq->rxd_to_pkt_fields = ice_rxd_to_pkt_fields_by_comms_aux_v1;
>  		break;
>  
>  	case ICE_RXDID_COMMS_AUX_IPV4:
> -		rxq->xtr_ol_flag = rte_net_ice_dynflag_proto_xtr_ipv4_mask;
> +		flag_name = RTE_PMD_DYNFLAG_PROTO_XTR_IPV4_NAME;
>  		rxq->rxd_to_pkt_fields = ice_rxd_to_pkt_fields_by_comms_aux_v1;
>  		break;
>  
>  	case ICE_RXDID_COMMS_AUX_IPV6:
> -		rxq->xtr_ol_flag = rte_net_ice_dynflag_proto_xtr_ipv6_mask;
> +		flag_name = RTE_PMD_DYNFLAG_PROTO_XTR_IPV6_NAME;
>  		rxq->rxd_to_pkt_fields = ice_rxd_to_pkt_fields_by_comms_aux_v1;
>  		break;
>  
>  	case ICE_RXDID_COMMS_AUX_IPV6_FLOW:
> -		rxq->xtr_ol_flag = rte_net_ice_dynflag_proto_xtr_ipv6_flow_mask;
> +		flag_name = RTE_PMD_DYNFLAG_PROTO_XTR_IPV6_FLOW_NAME;
>  		rxq->rxd_to_pkt_fields = ice_rxd_to_pkt_fields_by_comms_aux_v1;
>  		break;
>  
>  	case ICE_RXDID_COMMS_AUX_TCP:
> -		rxq->xtr_ol_flag = rte_net_ice_dynflag_proto_xtr_tcp_mask;
> +		flag_name = RTE_PMD_DYNFLAG_PROTO_XTR_TCP_NAME;
>  		rxq->rxd_to_pkt_fields = ice_rxd_to_pkt_fields_by_comms_aux_v1;
>  		break;
>  
>  	case ICE_RXDID_COMMS_AUX_IP_OFFSET:
> -		rxq->xtr_ol_flag = rte_net_ice_dynflag_proto_xtr_ip_offset_mask;
> +		flag_name = RTE_PMD_DYNFLAG_PROTO_XTR_IP_OFFSET_NAME;
>  		rxq->rxd_to_pkt_fields = ice_rxd_to_pkt_fields_by_comms_aux_v2;
>  		break;
>  
> @@ -192,8 +188,21 @@ ice_select_rxd_to_pkt_fields_handler(struct ice_rx_queue *rxq, uint32_t rxdid)
>  		break;
>  	}
>  
> -	if (!rte_net_ice_dynf_proto_xtr_metadata_avail())
> -		rxq->xtr_ol_flag = 0;
> +	if (!flag_name)
> +		return;
> +
> +	flag_off = rte_mbuf_dynflag_lookup(flag_name, NULL);
> +	if (flag_off == -1)
> +		return;
> +
> +	metadata_off = rte_mbuf_dynfield_lookup
> +				(RTE_PMD_DYNFIELD_PROTO_XTR_METADATA_NAME,
> +				 &metadata_param);
> +	if (metadata_off == -1 || metadata_param.size != sizeof(uint32_t))
> +		return;
> +
> +	rxq->xtr_metadata_off = metadata_off;
> +	rxq->xtr_ol_flag = 1ULL << flag_off;
>  }
>  
>  static enum ice_status
> diff --git a/drivers/net/ice/ice_rxtx.h b/drivers/net/ice/ice_rxtx.h
> index 23409d479a..53d0a609f3 100644
> --- a/drivers/net/ice/ice_rxtx.h
> +++ b/drivers/net/ice/ice_rxtx.h
> @@ -85,6 +85,7 @@ struct ice_rx_queue {
>  	bool q_set; /* indicate if rx queue has been configured */
>  	bool rx_deferred_start; /* don't start this queue in dev start */
>  	uint8_t proto_xtr; /* Protocol extraction from flexible descriptor */
> +	int xtr_metadata_off; /* Protocol extraction offload metadata offset */
>  	uint64_t xtr_ol_flag; /* Protocol extraction offload flag */
>  	ice_rxd_to_pkt_fields_t rxd_to_pkt_fields; /* handle FlexiMD by RXDID */
>  	ice_rx_release_mbufs_t rx_rel_mbufs;
> diff --git a/drivers/net/ice/rte_pmd_ice.h b/drivers/net/ice/rte_pmd_ice.h
> index 9a436a140b..df27b59d35 100644
> --- a/drivers/net/ice/rte_pmd_ice.h
> +++ b/drivers/net/ice/rte_pmd_ice.h
> @@ -22,220 +22,55 @@
>  extern "C" {
>  #endif
>  
> -/**
> - * The supported network protocol extraction metadata format.
> - */
> -union rte_net_ice_proto_xtr_metadata {
> -	uint32_t metadata;
> -
> -	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;
> -
> -	uint32_t ip_ofs;
> -};
> -
> -/* Offset of mbuf dynamic field for protocol extraction data */
> -extern int rte_net_ice_dynfield_proto_xtr_metadata_offs;
> -
> -/* Mask of mbuf dynamic flags for protocol extraction type */
> -extern uint64_t rte_net_ice_dynflag_proto_xtr_vlan_mask;
> -extern uint64_t rte_net_ice_dynflag_proto_xtr_ipv4_mask;
> -extern uint64_t rte_net_ice_dynflag_proto_xtr_ipv6_mask;
> -extern uint64_t rte_net_ice_dynflag_proto_xtr_ipv6_flow_mask;
> -extern uint64_t rte_net_ice_dynflag_proto_xtr_tcp_mask;
> -extern uint64_t rte_net_ice_dynflag_proto_xtr_ip_offset_mask;
> +#ifndef __INTEL_FXP_RX_DESC_METADATA__
> +#define __INTEL_FXP_RX_DESC_METADATA__
>  
>  /**
> - * The mbuf dynamic field pointer for protocol extraction metadata.
> + * The mbuf dynamic field for metadata extraction from NIC:
> + *  a). Extract 16b (2 Bytes) from the defined offset location of the specified
> + *      protocol in the packet.
> + *  b). Report the offset to the selected protocol.
>   */
> -#define RTE_NET_ICE_DYNF_PROTO_XTR_METADATA(m) \
> -	RTE_MBUF_DYNFIELD((m), \
> -			  rte_net_ice_dynfield_proto_xtr_metadata_offs, \
> -			  uint32_t *)
> +#define RTE_PMD_DYNFIELD_PROTO_XTR_METADATA_NAME \
> +	"rte_pmd_dynfield_proto_xtr_metadata"
>  
>  /**
> - * The mbuf dynamic flag for VLAN protocol extraction metadata, it is valid
> - * when dev_args 'proto_xtr' has 'vlan' specified.
> + * The mbuf dynamic flag for VLAN protocol extraction metadata type.
>   */
> -#define RTE_PKT_RX_DYNF_PROTO_XTR_VLAN \
> -	(rte_net_ice_dynflag_proto_xtr_vlan_mask)
> +#define RTE_PMD_DYNFLAG_PROTO_XTR_VLAN_NAME \
> +	"rte_pmd_dynflag_proto_xtr_vlan"
>  
>  /**
> - * The mbuf dynamic flag for IPv4 protocol extraction metadata, it is valid
> - * when dev_args 'proto_xtr' has 'ipv4' specified.
> + * The mbuf dynamic flag for IPv4 protocol extraction metadata type.
>   */
> -#define RTE_PKT_RX_DYNF_PROTO_XTR_IPV4 \
> -	(rte_net_ice_dynflag_proto_xtr_ipv4_mask)
> +#define RTE_PMD_DYNFLAG_PROTO_XTR_IPV4_NAME \
> +	"rte_pmd_dynflag_proto_xtr_ipv4"
>  
>  /**
> - * The mbuf dynamic flag for IPv6 protocol extraction metadata, it is valid
> - * when dev_args 'proto_xtr' has 'ipv6' specified.
> + * The mbuf dynamic flag for IPv6 protocol extraction metadata type.
>   */
> -#define RTE_PKT_RX_DYNF_PROTO_XTR_IPV6 \
> -	(rte_net_ice_dynflag_proto_xtr_ipv6_mask)
> +#define RTE_PMD_DYNFLAG_PROTO_XTR_IPV6_NAME \
> +	"rte_pmd_dynflag_proto_xtr_ipv6"
>  
>  /**
> - * The mbuf dynamic flag for IPv6 with flow protocol extraction metadata, it is
> - * valid when dev_args 'proto_xtr' has 'ipv6_flow' specified.
> + * The mbuf dynamic flag for IPv6 with flow protocol extraction metadata type.
>   */
> -#define RTE_PKT_RX_DYNF_PROTO_XTR_IPV6_FLOW \
> -	(rte_net_ice_dynflag_proto_xtr_ipv6_flow_mask)
> +#define RTE_PMD_DYNFLAG_PROTO_XTR_IPV6_FLOW_NAME \
> +	"rte_pmd_dynflag_proto_xtr_ipv6_flow"
>  
>  /**
> - * The mbuf dynamic flag for TCP protocol extraction metadata, it is valid
> - * when dev_args 'proto_xtr' has 'tcp' specified.
> + * The mbuf dynamic flag for TCP protocol extraction metadata type.
>   */
> -#define RTE_PKT_RX_DYNF_PROTO_XTR_TCP \
> -	(rte_net_ice_dynflag_proto_xtr_tcp_mask)
> +#define RTE_PMD_DYNFLAG_PROTO_XTR_TCP_NAME \
> +	"rte_pmd_dynflag_proto_xtr_tcp"
>  
>  /**
> - * The mbuf dynamic flag for IP_OFFSET extraction metadata, it is valid
> - * when dev_args 'proto_xtr' has 'ip_offset' specified.
> +  * The mbuf dynamic flag for IPv4 or IPv6 header offset report metadata type.
>   */
> -#define RTE_PKT_RX_DYNF_PROTO_XTR_IP_OFFSET \
> -	(rte_net_ice_dynflag_proto_xtr_ip_offset_mask)
> +#define RTE_PMD_DYNFLAG_PROTO_XTR_IP_OFFSET_NAME \
> +	"rte_pmd_dynflag_proto_xtr_ip_offset"
>  
> -/**
> - * Check if mbuf dynamic field for protocol extraction metadata is registered.
> - *
> - * @return
> - *   True if registered, false otherwise.
> - */
> -__rte_experimental
> -static __rte_always_inline int
> -rte_net_ice_dynf_proto_xtr_metadata_avail(void)
> -{
> -	return rte_net_ice_dynfield_proto_xtr_metadata_offs != -1;
> -}
> -
> -/**
> - * Get the mbuf dynamic field for protocol extraction metadata.
> - *
> - * @param m
> - *    The pointer to the mbuf.
> - * @return
> - *   The saved protocol extraction metadata.
> - */
> -__rte_experimental
> -static __rte_always_inline uint32_t
> -rte_net_ice_dynf_proto_xtr_metadata_get(struct rte_mbuf *m)
> -{
> -	return *RTE_NET_ICE_DYNF_PROTO_XTR_METADATA(m);
> -}
> -
> -/**
> - * Dump the mbuf dynamic field for protocol extraction metadata.
> - *
> - * @param m
> - *    The pointer to the mbuf.
> - */
> -__rte_experimental
> -static inline void
> -rte_net_ice_dump_proto_xtr_metadata(struct rte_mbuf *m)
> -{
> -	union rte_net_ice_proto_xtr_metadata data;
> -
> -	if (!rte_net_ice_dynf_proto_xtr_metadata_avail())
> -		return;
> -
> -	data.metadata = rte_net_ice_dynf_proto_xtr_metadata_get(m);
> -
> -	if (m->ol_flags & RTE_PKT_RX_DYNF_PROTO_XTR_VLAN)
> -		printf(" - Protocol Extraction:[0x%04x:0x%04x],vlan,stag=%u:%u:%u,ctag=%u:%u:%u",
> -		       data.raw.data0, data.raw.data1,
> -		       data.vlan.stag_pcp,
> -		       data.vlan.stag_dei,
> -		       data.vlan.stag_vid,
> -		       data.vlan.ctag_pcp,
> -		       data.vlan.ctag_dei,
> -		       data.vlan.ctag_vid);
> -	else if (m->ol_flags & RTE_PKT_RX_DYNF_PROTO_XTR_IPV4)
> -		printf(" - Protocol Extraction:[0x%04x:0x%04x],ipv4,ver=%u,hdrlen=%u,tos=%u,ttl=%u,proto=%u",
> -		       data.raw.data0, data.raw.data1,
> -		       data.ipv4.version,
> -		       data.ipv4.ihl,
> -		       data.ipv4.tos,
> -		       data.ipv4.ttl,
> -		       data.ipv4.protocol);
> -	else if (m->ol_flags & RTE_PKT_RX_DYNF_PROTO_XTR_IPV6)
> -		printf(" - Protocol Extraction:[0x%04x:0x%04x],ipv6,ver=%u,tc=%u,flow_hi4=0x%x,nexthdr=%u,hoplimit=%u",
> -		       data.raw.data0, data.raw.data1,
> -		       data.ipv6.version,
> -		       data.ipv6.tc,
> -		       data.ipv6.flowhi4,
> -		       data.ipv6.nexthdr,
> -		       data.ipv6.hoplimit);
> -	else if (m->ol_flags & RTE_PKT_RX_DYNF_PROTO_XTR_IPV6_FLOW)
> -		printf(" - Protocol Extraction:[0x%04x:0x%04x],ipv6_flow,ver=%u,tc=%u,flow=0x%x%04x",
> -		       data.raw.data0, data.raw.data1,
> -		       data.ipv6_flow.version,
> -		       data.ipv6_flow.tc,
> -		       data.ipv6_flow.flowhi4,
> -		       data.ipv6_flow.flowlo16);
> -	else if (m->ol_flags & RTE_PKT_RX_DYNF_PROTO_XTR_TCP)
> -		printf(" - Protocol Extraction:[0x%04x:0x%04x],tcp,doff=%u,flags=%s%s%s%s%s%s%s%s",
> -		       data.raw.data0, data.raw.data1,
> -		       data.tcp.doff,
> -		       data.tcp.cwr ? "C" : "",
> -		       data.tcp.ece ? "E" : "",
> -		       data.tcp.urg ? "U" : "",
> -		       data.tcp.ack ? "A" : "",
> -		       data.tcp.psh ? "P" : "",
> -		       data.tcp.rst ? "R" : "",
> -		       data.tcp.syn ? "S" : "",
> -		       data.tcp.fin ? "F" : "");
> -	else if (m->ol_flags & RTE_PKT_RX_DYNF_PROTO_XTR_IP_OFFSET)
> -		printf(" - Protocol Offset:ip_offset=%u",
> -		       data.ip_ofs);
> -}
> +#endif
>  
>  #ifdef __cplusplus
>  }
> diff --git a/drivers/net/ice/version.map b/drivers/net/ice/version.map
> index 632a296a0c..4a76d1d52d 100644
> --- a/drivers/net/ice/version.map
> +++ b/drivers/net/ice/version.map
> @@ -1,16 +1,3 @@
>  DPDK_21 {
>  	local: *;
>  };
> -
> -EXPERIMENTAL {
> -	global:
> -
> -	# added in 19.11
> -	rte_net_ice_dynfield_proto_xtr_metadata_offs;
> -	rte_net_ice_dynflag_proto_xtr_vlan_mask;
> -	rte_net_ice_dynflag_proto_xtr_ipv4_mask;
> -	rte_net_ice_dynflag_proto_xtr_ipv6_mask;
> -	rte_net_ice_dynflag_proto_xtr_ipv6_flow_mask;
> -	rte_net_ice_dynflag_proto_xtr_tcp_mask;
> -	rte_net_ice_dynflag_proto_xtr_ip_offset_mask;
> -};
> -- 
> 2.29.0
>
  

Hi Olivier,

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Monday, October 26, 2020 18:22
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Chen, Zhaoyan <zhaoyan.chen@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> Ray Kinsella <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>
> Subject: Re: [dpdk-dev] [PATCH v1] net/ice: refactor dynamic mbuf in data extraction
> 
> Hi Haiyue,
> 
> On Sun, Oct 25, 2020 at 03:13:52PM +0800, Haiyue Wang wrote:
> > Current dynamic mbuf design is that the driver will register the needed
> > field and flags at the device probing time, this will make iavf PMD use
> > different names to register the dynamic mbuf field and flags, but both
> > of them use the exactly same protocol extraction metadata.
> >
> > This will run out of the limited dynamic mbuf resource, meanwhile, the
> > application has to handle the dynamic mbuf separately.
> >
> > For making things simple and consistent, refactor dynamic mbuf in data
> > extraction handling: the PMD just lookups the same name at the queue
> > setup time after the application registers it.
> >
> > In other words, make the dynamic mbuf string name as API, not the data
> > object which is defined in each PMD.
> 
> In case the dynamic mbuf field is shared by several PMDs, it seems to
> be indeed a better solution.
> 
> Currently, the "union rte_pmd_proto_xtr_metadata" is still defined in
> rte_pmd_ice.h. Will it be the same for iavf, and will it be factorized
> somewhere? However I don't know where could be a good place.
> 
> There is already lib/librte_mbuf/rte_mbuf_dyn.h which is the place to
> centralize the name and description of dynamic fields/flags used in
> libraries. But I think neither structure definitions nor PMD-specific
> flags should go there too. I'd prefer to have them in
> drivers/net/<common-something>, but I'm not sure it is possible.

May be new 'lib/librte_mbuf/rte_mbuf_dyn_pmd.h' for all PMDs specific ?
So that the application knows exactly how many *dynamic* things. Also,
a new API to query the dynamic information + dev_ops may be introduced
in next release cycle, then 'rte_pmd_mlx5_get_dyn_flag_names' can be
removed. And the application will be clean.

Currently, we use " #define __INTEL_RX_FLEX_DESC_METADATA__ " to fix the
duplicated definition, but the application have to include the two header
files like "rte_pmd_ice.h" / "rte_pmd_iavf.h"

> 
> Also, it is difficult from the patch to see the impact it has on an
> application that was using these metadata. Should we have an example
> of use?
> 

Thanks your link in previous mail:
http://inbox.dpdk.org/dev/20191030165626.w3flq5wdpitpsv2v@platinum/

Original patch uses: Solution 1, provide static inline helpers to access to the
dyn fields/flags

Now now: Solution 2, without global variable export and helpers: the application
calls rte_mbuf_dynfield_register(&rte_pmd_ice_proto_xtr_metadata_param) to get
the offset, and store it privately.

https://patchwork.dpdk.org/patch/82165/
In v3 patch, I kept the metadata format, and rename it to be more generic:
'union rte_pmd_proto_xtr_metadata', but no dump function as the original design.

> Thanks,
> Olivier
> 
> 
> > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> > ---
> > 2.29.0
> >