[dpdk-dev,03/13] mbuf: add packet_type field

  Replace a reserved slot with the new packet type field used to identify
the type of the packet, i.e. what protocols are used.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_mbuf/rte_mbuf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Hi Bruce,

On 09/03/2014 05:49 PM, Bruce Richardson wrote:
> Replace a reserved slot with the new packet type field used to identify
> the type of the packet, i.e. what protocols are used.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/librte_mbuf/rte_mbuf.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index f136d37..8d0c6fb 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -146,7 +146,7 @@ struct rte_mbuf {
>  	uint32_t reserved1;     /**< Unused field. Required for padding */
>  
>  	/* remaining bytes are set on RX when pulling packet from descriptor */
> -	uint16_t reserved2;     /**< Unused field. Required for padding */
> +	uint16_t packet_type;   /**< Type of packet, e.g. protocols used */
>  	uint16_t data_len;      /**< Amount of data in segment buffer. */
>  	uint32_t pkt_len;       /**< Total pkt len: sum of all segments. */
>  	uint16_t l3_len:9;      /**< L3 (IP) Header Length. */
> 

This patch adds a new fields that nobody uses. So why should we add it ?
  

I would use it :)
It's useful to store the IP protocol number (UDP, TCP etc) and version
of IP (4, 6) and then relay packet to specific handler.

08.09.2014 16:17, Olivier MATZ пишет:
> Hi Bruce,
>
> On 09/03/2014 05:49 PM, Bruce Richardson wrote:
>> Replace a reserved slot with the new packet type field used to identify
>> the type of the packet, i.e. what protocols are used.
>>
>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>> ---
>>  lib/librte_mbuf/rte_mbuf.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index f136d37..8d0c6fb 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -146,7 +146,7 @@ struct rte_mbuf {
>>  	uint32_t reserved1;     /**< Unused field. Required for padding */
>>  
>>  	/* remaining bytes are set on RX when pulling packet from descriptor */
>> -	uint16_t reserved2;     /**< Unused field. Required for padding */
>> +	uint16_t packet_type;   /**< Type of packet, e.g. protocols used */
>>  	uint16_t data_len;      /**< Amount of data in segment buffer. */
>>  	uint32_t pkt_len;       /**< Total pkt len: sum of all segments. */
>>  	uint16_t l3_len:9;      /**< L3 (IP) Header Length. */
>>
> This patch adds a new fields that nobody uses. So why should we add it ?
  

Hi Yerden,

On 09/08/2014 12:33 PM, Yerden Zhumabekov wrote:
> 08.09.2014 16:17, Olivier MATZ пишет:
>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>> @@ -146,7 +146,7 @@ struct rte_mbuf {
>>>  	uint32_t reserved1;     /**< Unused field. Required for padding */
>>>  
>>>  	/* remaining bytes are set on RX when pulling packet from descriptor */
>>> -	uint16_t reserved2;     /**< Unused field. Required for padding */
>>> +	uint16_t packet_type;   /**< Type of packet, e.g. protocols used */
>>>  	uint16_t data_len;      /**< Amount of data in segment buffer. */
>>>  	uint32_t pkt_len;       /**< Total pkt len: sum of all segments. */
>>>  	uint16_t l3_len:9;      /**< L3 (IP) Header Length. */
>>>
>> This patch adds a new fields that nobody uses. So why should we add it ?
> 
> I would use it :)
> It's useful to store the IP protocol number (UDP, TCP etc) and version
> of IP (4, 6) and then relay packet to specific handler.

I'm not saying this field is useless. But even if it's useful
for some applications like yours, it does not mean that it should go in
the generic mbuf structure.

Also, for a new field, we should define who is in charge of filling it.
Is is the driver? Does it mean that all drivers have to be modified to
fill it? Or is it just a placeholder for applications? In this case,
shouldn't we use application-specific metadata? In the other direction
(TX), we would also need to define if this field must be filled by the
application before transmitting a mbuf to a driver.

Regards,
Olivier
  

Hi Olivier,

> -----Original Message-----

> From: Zhu, Heqing

> Sent: Tuesday, September 09, 2014 9:48 AM

> To: Wu, Jingjing; Liu, Jijiang; Zhang, Helin

> Cc: Zhu, Heqing

> Subject: FW: [dpdk-dev] [PATCH 03/13] mbuf: add packet_type field

> 

> One of you need respond to this thread? Please make the answer generic and

> easy to understand/accept if possible.

> 

> -----Original Message-----

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ

> Sent: Monday, September 08, 2014 7:17 PM

> To: Yerden Zhumabekov; Richardson, Bruce; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH 03/13] mbuf: add packet_type field

