Issues with ixgbe and rte_flow

Issues with ixgbe and rte_flow

[Le Scouarnec Nicolas]

Dear all,

I have been using the new API rte_flow to program filtering on an X540 (ixgbe) NIC. My goal is to send packets from different VLANs to different queues (filtering which should be supported by flow director as far as I understand). I enclosed the setup code at the bottom of this email. However, the code in ixgbe rejects the request I formulated. I looked at the rte_flow related code in ixgbe and I found some weird checkings in function ixgbe_parse_fdir_filter_normal. The code verifies that vlan_spec->tpid == ETHER_TYPE_VLAN, which should never happen on real packets, as it is eth_spec->type which has this value in typical setups. The same comment applies to the mask, only eth_mask->tpid should be required to be 0xffff. Overall, unless I misunderstood the rte_flow API, the checks should be done on eth_spec/eth_mask rather than vlan_spec/vlan_mask.  

I also have a side comment which might be more related to the general rte_flow API than to the specific implementation in ixgbe. The rte_flow_error returned is not very useful for it does return the error of the last tried filter-type (L2 tunnel in ixgbe), and not the error of the filter-type that my setup should use (flow director). As NICs can have several filter-types, It would be be useful to the user if rte_flow_validate/create could return the errors for all filter types tried although that would require to change the rte_flow API and returning an array of rte_flow_error and not a single struct. 

Excerpt of the code that reject my filtering rule:

              if (vlan_spec->tpid != rte_cpu_to_be_16(ETHER_TYPE_VLAN)) {
			memset(rule, 0, sizeof(struct ixgbe_fdir_rule));
			rte_flow_error_set(error, EINVAL,
				RTE_FLOW_ERROR_TYPE_ITEM,
				item, "Not supported by fdir filter");
			return -rte_errno;
		}


		rule->ixgbe_fdir.formatted.vlan_id = vlan_spec->tci;


		if (vlan_mask->tpid != (uint16_t)~0U) {
			memset(rule, 0, sizeof(struct ixgbe_fdir_rule));
			rte_flow_error_set(error, EINVAL,
				RTE_FLOW_ERROR_TYPE_ITEM,
				item, "Not supported by fdir filter");
			return -rte_errno;
		}

