[dpdk-dev] [PATCH v6 1/9] librte_mbuf:the rte_mbuf structure changes

[Liu, Jijiang]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Wednesday, November 12, 2014 9:26 PM
> To: Liu, Jijiang
> Cc: Zhang, Helin; dev at dpdk.org; Richardson, Bruce
> Subject: Re: [dpdk-dev] [PATCH v6 1/9] librte_mbuf:the rte_mbuf structure
> changes
> 
> Hi guys,
> 
> We still have some problems with the mbuf changes introduced for VXLAN.
> I want to raise the packet type issue here.
> 
> 2014-10-23 02:23, Zhang, Helin:
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon
> > > 2014-10-21 14:14, Liu, Jijiang:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > > > > 2014-10-21 16:46, Jijiang Liu:
> > > > > > -	uint16_t reserved2;       /**< Unused field. Required for padding
> */
> > > > > > +
> > > > > > +	/**
> > > > > > +	 * Packet type, which is used to indicate ordinary L2 packet
> format and
> > > > > > +	 * also tunneled packet format such as IP in IP, IP in GRE, MAC in
> GRE
> > > > > > +	 * and MAC in UDP.
> > > > > > +	 */
> > > > > > +	uint16_t packet_type;
> > > > >
> > > > > Why not name it "l2_type"?
> >
> > 'packet_type' is for storing the hardware identified packet type upon
> > different layers of protocols (l2, l3, l4, ...).
> > It is quite useful for user application or middle layer software
> > stacks, it can know what the packet type is without checking the packet too
> much by software.
> > Actually ixgbe already has packet types (less than 10), which is transcoded into
> 'ol_flags'.
> > For i40e, the packet type can represent about 256 types of packet,
> > 'ol_flags' does not have enough bits for it anymore. So put the i40e packet types
> into mbuf would be better.
> > Also this field can be used for NON-Intel NICs, I think there must be
> > the similar concepts of other NICs. And 16 bits 'packet_type' has severl
> reserved bits for future and NON-Intel NICs.
> 
> Thanks Helin, that's the best description of packet_type I've seen so far.
> It's not so clear in the commit log:
> 	http://dpdk.org/browse/dpdk/commit/?id=73b7d59cf4f6faf
> 
> > > > In datasheet, this term is called packet type(s).
> > >
> > > That's exactly the point I want you really understand!
> > > This is a field in generic mbuf structure, so your datasheet has no value here.
> > >
> > > > Personally , I think packet type is  more clear what meaning of this field is .
> > >
> > > You cannot add an API field without knowing what will be its generic meaning.
> > > Please think about it and describe its scope.
> 
> I integrated this patch with the VXLAN patchset in the hope that you'll improve
> the situation afterwards.
> This is the answer you recently gave to Olivier:
> 	http://dpdk.org/ml/archives/dev/2014-November/007599.html
> "
> 	Regarding adding a packet_type in mbuf, we ever had a lot of discussions
> as follows:
> 	http://dpdk.org/ml/archives/dev/2014-October/007027.html
> 	http://dpdk.org/ml/archives/dev/2014-September/005240.html
> 	http://dpdk.org/ml/archives/dev/2014-September/005241.html
> 	http://dpdk.org/ml/archives/dev/2014-September/005274.html
> "
> 
> To sum up the situation:
> - We don't know what are the possible values of packet_type
> - It's only filled by i40e, while other drivers use ol_flags
> - There is no special value "unknown" which should be set by drivers
>   not supporting this feature.
> - Its only usage is to print a decimal value in app/test-pmd/rxonly.c
> 
> It's now clear that nobody cares about this part of the API.
> So I'm going to remove packet_type from mbuf.
> I don't want to keep something that we don't know how to use, that is not
> consistent across drivers, and that overlap another API part (ol_flags).

The packet type in 40e is very important for user, using packet type can help to speed up packet analysis/identification in their application, especially tunneling packet format.
Now I'm working on implementing packet type definition in rte_ethdev.h file and  translation table in i40e, which is almost done. 
The packet type  definition in in rte_ethdev.h file like below. 
/*
 * Ethernet packet type
 */
enum rte_eth_ptype {
        /* undefined packet type, means HW can't recognise it */
        RTE_PTYPE_UNDEF = 0,
...

        /* IPv4 --> GRE/Teredo/VXLAN --> MAC --> IPv4 */
        RTE_PTYPE_IPv4_GRENAT_MAC_IPv4FRAG_PAY3,
        RTE_PTYPE_IPv4_GRENAT_MAC_IPv4_PAY3,
        RTE_PTYPE_IPv4_GRENAT_MAC_IPv4_UDP_PAY4,
        RTE_PTYPE_IPv4_GRENAT_MAC_IPv4_TCP_PAY4,
        RTE_PTYPE_IPv4_GRENAT_MAC_IPv4_SCTP_PAY4,
        RTE_PTYPE_IPv4_GRENAT_MAC_IPv4_ICMP_PAY4,
 
        /* IPv4 --> GRE/Teredo/VXLAN --> MAC --> IPv6 */
        RTE_PTYPE_IPv4_GRENAT_MAC_IPv6FRAG_PAY3
        RTE_PTYPE_IPv4_GRENAT_MAC_IPv6_PAY3,
        RTE_PTYPE_IPv4_GRENAT_MAC_IPv6_UDP_PAY4,
        RTE_PTYPE_IPv4_GRENAT_MAC_IPv6_TCP_PAY4,
        RTE_PTYPE_IPv4_GRENAT_MAC_IPv6_SCTP_PAY4,
        RTE_PTYPE_IPv4_GRENAT_MAC_IPv6_ICMP_PAY4,
 
        /* IPv4 --> GRE/Teredo/VXLAN --> MAC/VLAN */
        RTE_PTYPE_IPv4_GRENAT_MACVLAN_PAY3,
... 
}

Yes, we don't use packet type in many places now, which doesn't mean we don't use it  in the future( when supporting another tunneling packet).

It is ok for me if you want to remove the packet_type filed in mbuf,  but we will send a separate patch set for introducing packet type in the future, which includes 1g/10/40g PMD changes.

> --
> Thomas