Re: [dpdk-dev] [PATCH v1] net/ice: refactor dynamic mbuf in data extraction

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

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
> >