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

["Richardson, Bruce" <bruce.richardson@intel.com>]

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