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

[Olivier MATZ <olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

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