For reference, here is the setup code I use

	struct rte_flow_attr attr;
	attr.group = 0;
	attr.priority = 0;
	attr.egress = 0;
	attr.ingress = 1;


	struct rte_flow_item_eth eth_mask;
	struct rte_flow_item_eth eth_spec;
	struct rte_flow_item_vlan vlan_mask;
	struct rte_flow_item_vlan vlan_spec;
	struct rte_flow_action_queue queue;


	struct rte_flow_item items[3];
	items[0].type = RTE_FLOW_ITEM_TYPE_ETH;
	items[1].type = RTE_FLOW_ITEM_TYPE_VLAN;
	items[0].last = NULL;
	items[0].spec = &eth_spec;
	items[0].mask = &eth_mask;
	items[1].last = NULL;
	items[1].spec = &vlan_spec;
	items[1].mask = &vlan_mask;
	items[2].type = RTE_FLOW_ITEM_TYPE_END;


	eth_mask.src = ETH(00,00,00,00,00,00);
	eth_mask.dst = ETH(ff,ff,ff,ff,ff,ff);
	eth_mask.type = 0xffff;


	eth_spec.src = ETH(00,00,00,00,00,00);
	eth_spec.dst = *dst;
	eth_spec.type = rte_cpu_to_be_16(ETHER_TYPE_VLAN);


	vlan_spec.tci = vlan_be;
	vlan_spec.tpid = 0;


	vlan_mask.tci = rte_cpu_to_be_16(0x0fff);
	vlan_mask.tpid =  0;


	struct rte_flow_action actions[2];
	actions[0].type = RTE_FLOW_ACTION_TYPE_QUEUE;
	actions[0].conf = &queue;
	actions[1].type = RTE_FLOW_ACTION_TYPE_END;
	actions[1].conf = NULL;
	queue.index = queue_id;


	struct rte_flow_error error;


	printf("%p %p %p %p %p\n", &items[0], &items[1], &items[2], &actions[0], &actions[1]);


	if(rte_flow_validate(port_id, &attr, items, actions, &error) < 0){
...

Best regards,
Nicolas Le Scouarnec



     
 This e-mail and any files transmitted with it are confidential and are intended solely for the use of the individual or entity to whom they are addressed.  If you are not the intended recipient or the person responsible  for delivering this communication to the intended recipient, please be advised that you have received this communication in error and that any use, dissemination, printing or copying of this communication (including any attachments) is strictly prohibited.
 
    

Re: Issues with ixgbe and rte_flow

[Lu, Wenzhuo]

Hi Le Scouarnec,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Le Scouarnec Nicolas
> Sent: Tuesday, March 7, 2017 7:12 PM
> To: dev@dpdk.org
> Cc: Yigit, Ferruh
> Subject: [dpdk-dev] Issues with ixgbe and rte_flow
> 
> Dear all,
> 
> I have been using the new API rte_flow to program filtering on an X540 (ixgbe)
> NIC. My goal is to send packets from different VLANs to different queues
> (filtering which should be supported by flow director as far as I understand). I
> enclosed the setup code at the bottom of this email. However, the code in ixgbe
> rejects the request I formulated. I looked at the rte_flow related code in ixgbe
> and I found some weird checkings in function ixgbe_parse_fdir_filter_normal.
> The code verifies that vlan_spec->tpid == ETHER_TYPE_VLAN, which should
> never happen on real packets, as it is eth_spec->type which has this value in
> typical setups. The same comment applies to the mask, only eth_mask->tpid
> should be required to be 0xffff. Overall, unless I misunderstood the rte_flow API,
> the checks should be done on eth_spec/eth_mask rather than
> vlan_spec/vlan_mask.
> 
> I also have a side comment which might be more related to the general rte_flow
> API than to the specific implementation in ixgbe. The rte_flow_error returned is
> not very useful for it does return the error of the last tried filter-type (L2 tunnel
> in ixgbe), and not the error of the filter-type that my setup should use (flow
> director). As NICs can have several filter-types, It would be be useful to the user
> if rte_flow_validate/create could return the errors for all filter types tried
> although that would require to change the rte_flow API and returning an array
> of rte_flow_error and not a single struct.
> 
> Excerpt of the code that reject my filtering rule:
> 
>               if (vlan_spec->tpid != rte_cpu_to_be_16(ETHER_TYPE_VLAN)) {
> 			memset(rule, 0, sizeof(struct ixgbe_fdir_rule));
> 			rte_flow_error_set(error, EINVAL,
> 				RTE_FLOW_ERROR_TYPE_ITEM,
> 				item, "Not supported by fdir filter");
> 			return -rte_errno;
> 		}
> 
> 
> 		rule->ixgbe_fdir.formatted.vlan_id = vlan_spec->tci;
> 
> 
> 		if (vlan_mask->tpid != (uint16_t)~0U) {
> 			memset(rule, 0, sizeof(struct ixgbe_fdir_rule));
> 			rte_flow_error_set(error, EINVAL,
> 				RTE_FLOW_ERROR_TYPE_ITEM,
> 				item, "Not supported by fdir filter");
> 			return -rte_errno;
> 		}
> 
> For reference, here is the setup code I use
> 
> 	struct rte_flow_attr attr;
> 	attr.group = 0;
> 	attr.priority = 0;
> 	attr.egress = 0;
> 	attr.ingress = 1;
> 
> 
> 	struct rte_flow_item_eth eth_mask;
> 	struct rte_flow_item_eth eth_spec;
> 	struct rte_flow_item_vlan vlan_mask;
> 	struct rte_flow_item_vlan vlan_spec;
> 	struct rte_flow_action_queue queue;
> 
> 
> 	struct rte_flow_item items[3];
> 	items[0].type = RTE_FLOW_ITEM_TYPE_ETH;
> 	items[1].type = RTE_FLOW_ITEM_TYPE_VLAN;
> 	items[0].last = NULL;
> 	items[0].spec = &eth_spec;
> 	items[0].mask = &eth_mask;
> 	items[1].last = NULL;
> 	items[1].spec = &vlan_spec;
> 	items[1].mask = &vlan_mask;
> 	items[2].type = RTE_FLOW_ITEM_TYPE_END;
> 
> 
> 	eth_mask.src = ETH(00,00,00,00,00,00);
> 	eth_mask.dst = ETH(ff,ff,ff,ff,ff,ff);
> 	eth_mask.type = 0xffff;
> 
> 
> 	eth_spec.src = ETH(00,00,00,00,00,00);
> 	eth_spec.dst = *dst;
> 	eth_spec.type = rte_cpu_to_be_16(ETHER_TYPE_VLAN);
> 
> 
> 	vlan_spec.tci = vlan_be;
> 	vlan_spec.tpid = 0;
> 
> 
> 	vlan_mask.tci = rte_cpu_to_be_16(0x0fff);
> 	vlan_mask.tpid =  0;
To my opinion, this setting is not right. As we know, vlan tag is inserted between MAC source address and Ether type.
So if we have a MAC+VLAN+IPv4 packet, the vlan_spec.tpid should be 0x8100, the eth_spec.type should be 0x0800.
+ Adrien, the author. He can correct me if I'm wrong.

Re: Issues with ixgbe and rte_flow

[Le Scouarnec Nicolas]

My response is inline bellow, and further comment on the code excerpt also


From: Lu, Wenzhuo <wenzhuo.lu@intel.com>
Sent: Wednesday, March 8, 2017 4:16 AM
To: Le Scouarnec Nicolas; dev@dpdk.org; Adrien Mazarguil (adrien.mazarguil@6wind.com)
Cc: Yigit, Ferruh
Subject: RE: Issues with ixgbe and rte_flow
    
>> I have been using the new API rte_flow to program filtering on an X540 (ixgbe)
>> NIC. My goal is to send packets from different VLANs to different queues
>> (filtering which should be supported by flow director as far as I understand). I
>> enclosed the setup code at the bottom of this email.
>> For reference, here is the setup code I use
>>
>>       vlan_spec.tci = vlan_be;
>>       vlan_spec.tpid = 0;
>>
>>       vlan_mask.tci = rte_cpu_to_be_16(0x0fff);
>>       vlan_mask.tpid =  0;

>To my opinion, this setting is not right. As we know, vlan tag is inserted between MAC source address and Ether type.
>So if we have a MAC+VLAN+IPv4 packet, the vlan_spec.tpid should be 0x8100, the eth_spec.type should be 0x0800.
>+ Adrien, the author. He can correct me if I'm wrong.

Ok, I apologize, you're right. Being more used to the software-side than to the hardware-side, I misunderstood struct rte_flow_item_vlan and though it was the "equivalent" of struct vlan_hdr, in which case the vlan_hdr contains the type of the encapsulated frame.

(  /**
 * Ethernet VLAN Header.
 * Contains the 16-bit VLAN Tag Control Identifier and the Ethernet type
 * of the encapsulated frame.
 */
struct vlan_hdr {
	uint16_t vlan_tci; /**< Priority (3) + CFI (1) + Identifier Code (12) */
	uint16_t eth_proto;/**< Ethernet type of encapsulated frame. */
} __attribute__((__packed__));        )


Best regards,
Nicolas Le Scouarnec

Re: Issues with ixgbe and rte_flow

[Adrien Mazarguil]

CC'ing users@dpdk.org since this issue primarily affects rte_flow users, and
several PMD maintainers to get their opinion on the matter, see below.

On Wed, Mar 08, 2017 at 09:24:26AM +0000, Le Scouarnec Nicolas wrote:
> My response is inline bellow, and further comment on the code excerpt also
> 
> 
> From: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Sent: Wednesday, March 8, 2017 4:16 AM
> To: Le Scouarnec Nicolas; dev@dpdk.org; Adrien Mazarguil (adrien.mazarguil@6wind.com)
> Cc: Yigit, Ferruh
> Subject: RE: Issues with ixgbe and rte_flow
>     
> >> I have been using the new API rte_flow to program filtering on an X540 (ixgbe)
> >> NIC. My goal is to send packets from different VLANs to different queues
> >> (filtering which should be supported by flow director as far as I understand). I
> >> enclosed the setup code at the bottom of this email.
> >> For reference, here is the setup code I use
> >>
> >>       vlan_spec.tci = vlan_be;
> >>       vlan_spec.tpid = 0;
> >>
> >>       vlan_mask.tci = rte_cpu_to_be_16(0x0fff);
> >>       vlan_mask.tpid =  0;
> 
> >To my opinion, this setting is not right. As we know, vlan tag is inserted between MAC source address and Ether type.
> >So if we have a MAC+VLAN+IPv4 packet, the vlan_spec.tpid should be 0x8100, the eth_spec.type should be 0x0800.
> >+ Adrien, the author. He can correct me if I'm wrong.

That's right, however the confusion is understandable, perhaps the
documentation should be clearer. It currently states what follows without
describing the reason:

 /**
  * RTE_FLOW_ITEM_TYPE_VLAN
  *
  * Matches an 802.1Q/ad VLAN tag.
  *
  * This type normally follows either RTE_FLOW_ITEM_TYPE_ETH or
  * RTE_FLOW_ITEM_TYPE_VLAN.
  */

> Ok, I apologize, you're right. Being more used to the software-side than to the hardware-side, I misunderstood struct rte_flow_item_vlan and though it was the "equivalent" of struct vlan_hdr, in which case the vlan_hdr contains the type of the encapsulated frame.
> 
> (  /**
>  * Ethernet VLAN Header.
>  * Contains the 16-bit VLAN Tag Control Identifier and the Ethernet type
>  * of the encapsulated frame.
>  */
> struct vlan_hdr {
> 	uint16_t vlan_tci; /**< Priority (3) + CFI (1) + Identifier Code (12) */
> 	uint16_t eth_proto;/**< Ethernet type of encapsulated frame. */
> } __attribute__((__packed__));        )

Indeed, struct vlan_hdr and struct rte_flow_item_vlan are not mapped at the
same offset; the former includes EtherType of the inner packet (eth_proto),
while the latter describes the inserted VLAN header itself starting with
TPID.

This approach was chosen for rte_flow for consistency with the fact each
pattern item describes exactly one protocol header, even though in the case
of VLAN and other layer 2.5 protocols, some happen to be embedded.
IPv4/IPv6 options will be provided as separate items in a similar fashion.

It also allows adding/removing VLAN tags to an existing rule without
modifying the EtherType of the inner frame.

Now assuming you're not the only one facing that issue, if the current
definition does not make sense, perhaps we can update the API before it's
too late. I'll attempt to summarize it with an example below.

In any case, matching nonspecific VLAN-tagged and QinQ UDPv4 packets in
testpmd is written as:

 flow create 0 pattern eth / vlan / ipv4 / udp / end actions queue 1 / end
 flow create 0 pattern eth / vlan / vlan / ipv4 / udp / end actions queue 1 / end

However, with the current API described above, specifying inner/outer
EtherTypes for the above packets yields (as a reminder, 0x8100 stands for
VLAN, 0x8000 for IPv4 and 0x88A8 for QinQ):

#1

 flow create 0 pattern eth type is 0x8000 / vlan tpid is 0x8100 / ipv4 / udp / actions queue 1 / end
 flow create 0 pattern eth type is 0x8000 / vlan tpid is 0x88A8 / vlan tpid is 0x8100 / ipv4 / udp / actions queue 1 / end

Instead of the arguably more accurate (renaming "tpid" to "inner_type" for
clarity):

#2

 flow create 0 pattern eth type is 0x8100 / vlan type is 0x8000 / ipv4 / udp / actions queue 1 / end
 flow create 0 pattern eth type is 0x88A8 / vlan inner_type is 0x8100 / vlan inner_type is 0x8000 / ipv4 / udp / actions queue 1 / end

So, should the VLAN item be updated to behave as described in #2?

Note: doing so will cause a serious API/ABI breakage, I know it was not
supposed to happen according to the rte_flow sales pitch, but hey.

-- 
Adrien Mazarguil
6WIND

Re: FW: Issues with ixgbe and rte_flow

[Adrien Mazarguil]

Hi,

On Fri, Mar 10, 2017 at 07:12:24AM +0000, Lu, Wenzhuo wrote:
> Some replies in line. 
> Send it again with off the users@dpdk.org. Seems I cannot send the mail successfully with it.

I'm removing everyone from the CC list and putting back dev@dpdk.org then,
let's not break everyone's DPDK-related spam filters anymore.

This is separate from the VLAN item issue mentioned in the same thread, I
think this one is related to the ixgbe implementation (sorry Wenzhuo :)
More below.

[...]
> Hi Le Scouarnec,
> 
> > -----Original Message-----
> > From: Le Scouarnec Nicolas 
[...]
> > I also have a side comment which might be more related to the general
> > rte_flow API than to the specific implementation in ixgbe. I don't know
> > if it is specific to ixgbe's implementation but, as a user, the
> > rte_flow_error returned was not very useful for it does return the error
> > of the last tried filter-type (L2 tunnel in ixgbe), and not the error of
> > the filter-type that my setup should use (flow director).

The helpfulness of error messages is entirely a PMD's responsibility since
they are not hard-coded into the API. rte_flow is deliberately not aware of
the various underlying APIs used by PMDs to implement flow rules.

In this case, assuming your rule could only work through flow director, the
PMD should have saved and reported the error encountered with that filter
type (either by saving it before attempting others, or recognizing that this
rule wouldn't work with others and not attempt them).

> > I had to change the order in which filters are tried in ixgbe code to
> > get a more useful error message. As NICs can have several filter-types,
> > It would be be useful to the user if rte_flow_validate/create could
> > return the errors for all filter types tried although that would require
> > to change the rte_flow API and returning an array of rte_flow_error and
> > not a single struct.

rte_flow_error is a compromise to provide a detailed explanation about the
errno value returned by a function, which describes exactly one error
(ideally the first error encountered).

While returning an array could provide additional details about subsequent
errors, I think it would needlessly complicate the API and make it slower
without much benefit, given that most (if not all) PMD functions return as
soon as one error is detected and also for performance reasons.

> It's a good suggestion.  I remember we have some discussion about how to feedback the error to the APP. I think the reason why we don't make it too complex because it's the first step of generic API.  Now we see some feedback from the users, we can keep optimizing it :)

Right. Note ixgbe could append several messages to rte_flow_error.message if
necessary as in such cases. Storage for the message is provided by the PMD
and can be const, static or dynamic.

However I really think the best approach would be to report the most
relevant (first) error only.

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

It's really about whether we want to make the inner type part of the VLAN
item with TPID outside or keep it as-is. Anyway please reply to my previous
message if you want to talk about that and let's fork this one to discuss
the rte_flow_error issue.

-- 
Adrien Mazarguil
6WIND

Re: FW: Issues with ixgbe and rte_flow

[Lu, Wenzhuo]

Hi Adrien,

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Friday, March 10, 2017 7:46 PM
> To: Lu, Wenzhuo
> Cc: Le Scouarnec Nicolas; dev@dpdk.org
> Subject: Re: FW: Issues with ixgbe and rte_flow
> 
> Hi,
> 
> On Fri, Mar 10, 2017 at 07:12:24AM +0000, Lu, Wenzhuo wrote:
> > Some replies in line.
> > Send it again with off the users@dpdk.org. Seems I cannot send the mail
> successfully with it.
> 
> I'm removing everyone from the CC list and putting back dev@dpdk.org then,
> let's not break everyone's DPDK-related spam filters anymore.
> 
> This is separate from the VLAN item issue mentioned in the same thread, I think
> this one is related to the ixgbe implementation (sorry Wenzhuo :) More below.
No need to sorry :) It's my bad. Forgot to mention that this rte_flow_error problem is related to the PMD implementation. 
More or less it caused by the HW. Although we have this generic filter, but in HW there're still several different filters, PMD has to check them one by one.

> 
> [...]
> > Hi Le Scouarnec,
> >
> > > -----Original Message-----
> > > From: Le Scouarnec Nicolas
> [...]
> > > I also have a side comment which might be more related to the
> > > general rte_flow API than to the specific implementation in ixgbe. I
> > > don't know if it is specific to ixgbe's implementation but, as a
> > > user, the rte_flow_error returned was not very useful for it does
> > > return the error of the last tried filter-type (L2 tunnel in ixgbe),
> > > and not the error of the filter-type that my setup should use (flow director).
> 
> The helpfulness of error messages is entirely a PMD's responsibility since they
> are not hard-coded into the API. rte_flow is deliberately not aware of the
> various underlying APIs used by PMDs to implement flow rules.
> 
> In this case, assuming your rule could only work through flow director, the PMD
> should have saved and reported the error encountered with that filter type
> (either by saving it before attempting others, or recognizing that this rule
> wouldn't work with others and not attempt them).
> 
> > > I had to change the order in which filters are tried in ixgbe code
> > > to get a more useful error message. As NICs can have several
> > > filter-types, It would be be useful to the user if
> > > rte_flow_validate/create could return the errors for all filter
> > > types tried although that would require to change the rte_flow API
> > > and returning an array of rte_flow_error and not a single struct.
> 
> rte_flow_error is a compromise to provide a detailed explanation about the
> errno value returned by a function, which describes exactly one error (ideally the
> first error encountered).
> 
> While returning an array could provide additional details about subsequent
> errors, I think it would needlessly complicate the API and make it slower without
> much benefit, given that most (if not all) PMD functions return as soon as one
> error is detected and also for performance reasons.
> 
> > It's a good suggestion.  I remember we have some discussion about how
> > to feedback the error to the APP. I think the reason why we don't make
> > it too complex because it's the first step of generic API.  Now we see
> > some feedback from the users, we can keep optimizing it :)
> 
> Right. Note ixgbe could append several messages to rte_flow_error.message if
> necessary as in such cases. Storage for the message is provided by the PMD and
> can be const, static or dynamic.
> 
> However I really think the best approach would be to report the most relevant
> (first) error only.
It's good if we can find which one is the most relevant. It sounds like introducing an AI to judge which kind of the filter is the best choice.
And considering some filters may have overlap, like n-tuple and flow director.  So maybe the first one is the only option here :)

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

