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

  Remove the "reserved2" field and add the "packet_type" and the "inner_l2_l3_len" fields in the rte_mbuf structure.

The packet type field is used to indicate ordinary L2 packet format and also tunneling packet format such as IP in IP,
IP in GRE, MAC in GRE and MAC in UDP.

The inner L2 length and the inner L3 length are used for TX offloading of tunneling packet.
 
Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>
Acked-by: Jingjing Wu <jingjing.wu@intel.com>
---
 lib/librte_mbuf/rte_mbuf.h |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)
  

Hi Jijiang,

2014-10-21 16:46, Jijiang Liu:
> Remove the "reserved2" field and add the "packet_type"

"Remove and add" can be said "Replace".

> and the "inner_l2_l3_len" fields in the rte_mbuf structure.

Please explain that you are using 2 bytes of the second cache line
for TX offloading of tunnels.

>  	/* remaining bytes are set on RX when pulling packet from descriptor */
>  	MARKER rx_descriptor_fields1;
> -	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"?
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, October 21, 2014 6:26 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 1/9] librte_mbuf:the rte_mbuf structure
> changes
> 
> Hi Jijiang,
> 
> 2014-10-21 16:46, Jijiang Liu:
> > Remove the "reserved2" field and add the "packet_type"
> 
> "Remove and add" can be said "Replace".
> 
> > and the "inner_l2_l3_len" fields in the rte_mbuf structure.
> 
> Please explain that you are using 2 bytes of the second cache line for TX
> offloading of tunnels.
> 
> >  	/* remaining bytes are set on RX when pulling packet from descriptor
> */
> >  	MARKER rx_descriptor_fields1;
> > -	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"?

In datasheet, this term is called packet type(s).
Personally , I think packet type is  more clear what meaning of this field is . ^_^

> --
> Thomas
  

2014-10-21 14:14, Liu, Jijiang:
> From: Thomas Monjalon [mailto:thomas.monjalon@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"?
> 
> 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.

Thanks
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, October 22, 2014 4:46 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 1/9] librte_mbuf:the rte_mbuf structure
> changes
> 
> 2014-10-21 14:14, Liu, Jijiang:
> > From: Thomas Monjalon [mailto:thomas.monjalon@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"?
> >
> > 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.
 its generic meaning is that each UNIT  number stand for a kind of packet format。
I will add more description for this field.


> Thanks
> --
> Thomas
  

Hi

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Wednesday, October 22, 2014 4:46 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 1/9] librte_mbuf:the rte_mbuf structure
> changes
> 
> 2014-10-21 14:14, Liu, Jijiang:
> > From: Thomas Monjalon [mailto:thomas.monjalon@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.

> >
> > 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.
> 
> Thanks
> --
> Thomas

Regards,
Helin
  

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@dpdk.org] On Behalf Of Thomas Monjalon
> > 2014-10-21 14:14, Liu, Jijiang:
> > > From: Thomas Monjalon [mailto:thomas.monjalon@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).
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, November 12, 2014 9:26 PM
> To: Liu, Jijiang
> Cc: Zhang, Helin; dev@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@dpdk.org] On Behalf Of Thomas Monjalon
> > > 2014-10-21 14:14, Liu, Jijiang:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@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
Though I haven't investigate this too much, my opinion is that we should
use packet_type in the future, and rework igb/ixgbe PMD to remove all
packet types in ol_flags and use packet_type instead.
Then example app can use the packet type directly. And all igb, ixgbe and
i40e packet_type are consistent. Sure we might need to define all packet
types in rte_ethdev.h or similar header files.

> 
> 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).
> 
> --
> Thomas

Regards,
Helin
  

2014-11-12 14:31, Zhang, Helin:
> > 2014-10-23 02:23, Zhang, Helin:
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > > > 2014-10-21 14:14, Liu, Jijiang:
> > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > 2014-10-21 16:46, Jijiang Liu:
> > > > > > > +	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
> 
> Though I haven't investigate this too much, my opinion is that we should
> use packet_type in the future, and rework igb/ixgbe PMD to remove all
> packet types in ol_flags and use packet_type instead.
> Then example app can use the packet type directly. And all igb, ixgbe and
> i40e packet_type are consistent. Sure we might need to define all packet
> types in rte_ethdev.h or similar header files.

