Re: [dpdk-dev] [PATCH v8] net/ice: optimize protocol extraction by dynamic mbuf API

["Wang, Haiyue" <haiyue.wang@intel.com>]

Hi Xiaolong,

> -----Original Message-----
> From: Ye, Xiaolong <xiaolong.ye@intel.com>
> Sent: Thursday, November 7, 2019 17:09
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; olivier.matz@6wind.com
> Subject: Re: [PATCH v8] net/ice: optimize protocol extraction by dynamic mbuf API
> 
> Hi, Haiyue
> 
> On 11/07, 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, a
> >private API is used to decode this metadata.
> >
> >Use the dynamic mbuf field and flags to register the needed fields in
> >mbuf, to avoid overwriting 'rte_mbuf::udata64' if the application uses
> >it. It only needs 4B size to save the protocol extraction data, and its
> >type and validity is indicated by related bit in 'rte_mbuf::ol_flags'.
> >
> >Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> >---
> >v8: - Cleanup the doxgen, rename the dynamic filed and flags with ice prefix.
> >
> >v7: - Change the log level from ERR to DEBUG for a successful registration,
> >      ERR is used to develop firstly.
> >
> >v6: - Don't need to register all flags when if one dev_args is set, register
> >      it as required.
> >
> >v5: - Remove the '_OL/_ol' in dynamic mbuf flag to make the variable clean.
> >
> >v4: - Include the 'rte_pmd_ice.h' header in ICE source as needed, reduce
> >      its compile scope.
> >
> >v3: - Use the conventions name "rte_net_<pmd>_dynfield_<name>" since it
> >      is defined in a in PMD.
> >    - Add helpers API for easiy access.
> >
> >v2: - disable the protocol extraction if failed to register some ol_flags
> >    - rewrite the commit message
> >
> > doc/api/doxy-api-index.md               |   1 +
> > doc/api/doxy-api.conf.in                |   1 +
> > doc/guides/nics/ice.rst                 |  14 +-
> > drivers/net/ice/ice_ethdev.c            |  84 ++++++-
> > drivers/net/ice/ice_ethdev.h            |   9 +
> > drivers/net/ice/ice_rxtx.c              |  88 ++++---
> > drivers/net/ice/ice_rxtx.h              |   1 -
> > drivers/net/ice/rte_pmd_ice.h           | 312 +++++++++++++++---------
> > drivers/net/ice/rte_pmd_ice_version.map |  12 +
> > 9 files changed, 369 insertions(+), 153 deletions(-)
> >
> 
> [snip]
> 
> >
> >-  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, and the related dynamic mbuf flags is set.
> 
> Above 'with' is redundant?

Yes, will fix next version.

> 
> >
> >   .. table:: Protocol extraction : ``vlan``
> >
> >@@ -175,10 +176,11 @@ 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.
> >+  Use ``rte_net_ice_dynf_proto_xtr_metadata_get`` to access the protocol
> >+  extraction metadata, and use ``PKT_RX_DYNF_PROTO_XTR_*`` to get the
> >+  metadata type of ``struct rte_mbuf::ol_flags``.
> >
> >-  The ``dump_proto_xtr_flds(struct rte_mbuf *mb)`` routine shows how to
> >+  The ``rte_net_ice_dump_proto_xtr_metadata`` routine shows how to
> >   access the protocol extraction result in ``struct rte_mbuf``.
> >
> 
> [snip]
> 
> >+
> >+	PMD_DRV_LOG(DEBUG,
> >+		    "Protocol extraction metadata offset in mbuf is : %d",
> >+		    offset);
> >+	rte_net_ice_dynfield_proto_xtr_metadata = offset;
> 
> Seems rte_net_ice_dump_proto_xtr_metadata_off is a better name judging from
> its real meaning.
> 

Make sense, will change it.

> >+
> >+	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)
> 
> [snip]
> 
> >+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);
> 
> ol_flag here is obtained through offset which is returned by
> rte_mbuf_dynflag_register, will it have any chance to conflict with
> existing offload flags such as PKT_RX_VLAN, PKT_RX_VLAN_STRIPPED, ...?
> 

This API will choose between "PKT_FIRST_FREE" and "PKT_LAST_FREE",
not conflict. ;-)

> >+	if (unlikely(!ol_flag))
> >+		return;
> >+
> >+	mb->ol_flags |= ol_flag;
> >+
> 
> [snip]
> 
> >--- a/drivers/net/ice/rte_pmd_ice_version.map
> >+++ b/drivers/net/ice/rte_pmd_ice_version.map
> >@@ -2,3 +2,15 @@ DPDK_19.02 {
> >
> > 	local: *;
> > };
> >+
> >+EXPERIMENTAL {
> >+	global:
> >+
> >+	# added in 19.11
> >+	rte_net_ice_dynfield_proto_xtr_metadata;
> >+	rte_net_ice_dynflag_proto_xtr_vlan;
> >+	rte_net_ice_dynflag_proto_xtr_ipv4;
> >+	rte_net_ice_dynflag_proto_xtr_ipv6;
> >+	rte_net_ice_dynflag_proto_xtr_ipv6_flow;
> >+	rte_net_ice_dynflag_proto_xtr_tcp;
> 
> Why put the variable other than the experimental api in the map?
> 

The experimental API is static and inline, not global, so no need
to be put here. But the above variables are global, so need to be
put here for compiling shared.

Please refer to: git log -p lib/librte_ethdev/rte_ethdev_version.map

> Thanks,
> Xiaolong
> 
> >+};
> >--
> >2.17.1
> >