> 
> It's really about whether we want to make the inner type part of the VLAN item
> with TPID outside or keep it as-is. Anyway please reply to my previous message
> if you want to talk about that and let's fork this one to discuss the
> rte_flow_error issue.
> 
> --
> Adrien Mazarguil
> 6WIND

Re: FW: Issues with ixgbe and rte_flow

[Adrien Mazarguil]

Hi Wenzhuo,

On Mon, Mar 13, 2017 at 02:33:52AM +0000, Lu, Wenzhuo wrote:
[...]
> > > It's a good suggestion.  I remember we have some discussion about how
> > > to feedback the error to the APP. I think the reason why we don't make
> > > it too complex because it's the first step of generic API.  Now we see
> > > some feedback from the users, we can keep optimizing it :)
> > 
> > Right. Note ixgbe could append several messages to rte_flow_error.message if
> > necessary as in such cases. Storage for the message is provided by the PMD and
> > can be const, static or dynamic.
> > 
> > However I really think the best approach would be to report the most relevant
> > (first) error only.
> It's good if we can find which one is the most relevant. It sounds like introducing an AI to judge which kind of the filter is the best choice.
> And considering some filters may have overlap, like n-tuple and flow director.  So maybe the first one is the only option here :)

OK, I also think it's better that way. 

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

