[dpdk-dev] FW: Issues with ixgbe and rte_flow

[Adrien Mazarguil]

Hi Nicolas,

On Wed, Mar 15, 2017 at 02:29:44PM +0000, Le Scouarnec Nicolas wrote:
> Hi Adrien, 
> 
> > > > > And about the tpid, ethertype. I have a thought that why we need it as it's
> > > > duplicate with the item type. I think the initial design is just following the IEEE
> > > > spec to define the structures so we will not miss anything. But why not do some
> > > > optimization. For VLAN the tpid must be 0x8100, for IPv4, the ethertype must be
> > > > 0x0800. So why bothering let APP provide them and driver check them? Seems
> > > > we can just remove these fields from the structures, it can make things simpler.
> > > > >
> > > > > Adrien, as you're the maintainer of rte_flow, any thought about these ideas?
> > > > Thanks.
> > >
> > > > Basically I think we must give users the flexibility to provide nonstandard TPIDs
> > > > as well (there's apparently already a few), so we can't just leave it out entirely.
> > > Agree that TPID or something else like that can be changed. But my point is when
> > > using the filter, users don't care about the value of TPID, they only care about the
> > > vlan packets should be filtered. The type already tells the driver that. 
> > > No matter we use  the well-known or self-defined TPID, it's duplicate of vlan type.
> 
> > You're right about the usefulness of specifying TPID in most cases, however
> > because the pattern is not arranged in the same order as the packet, users
> > do not know what EtherType they should specify inside struct
> > rte_flow_item_eth if they want to provide one, which I think will haunt us
> > for a long time (Nicolas' feedback gave me this impression.)
> 
> > I 'm now convinced modifying rte_flow_eth_vlan could make this much clearer
> > and intend to update the API accordingly. So far affected PMDs would be
> > i40e, ixgbe, mlx4, mlx5, sfc and tap.
> 
> Overall, as a user, I feel one difficulty/complexity in using the API comes from the need to
> specify both the stack of protocol (in type) and at each level the "next protocol type" of the header (in spec).
> 
> Then, the PMD has to check that what I specified as the "next protocol type" is coherent with the protocol
> stack before setting up the filters. Basically, for a valid filter, I should always have
> rte_flow_item[1].type == rte_flow_item[0].spec.type, and  rte_flow_item[2].type == rte_flow_item[1].spec.{type,next_protocol}
>  (except for L2.5 protocol as I experienced, which makes thinks confusing). Couldn't the API leverage this fact so that I don't
> need to specify the ether_type, TPID, next_protocol_id, ... which are redundant with rte_flow_item.type ?

Just to be clear, as a user you don't *need* to provide them, however the
API certainly does not prevent you to do so. Only masked fields are
relevant, and the default item masks (rte_flow_item_*_mask) do not include
protocol types because as you're pointing out, that would indeed be a pain.

Is the documentation not clear enough regarding this?
(see "8.2.3 Pattern item")

Also, items like VLAN can already have several valid "types" (0x88a8,
0x8100, 0x9100), and who knows what will come up in the future. This allows
applications to request matching a specific type only if they care about it.

> Changing this wouldn't reduce the expressiveness of the filter, as long as there is a rte_flow_item type
> CustomL3OverEthernet where you can specify the ether_type expected at L2, and a CustomL4OverIP
> where you can specifiy the next protocol id at L3 (IP), and if needed to support custom TPID, CustomVLANOverEthernet.
> These would be used by a small minority of users, thus keeping the API simpler for users of most common protocols. 

I think adding custom types would be more complicated than the current
approach of leaving the payload type field unspecified or set it to some
custom value that PMDs may or may not accept depending on their
capabilities.

If you take testpmd's flow command for instance, what is not explicitly
specified on the command-line is left unmasked:

 flow create 0 pattern eth / ipv4 dst is 10.1.1.1 / udp / end actions drop / end

This rule does not request specific ETH nor IP payload types, testpmd does
not generate them and it works as intended. I think this behavior already
satisfies the above requirements.

Best regards,

-- 
Adrien Mazarguil
6WIND