> 

> Hi Yerden,

> 

> On 09/08/2014 12:33 PM, Yerden Zhumabekov wrote:

> > 08.09.2014 16:17, Olivier MATZ пишет:

> >>> --- a/lib/librte_mbuf/rte_mbuf.h

> >>> +++ b/lib/librte_mbuf/rte_mbuf.h

> >>> @@ -146,7 +146,7 @@ struct rte_mbuf {

> >>>  	uint32_t reserved1;     /**< Unused field. Required for padding */

> >>>

> >>>  	/* remaining bytes are set on RX when pulling packet from descriptor

> */

> >>> -	uint16_t reserved2;     /**< Unused field. Required for padding */

> >>> +	uint16_t packet_type;   /**< Type of packet, e.g. protocols used */

> >>>  	uint16_t data_len;      /**< Amount of data in segment buffer. */

> >>>  	uint32_t pkt_len;       /**< Total pkt len: sum of all segments. */

> >>>  	uint16_t l3_len:9;      /**< L3 (IP) Header Length. */

> >>>

> >> This patch adds a new fields that nobody uses. So why should we add it ?

> >

> > I would use it :)

> > It's useful to store the IP protocol number (UDP, TCP etc) and version

> > of IP (4, 6) and then relay packet to specific handler.

> 

> I'm not saying this field is useless. But even if it's useful for some applications

> like yours, it does not mean that it should go in the generic mbuf structure.


In some types of NIC, for an example, Intel Fortville, which supports various packet types, and packet type value is filled in a field of receive Descriptor by HW,
Normally, application analyses easily what incoming packet format is if it know what packet type is. It is a common approach for analyzing packet format.

> Also, for a new field, we should define who is in charge of filling it.

> Is is the driver? Does it mean that all drivers have to be modified to fill it? Or is

> it just a placeholder for applications? In this case, shouldn't we use

> application-specific metadata?


Yes, driver is in charge of filling it if this type of NIC has a packet type filed in receive Descriptor.

> In the other direction (TX), we would also need

> to define if this field must be filled by the application before transmitting a

> mbuf to a driver.


Normally, TX side don't care the packet type, instead, feature offload flag will be used in TX side.
         In RX side, NIC HW analyses incoming packet and know what packet type is, and fill packet type value in Receive Descriptor.
Of course, I also don't object to add a packet type in TX side if mbuf TX space is enough, but from the present point of view, it is not necessary. 

> Regards,

> Olivier


Regards,
Jijiang Liu
  

> -----Original Message-----

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ

> Sent: Monday, September 8, 2014 7:17 PM

> To: Yerden Zhumabekov; Richardson, Bruce; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH 03/13] mbuf: add packet_type field

> 

> Hi Yerden,

> 

> On 09/08/2014 12:33 PM, Yerden Zhumabekov wrote:

> > 08.09.2014 16:17, Olivier MATZ пишет:

> >>> --- a/lib/librte_mbuf/rte_mbuf.h

> >>> +++ b/lib/librte_mbuf/rte_mbuf.h

> >>> @@ -146,7 +146,7 @@ struct rte_mbuf {

> >>>  	uint32_t reserved1;     /**< Unused field. Required for padding */

> >>>

> >>>  	/* remaining bytes are set on RX when pulling packet from descriptor

> */

> >>> -	uint16_t reserved2;     /**< Unused field. Required for padding */

> >>> +	uint16_t packet_type;   /**< Type of packet, e.g. protocols used */

> >>>  	uint16_t data_len;      /**< Amount of data in segment buffer. */

> >>>  	uint32_t pkt_len;       /**< Total pkt len: sum of all segments. */

> >>>  	uint16_t l3_len:9;      /**< L3 (IP) Header Length. */

> >>>

> >> This patch adds a new fields that nobody uses. So why should we add it ?

> >

> > I would use it :)