I'll reply to the other thread about that.

-- 
Adrien Mazarguil
6WIND

Re: FW: Issues with ixgbe and rte_flow

[Le Scouarnec Nicolas]

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 ?

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. 

Best regards,

-- 
Nicolas Le Scouarnec

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

Re: FW: Issues with ixgbe and rte_flow

[Le Scouarnec Nicolas]

Hi Adrien,

>On Wed, Mar 15, 2017 at 02:29:44PM +0000, Le Scouarnec Nicolas wrote:
>> 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")

To me it wasn't clear that the PMD/DPDK would take care of "type" fields in network headers for me,
hence, I tried to set them correctly (and got it wrong for ether_type/tpid) -- I feared that filtering on VLAN tci
without restricting to VLAN packets (setting ether_type) would be undefined behavior. I just check ixgbe_flow and
as you said it just ignores the types and relies on the stack so my previous comment and suggestion
was wrong.

The documentation is very clear on struct and how to use them, but a few common examples (in C) would have been useful to me;
for example I could have noticed that the example never set the ether_type & cie. testpmd is hard to read as an example.

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

You're right. My comment was based on the misconception that it was mandatory to correctly specify ether_types / next_protocol_id / ...

Best regards.

Re: FW: Issues with ixgbe and rte_flow

[Adrien Mazarguil]

