[dpdk-dev] [PATCH 0/1] proposed minor change in rte_flow_validate semantics

John Daley (johndale) johndale at cisco.com
Fri Mar 24 18:23:23 CET 2017


Hi Adrien,

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> Sent: Friday, March 24, 2017 2:47 AM
> To: John Daley (johndale) <johndale at cisco.com>
> Cc: dev at dpdk.org
> Subject: Re: [PATCH 0/1] proposed minor change in rte_flow_validate
> semantics
> 
> Hi John,
> 
> On Thu, Mar 23, 2017 at 07:36:58PM -0700, John Daley wrote:
> > Hi,
> >
> > In implementing rte_flow_validate() for the Cisco enic, I got to
> > wondering if the semantics might be slightly off given how I see apps using
> it.
> >
> > Please forgive me if this has already been discussed, but during
> > runtime, I can't see any reason why rte_flow_validate() would be
> > called when
> > rte_flow_create() returns the same errors as validate, but also
> > creates the filter if all is good (calling validate first is just extra overhead).
> > What are the use cases for calling validate when the app is up and running?
> 
> In short (I'll elaborate below), to get a rough idea of the capabilities of a given
> port at initialization time. One should use rte_flow_validate() with
> nonspecific rules to determine if the PMD understands them at all *given the
> circumstances* (the current configuration state of the device).
> 
> Nothing prevents one from using it just before calling rte_flow_create(),
> however doing so is indeed redundant.
> 
> > I see rte_flow_validate() being run at startup for answering these 2
> questions:
> > 1.) Given enough resources, is the device capable of handling flows a,
> > b, c, ..,  which the app may want to create during runtime?
> 
> Yes, breaking this down means all the following:
> 
> 1. Recognize all pattern items and actions.
> 2. Make sure items are properly stacked and actions can be combined.
> 3. Check for pattern item specification structures (e.g. limits, masks,
>    ranges, and so on).
> 4. Check for actions configuration structures (e.g. target queue exists).
> 
> Depending on the flow rule, 3. and 4. may have to check the current device
> configuration in order to validate the rule.

Agreed. You didn't include checking for duplicate patterns with conflicting actions. Is that a requirement also? I hope some of that and some of 1, 2 and 3 can be pushed up into rte_ether/flow someday. I.e. only present flows to the PMDs which are already valid, so the PMD just needs to see if it is acceptable to the device.

> 
> > 2.) How many concurrent flows of type a, b, c, .. can the device
> > handle? To answer this app needs to do a bunch of rte_flow_create()'s
> (followed by a bunch of rte_flow_destroy()s).
> 
> Hehe, this is the brute force approach, actually the rte_flow API in its current
> form is not supposed to answer this question, and I don't think we should
> document it that way (note you could use rte_flow_flush() to destroy all
> flows at once faster).
> 
> Some devices (like mlx5) do not really have an upper limit on the number of
> possible flows one can configure, they're basically limited by the available
> memory on the host system.
> 
> > The answer to these questions will allow the app to decide beforehand
> > which filters to offload (if any) and set up function pointers or
> > whatever to run as efficiently as possible on the target device.
> >
> > rte_flow_validate() has comments that imply it should be run on the
> > fly,like "the flow rule is validated against its current configuration
> > state and the returned value should be considered valid by the caller for
> that state only".
> > The only real way to implement this is for enic (and I think other
> > nics) is to actually do a flow create/destroy within
> > rte_flow_validate(). But, see paragraph 2 above- why bother? I looked
> > at the i40e and mlx5 implementations and I didn't see any calls to the
> > nics to check a flow against current state so they might not be
> implemented as the comment imply they should be either.
> 
> Then you're probably right there is an issue with the documentation, it's not
> the intent (more below).
> 
> > If there are no use cases for calling validate at runtime,  I think we
> > ought to change the semantics of rte_flow_validate() a little to
> > define success to mean that the device will accept the flow IF there
> > are enough resources, rather than it will accept the flow given the
> > current state. We can safely remove the -EEXIST return code since no
> > other drivers set them yet. This makes the function easier to
> > implement correctly without trying to do something that's not useful
> anyway.
> >
> > The following patch just changes comments but I think that is all that
> > is required.
> 
> rte_flow_validate() and rte_flow_create() share the same error codes to
> make their implementation simpler. rte_flow_validate() basically exposes
> the validation part necessarily present in rte_flow_create().
> 
> EEXIST and other error codes *may* be returned by rte_flow_validate(), but
> this does not force PMDs to check possible collisions during the validation
> step, I'll take documentation is not clear enough regarding this.

My scan of the other PMDs didn't turn up any EEXIXT or EBUSY return codes from rte_flow_validate(). I suppose future implementation could, but I doubt they will since intel/mlx PMDs set the standard and they do not.

> 
> So to be completely clear, rte_flow_validate() already does *not* guarantee
> a rule can be created afterward, only that rte_flow_create() will understand
> that rule.
> 
> The other reason "configuration state" is mentioned in the documentation
> (besides target queues should exist) is that existing rules may have switched
> the device in a mode that does not allow different rule types.
> 
> If after initialization, matching TCP, UDP and ICMP is possible, creating a UDP
> rule might subsequently prevent the creation of otherwise valid TCP and
> ICMP rules. rte_flow_validate() should (but is not forced to) check for that.
> 
> What do you think about keeping the defined error codes as is and merging
> somehow my above statements in the documentation instead?

I guess I still don't see much value of checking a rule against the current state and no other PMDs are doing this. I still think just having a rte_flow_validate that answers my question 1 above has a lot of value and implying it does more (and having different PMDs do different things) will just confuse users (not to mention PMD maintainers ;). Maybe just cleaning up the documentations as you say is good enough. I think removing return error codes that can't be counted on is even better though. Otherwise the documentation gets harder, right? We would have to basically say "don't count on ENOMEM and EXIST doing what you think because they probably won't...." . My 2 cents, thanks!

> 
> --
> Adrien Mazarguil
> 6WIND


More information about the dev mailing list