> > It's useful to store the IP protocol number (UDP, TCP etc) and version

> > of IP (4, 6) and then relay packet to specific handler.


It is a common field which i40e PMD will use it to store the 'packet type ID'. i40e
hardware can recognize more than a hundred of packet types of received packets,
this is quite useful for upper layer stack or application. So this field is quite useful
and will be filled by PMD.
In ixgbe/igb, it has less than 10 packet types which are marked in offload flags. From
now on, it would be better to have new field here to put the hardware offloaded
packet type in and it could be used for future NICs.

> 

> I'm not saying this field is useless. But even if it's useful for some applications

> like yours, it does not mean that it should go in the generic mbuf structure.

> 

> Also, for a new field, we should define who is in charge of filling it.

> Is is the driver? Does it mean that all drivers have to be modified to fill it? Or is

> it just a placeholder for applications? In this case, shouldn't we use

> application-specific metadata? In the other direction (TX), we would also need

> to define if this field must be filled by the application before transmitting a mbuf

> to a driver.

Yes, PMD will fill it. I40e PMD will be the first one, ixgbe/igb can be kept as it is, or
modified to be consistent. It is used for RX side only, and for TX side, it can be
investigated to see if it can be used also. I think some new features in development
can think of that.
Anyway, it is a quite useful field for i40e and future generation of NICs.
> 

> Regards,

> Olivier
  

Hello,

On 09/09/2014 05:59 AM, Zhang, Helin wrote:
> It is a common field which i40e PMD will use it to store the 'packet type ID'. i40e
> hardware can recognize more than a hundred of packet types of received packets,
> this is quite useful for upper layer stack or application. So this field is quite useful
> and will be filled by PMD.
> In ixgbe/igb, it has less than 10 packet types which are marked in offload flags. From
> now on, it would be better to have new field here to put the hardware offloaded
> packet type in and it could be used for future NICs.
>
>>
>> I'm not saying this field is useless. But even if it's useful for some applications
>> like yours, it does not mean that it should go in the generic mbuf structure.
>>
>> Also, for a new field, we should define who is in charge of filling it.
>> Is is the driver? Does it mean that all drivers have to be modified to fill it? Or is
>> it just a placeholder for applications? In this case, shouldn't we use
>> application-specific metadata? In the other direction (TX), we would also need
>> to define if this field must be filled by the application before transmitting a mbuf
>> to a driver.
> Yes, PMD will fill it. I40e PMD will be the first one, ixgbe/igb can be kept as it is, or
> modified to be consistent. It is used for RX side only, and for TX side, it can be
> investigated to see if it can be used also. I think some new features in development
> can think of that.
> Anyway, it is a quite useful field for i40e and future generation of NICs.

To me, having the support in a hardware for that feature is not a
sufficient reason for adding this field. There are many hardware
features that will never be integrated in dpdk.

This first version of the patch:

- just adds a field that is not used by any code, so it is useless.
   At least testpmd or an application example should show how to
   use it.

- does not describe what enhancement is provided by adding the
   field (performance? in this case, numbers + use case would help
   to convince people).

- does not describe what can be the content of the field. Is it
   a protocol number?

- does not explain if all drivers must fill this field. If yes,
   the patch has to update all drivers. If not, something must be
   done to mark the packet field as unknown by default.

Regards,
Olivier
  

> -----Original Message-----

> From: Olivier MATZ [mailto:olivier.matz@6wind.com]

> Sent: Tuesday, September 9, 2014 4:03 PM

> To: Zhang, Helin; Yerden Zhumabekov; Richardson, Bruce; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH 03/13] mbuf: add packet_type field

> 

> Hello,

> 

> On 09/09/2014 05:59 AM, Zhang, Helin wrote:

> > It is a common field which i40e PMD will use it to store the 'packet

> > type ID'. i40e hardware can recognize more than a hundred of packet