On Thu, Mar 16, 2017 at 05:01:43PM +0000, Le Scouarnec Nicolas wrote:
> 
> Hi Adrien,
> 
> >On Wed, Mar 15, 2017 at 02:29:44PM +0000, Le Scouarnec Nicolas wrote:
> >> 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")
> 
> To me it wasn't clear that the PMD/DPDK would take care of "type" fields in network headers for me,
> hence, I tried to set them correctly (and got it wrong for ether_type/tpid) -- I feared that filtering on VLAN tci
> without restricting to VLAN packets (setting ether_type) would be undefined behavior. I just check ixgbe_flow and
> as you said it just ignores the types and relies on the stack so my previous comment and suggestion
> was wrong.

Phew, I'm relieved!

> The documentation is very clear on struct and how to use them, but a few common examples (in C) would have been useful to me;
> for example I could have noticed that the example never set the ether_type & cie. testpmd is hard to read as an example.

I understand, testpmd is really meant to validate PMD functionality, it's
probably not the best implementation example to start with. I'll keep that
in mind during future evolutions.

> > 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.
> 
> You're right. My comment was based on the misconception that it was mandatory to correctly specify ether_types / next_protocol_id / ...

Well thanks to that you've raised an interesting issue with the VLAN item
(TBH Wenzhuo and other people warned me about that, at the time I was
certain it would not be a problem.) I'll attempt to address it as soon as
possible.

Best regards,

-- 
Adrien Mazarguil
6WIND