[dpdk-dev] [PATCH 2/2] net/i40e: configurable PTYPE mapping

Zhang, Qi Z qi.z.zhang at intel.com
Tue Mar 7 03:37:02 CET 2017


Hi Ferruh:
	
	Thanks all the good suggestion and help.

	
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, March 6, 2017 11:32 PM
> To: Zhang, Qi Z <qi.z.zhang at intel.com>; Wu, Jingjing
> <jingjing.wu at intel.com>; Zhang, Helin <helin.zhang at intel.com>
> Cc: dev at dpdk.org; Lu, Wenzhuo <wenzhuo.lu at intel.com>
> Subject: Re: [dpdk-dev] [PATCH 2/2] net/i40e: configurable PTYPE mapping
> 
> On 2/27/2017 4:56 AM, Qi Zhang wrote:
> > The patch adds 4 APIs to support configurable PTYPE mapping for i40e
> > device.
> > rte_pmd_i40e_update_ptype_mapping.
> > rte_pmd_i40e_reset_ptype_mapping.
> > rte_pmd_i40e_get_ptype_mapping.
> > rte_pmd_i40e_replace_ptype_mapping.
> 
> Hi Qi,
> 
> These are added as PMD specific APIs, but not used anywhere, how did you
> test them? Or how others can test it?
> 
> Does it make sense to add them into testpmd?
> 
Yes, I plan to add 
> 
> 
> And related to API naming, what do you think about following syntax:
> <name_space>_<object>_<action> ?
> 
> This helps finding APIs for same object (ptype_mapping for this case):
> 
> rte_pmd_i40e_ptype_mapping_update()
> rte_pmd_i40e_ptype_mapping_reset()
> rte_pmd_i40e_ptype_mapping_get()
> rte_pmd_i40e_ptype_mapping_replace()

Agree, that's good.
> 
> 
> And not directly related to this patch, but it is good idea to extract PMD
> specific APIs into separate file, would you mind taking that task before this
> patch? And add these new APIs to that new file?
> 
I like this idea, since we have many changes on this recently, I'd better discuss with 
other team member to decide if do the separation before merge or after the merge.
> 
> > The mapping from hardware defined packet type to software defined
> > packet type can be updated/reset/read out with these APIs.
> > Also a software ptype with the most significent bit set will be
> > regarded as a custom defined ptype
> > (RTE_PMD_I40E_PTYPE_USER_DEFINE_MASK) so application can use it to
> defined its own PTYPE naming system.
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang at intel.com>
> > ---
> >  drivers/net/i40e/i40e_ethdev.c  | 190
> ++++++++++++++++++++++++++++++++++++++++
> >  drivers/net/i40e/i40e_rxtx.c    |   1 -
> >  drivers/net/i40e/i40e_rxtx.h    |   2 +
> >  drivers/net/i40e/rte_pmd_i40e.h |  81 +++++++++++++++++
> 
> Need to update *version.map file too, for shared library build. It is hard to
> catch these issues since APIs are not used.
> 
Will fix.
> <...>
> 
> > +
> > +int rte_pmd_i40e_update_ptype_mapping(
> 
> DPDK coding convention suggest having return type in separate line:
> int
> rte_pmd_i40e_update_ptype_mapping(...
> 
> > +			uint8_t port,
> > +			struct rte_pmd_i40e_ptype_mapping *mapping_items,
> > +			uint16_t count,
> > +			uint8_t exclusive)
> > +{
> > +	struct rte_eth_dev *dev;
> > +	struct i40e_adapter *ad;
> > +	int i;
> 
> For PMD specific APIs, port_id needs to be verified if it is i40e port or not.
> There is already is_device_supported() function in i40e for this.

Thanks
> 
> CC'ed Wenzhuo, he already did this a few times, and may help.
> 
> <...>
> 
> > +/**
> > + * Update hardware defined ptype to software defined packet type
> > + * mapping table.
> > + *
> > + * @param port
> > + *    pointer to port identifier of the device.
> > + * @param mapping_items
> > + *    the base address of the mapping items array.
> > + * @param count
> > + *    number of mapping items.
> > + * @param exclusive
> > + *    the flag indicate different ptype mapping update method.
> > + *    -(0) only overwrite refferred PTYPE mapping,
> 
> referred
> 
> > + *	keep other PTYPEs mapping unchanged.
> > + *    -(!0) overwrite referred PTYPE mapping,
> > + *	set other PTYPEs maps to PTYPE_UNKNOWN.
> > + */
> > +int rte_pmd_i40e_update_ptype_mapping(
> > +			uint8_t port,
> > +			struct rte_pmd_i40e_ptype_mapping *mapping_items,
> > +			uint16_t count,
> > +			uint8_t exclusive);
> > +
> > +/**
> > + * Reset hardware defined ptype to software defined ptype
> > + * mapping table to default.
> > + *
> > + * @param port
> > + *    pointer to port identifiier of the device
> 
> s/identifiier/identifier
> 
> <...>
> 
> > +/**
> > + * Replace a specific or a group of software defined ptypes
> > + * with a new one
> > + *
> > + * @param port
> > + *    pointer to port identifier of the device
> > + * @param target
> > + *    the packet type to be replaced
> > + * @param mask
> > + *    -(0) target represent a specific software defined ptype.
> > + *    -(!0) target is a mask to represent a group of software defined
> ptypes.
> > + * @param pkt_type
> > + *    the new packet type to overwrite
> > + */
> > +int rte_pmd_i40e_replace_pkt_type(uint8_t port,
> > +				  uint32_t target,
> > +				  uint8_t mask,
> > +				  uint32_t pkt_type);
> 
> API names does not match with one in commit log, and "pkt_type" usage is
> not consistent with other APIs.
Will fix.
> 
> > +
> >  #endif /* _PMD_I40E_H_ */
> >



More information about the dev mailing list