Exact!

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

Helin, I feel you perfectly understood the problem.
As the responsible of i40e, you can make a choice for 1.8 release:
- remove (incomplete) packet_type
- or complete it quickly

Thanks
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, November 12, 2014 9:26 PM
> To: Liu, Jijiang
> Cc: Zhang, Helin; dev@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@dpdk.org] On Behalf Of Thomas Monjalon
> > > 2014-10-21 14:14, Liu, Jijiang:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@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
  

2014-11-13 03:17, Liu, Jijiang:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2014-10-23 02:23, Zhang, Helin:
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > > > 2014-10-21 14:14, Liu, Jijiang:
> > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > 2014-10-21 16:46, Jijiang Liu:
> > > > > > > +	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,
> ... 
> }

OK, it seems well abstracted.
I think the last part of these names (PAY3/PAY4) is useless.

When this patch for API and i40e will be ready?
I'd prefer fixing the API instead of removing it.

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

When the patches for igb/ixgbe will be ready?

Thanks
  

Hi, 

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, November 13, 2014 4:53 PM
> To: Liu, Jijiang
> Cc: Zhang, Helin; dev@dpdk.org; Richardson, Bruce
> Subject: Re: [dpdk-dev] [PATCH v6 1/9] librte_mbuf:the rte_mbuf structure
> changes
> 
> 2014-11-13 03:17, Liu, Jijiang:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2014-10-23 02:23, Zhang, Helin:
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas
> > > > Monjalon
> > > > > 2014-10-21 14:14, Liu, Jijiang:
> > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > 2014-10-21 16:46, Jijiang Liu:
> > > > > > > > +	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,
> > ...
> > }
> 
> OK, it seems well abstracted.
> I think the last part of these names (PAY3/PAY4) is useless.
> 
> When this patch for API and i40e will be ready?
> I'd prefer fixing the API instead of removing it.

If needed, next week, I can send a patch for this.

> > 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.
> 
> When the patches for igb/ixgbe will be ready?
We need some time to investigate this for igb/ixgbe, probably some example codes and test application codes need to changed. 
You can assume that it cannot be done in DPDK1.8.

So here are my three suggestions:

1. keep packet_type in mbuf and wait for all the igb/ixgb/i40e changes done in DPDK2.0. Now, I don't send a separate  patch set for it. 
2. keep  packet_type in mbuf, I just send i40e patch set for this in DPDK1.8. In DPDK2.0, we will send a patch set  for igb/ixgbe.
3. It can be removed now,  and we will send a separate patch set for introducing packet type in the future.

> Thanks
> --
> Thomas
  

2014-11-13 11:24, Liu, Jijiang:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2014-11-13 03:17, Liu, Jijiang:
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > 2014-10-23 02:23, Zhang, Helin:
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas
> > > > > Monjalon
> > > > > > 2014-10-21 14:14, Liu, Jijiang:
> > > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > > 2014-10-21 16:46, Jijiang Liu:
> > > > > > > > > +	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,
> > > ...
> > > }
> > 
> > OK, it seems well abstracted.
> > I think the last part of these names (PAY3/PAY4) is useless.
> > 
> > When this patch for API and i40e will be ready?
> > I'd prefer fixing the API instead of removing it.
> 
> If needed, next week, I can send a patch for this.
> 
> > > 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.
> > 
> > When the patches for igb/ixgbe will be ready?
> 
> We need some time to investigate this for igb/ixgbe, probably some
> example codes and test application codes need to changed. 
> You can assume that it cannot be done in DPDK1.8.
> 
> So here are my three suggestions:
> 
> 1. keep packet_type in mbuf and wait for all the igb/ixgb/i40e changes
> done in DPDK2.0. Now, I don't send a separate  patch set for it. 
> 2. keep  packet_type in mbuf, I just send i40e patch set for this in
> DPDK1.8. In DPDK2.0, we will send a patch set  for igb/ixgbe.
> 3. It can be removed now,  and we will send a separate patch set for
> introducing packet type in the future.

Option 2 please :)
My main concerns are:
- clearly document it
- have hardware abstraction

Thanks