> > types of received packets, this is quite useful for upper layer stack

> > or application. So this field is quite useful and will be filled by PMD.

> > In ixgbe/igb, it has less than 10 packet types which are marked in

> > offload flags. From now on, it would be better to have new field here

> > to put the hardware offloaded packet type in and it could be used for future

> NICs.

> >

> >>

> >> I'm not saying this field is useless. But even if it's useful for

> >> some applications like yours, it does not mean that it should go in the

> generic mbuf structure.

> >>

> >> Also, for a new field, we should define who is in charge of filling it.

> >> Is is the driver? Does it mean that all drivers have to be modified

> >> to fill it? Or is it just a placeholder for applications? In this

> >> case, shouldn't we use application-specific metadata? In the other

> >> direction (TX), we would also need to define if this field must be

> >> filled by the application before transmitting a mbuf to a driver.

> > Yes, PMD will fill it. I40e PMD will be the first one, ixgbe/igb can

> > be kept as it is, or modified to be consistent. It is used for RX side

> > only, and for TX side, it can be investigated to see if it can be used

> > also. I think some new features in development can think of that.

> > Anyway, it is a quite useful field for i40e and future generation of NICs.

> 

> To me, having the support in a hardware for that feature is not a sufficient

> reason for adding this field. There are many hardware features that will never

> be integrated in dpdk.


At least this field is quite important for i40e.
e.g. packet type=43 means that hardware recognize it as a VXLAN packet. To avoid checking what type of packet by software, hardware can offload that, and fill the packet type ID in that field.
It cannot be put in ol_flags anymore, as it has more than 100 packet type can be recognized by hardware. Without it, vxlan feature cannot be implemented at all. 

> 

> This first version of the patch:

> 

> - just adds a field that is not used by any code, so it is useless.

>    At least testpmd or an application example should show how to

>    use it.


It will be used at least in vxlan feature which is in development. Without it, vxlan cannot be completed. So this is a very important field i40e and future NICs.

>

> - does not describe what enhancement is provided by adding the

>    field (performance? in this case, numbers + use case would help

>    to convince people).


I40e hardware can recognize received packets as different packet types, and there are about 256 packet types can be recognized by i40e hardware. It is not a enhancement, it is the key for at least i40e features.

> 

> - does not describe what can be the content of the field. Is it

>    a protocol number?

> 


The packet type is a offload feature of hardware, the value of it can mean one type of packet recognized by the i40e hardware. E.g.

| Packet type | Description            |
| 0         | Reserved              |
| 1         | MAC, PAY2            |
| 2         | MAC, TimeSync, PAY2    |
...
| 43        | MAC, IPV4, GRENAT, PAY3 |

> - does not explain if all drivers must fill this field. If yes,

>    the patch has to update all drivers. If not, something must be

>    done to mark the packet field as unknown by default.

> 

I40e needs it.
igb/ixgbe can be changed to support it, but not mandatory, as ol_flags can represent it.
Actually ixgbe and igb has packet type also, but the number of those types is less than 10, so it can be put in ol_flags. For i40e and future NICs, the number of that could be 256 or more, ol_flags does not have that many bits for it. The best idea is to fill the packet type ID directly into a field.

> Regards,

> Olivier


Regards,
Helin
  

> -----Original Message-----

> From: Olivier MATZ [mailto:olivier.matz@6wind.com]

> Sent: Tuesday, September 09, 2014 9:03 AM

> To: Zhang, Helin; Yerden Zhumabekov; Richardson, Bruce; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH 03/13] mbuf: add packet_type field

> 

> Hello,

> 

> On 09/09/2014 05:59 AM, Zhang, Helin wrote:

> > It is a common field which i40e PMD will use it to store the 'packet type ID'.

> i40e

> > hardware can recognize more than a hundred of packet types of received

> packets,

> > this is quite useful for upper layer stack or application. So this field is quite

> useful

> > and will be filled by PMD.

> > In ixgbe/igb, it has less than 10 packet types which are marked in offload flags.

> From

> > now on, it would be better to have new field here to put the hardware

> offloaded

> > packet type in and it could be used for future NICs.

> >

> >>

> >> I'm not saying this field is useless. But even if it's useful for some applications

> >> like yours, it does not mean that it should go in the generic mbuf structure.

> >>

> >> Also, for a new field, we should define who is in charge of filling it.

> >> Is is the driver? Does it mean that all drivers have to be modified to fill it? Or

> is

> >> it just a placeholder for applications? In this case, shouldn't we use

> >> application-specific metadata? In the other direction (TX), we would also

> need

> >> to define if this field must be filled by the application before transmitting a

> mbuf

> >> to a driver.

> > Yes, PMD will fill it. I40e PMD will be the first one, ixgbe/igb can be kept as it

> is, or

> > modified to be consistent. It is used for RX side only, and for TX side, it can be

> > investigated to see if it can be used also. I think some new features in

> development

> > can think of that.

> > Anyway, it is a quite useful field for i40e and future generation of NICs.

> 

> To me, having the support in a hardware for that feature is not a

> sufficient reason for adding this field. There are many hardware

> features that will never be integrated in dpdk.

> 

> This first version of the patch:

> 

> - just adds a field that is not used by any code, so it is useless.

>    At least testpmd or an application example should show how to

>    use it.

> 

> - does not describe what enhancement is provided by adding the

>    field (performance? in this case, numbers + use case would help

>    to convince people).

> 

> - does not describe what can be the content of the field. Is it

>    a protocol number?

> 

> - does not explain if all drivers must fill this field. If yes,

>    the patch has to update all drivers. If not, something must be

>    done to mark the packet field as unknown by default.

> 

> Regards,

> Olivier


Hi,

Points taken. Really, this patch doesn't belong in this set as I had planned things and better belongs in patch set 3 (coming soon, I hope) which should propose the new field additions. I simply put it here to avoid having to start renumbering and renaming reserved fields in the structure, but that is possibly the lesser of the two evils.

However, with regards to adding new fields in, I would like to have some agreement that I can add fields in without actually pushing in the patch to use them - so long as sufficient rational is provided for using the field and there is a soon pending change to actually use it. This patch did not meet the criteria for explanation, but if updated to do so, I would like to have this patch accepted on the basis of that explanation so as to enable those working on the drivers to make us of it. 

Regards,
/Bruce
  

> On Sep 8, 2014, at 4:17 AM, Olivier MATZ <olivier.matz@6wind.com> wrote:
> 
> Hi Yerden,
> 
> On 09/08/2014 12:33 PM, Yerden Zhumabekov wrote:
>> 08.09.2014 16:17, Olivier MATZ пишет:
>>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>>> @@ -146,7 +146,7 @@ struct rte_mbuf {
>>>> 	uint32_t reserved1;     /**< Unused field. Required for padding */
>>>> 
>>>> 	/* remaining bytes are set on RX when pulling packet from descriptor */
>>>> -	uint16_t reserved2;     /**< Unused field. Required for padding */
>>>> +	uint16_t packet_type;   /**< Type of packet, e.g. protocols used */
>>>> 	uint16_t data_len;      /**< Amount of data in segment buffer. */
>>>> 	uint32_t pkt_len;       /**< Total pkt len: sum of all segments. */
>>>> 	uint16_t l3_len:9;      /**< L3 (IP) Header Length. */
>>>> 
>>> This patch adds a new fields that nobody uses. So why should we add it ?
>> 
>> I would use it :)
>> It's useful to store the IP protocol number (UDP, TCP etc) and version
>> of IP (4, 6) and then relay packet to specific handler.
> 
> I'm not saying this field is useless. But even if it's useful
> for some applications like yours, it does not mean that it should go in
> the generic mbuf structure.
> 
> Also, for a new field, we should define who is in charge of filling it.
> Is is the driver? Does it mean that all drivers have to be modified to
> fill it? Or is it just a placeholder for applications? In this case,
> shouldn't we use application-specific metadata? In the other direction
> (TX), we would also need to define if this field must be filled by the
> application before transmitting a mbuf to a driver.


Funny, but these new fields (and extended mbuf) were prominent during the dpdk summit.

I think it’s going to be quite useful.