Re: [PATCH 01/22] ethdev: introduce generic flow API

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

Hi Kevin,

On Wed, Dec 14, 2016 at 11:48:04AM +0000, Kevin Traynor wrote:
> hi Adrien, sorry for the delay
> 
> <...>
> 
> >>>>
> >>>> Is it expected that the application or pmd will provide locking between
> >>>> these functions if required? I think it's going to have to be the app.
> >>>
> >>> Locking is indeed expected to be performed by applications. This API only
> >>> documents places where locking would make sense if necessary and expected
> >>> behavior.
> >>>
> >>> Like all control path APIs, this one assumes a single control thread.
> >>> Applications must take the necessary precautions.
> >>
> >> If you look at OVS now it's quite possible that you have 2 rx queues
> >> serviced by different threads, that would also install the flow rules in
> >> the software flow caches - possibly that could extend to adding hardware
> >> flows. There could also be another thread that is querying for stats. So
> >> anything that can be done to minimise the locking would be helpful -
> >> maybe query() could be atomic and not require any locking?
> > 
> > I think we need basic functions with as few constraints as possible on PMDs
> > first, this API being somewhat complex to implement on their side. That
> > covers the common use case where applications have a single control thread
> > or otherwise perform locking on their own.
> > 
> > Once the basics are there for most PMDs, we may add new functions, items,
> > properties and actions that provide additional constraints (timing,
> > multi-threading and so on), which remain to be defined according to
> > feedback. It is designed to be extended without causing ABI breakage.
> 
> I think Sugesh and I are trying to foresee some of the issues that may
> arise when integrating with something like OVS. OTOH it's
> hard/impossible to say what will be needed exactly in the API right now
> to make it suitable for OVS.
> 
> So, I'm ok with the approach you are taking by exposing a basic API
> but I think there should be an expectation that it may not be sufficient
> for a project like OVS to integrate in and may take several
> iterations/extensions - don't go anywhere!
> 
> > 
> > As for query(), let's see how PMDs handle it first. A race between query()
> > and create() on a given device is almost unavoidable without locking, same
> > for queries that reset counters in a given flow rule. Basic parallel queries
> > should not cause any harm otherwise, although this cannot be guaranteed yet.
> 
> You still have a race if there is locking, except it is for the lock,
> but it has the same effect. The downside of my suggestion is that all
> the PMDs would need to guarantee they could gets stats atomically - I'm
> not sure if they can or it's too restrictive.
> 
> > 
> 
> <...>
> 
> >>
> >>>
> >>>>> +
> >>>>> +/**
> >>>>> + * Destroy a flow rule on a given port.
> >>>>> + *
> >>>>> + * Failure to destroy a flow rule handle may occur when other flow rules
> >>>>> + * depend on it, and destroying it would result in an inconsistent state.
> >>>>> + *
> >>>>> + * This function is only guaranteed to succeed if handles are destroyed in
> >>>>> + * reverse order of their creation.
> >>>>
> >>>> How can the application find this information out on error?
> >>>
> >>> Without maintaining a list, they cannot. The specified case is the only
> >>> possible guarantee. That does not mean PMDs should not do their best to
> >>> destroy flow rules, only that ordering must remain consistent in case of
> >>> inability to destroy one.
> >>>
> >>> What do you suggest?
> >>
> >> I think if the app cannot remove a specific rule it may want to remove
> >> all rules and deal with flows in software for a time. So once the app
> >> knows it fails that should be enough.
> > 
> > OK, then since destruction may return an error already, is it fine?
> > Applications may call rte_flow_flush() (not supposed to fail unless there is
> > a serious issue, abort() in that case) and switch to SW fallback.
> 
> yes, it's fine.
> 
> > 
> 
> <...>
> 
> >>>>> + * @param[out] error
> >>>>> + *   Perform verbose error reporting if not NULL.
> >>>>> + *
> >>>>> + * @return
> >>>>> + *   0 on success, a negative errno value otherwise and rte_errno is set.
> >>>>> + */
> >>>>> +int
> >>>>> +rte_flow_query(uint8_t port_id,
> >>>>> +	       struct rte_flow *flow,
> >>>>> +	       enum rte_flow_action_type action,
> >>>>> +	       void *data,
> >>>>> +	       struct rte_flow_error *error);
> >>>>> +
> >>>>> +#ifdef __cplusplus
> >>>>> +}
> >>>>> +#endif
> >>>>
> >>>> I don't see a way to dump all the rules for a port out. I think this is
> >>>> neccessary for degbugging. You could have a look through dpif.h in OVS
> >>>> and see how dpif_flow_dump_next() is used, it might be a good reference.
> >>>
> >>> DPDK does not maintain flow rules and, depending on hardware capabilities
> >>> and level of compliance, PMDs do not necessarily do it either, particularly
> >>> since it requires space and application probably have a better method to
> >>> store these pointers for their own needs.
> >>
> >> understood
> >>
> >>>
> >>> What you see here is only a PMD interface. Depending on applications needs,
> >>> generic helper functions built on top of these may be added to manage flow
> >>> rules in the future.
> >>
> >> I'm thinking of the case where something goes wrong and I want to get a
> >> dump of all the flow rules from hardware, not query the rules I think I
> >> have. I don't see a way to do it or something to build a helper on top of?
> > 
> > Generic helper functions would exist on top of this API and would likely
> > maintain a list of flow rules themselves. The dump in that case would be
> > entirely implemented in software. I think that recovering flow rules from HW
> > may be complicated in many cases (even without taking storage allocation and
> > rules conversion issues into account), therefore if there is really a need
> > for it, we could perhaps add a dump() function that PMDs are free to
> > implement later.
> > 
> 
> ok. Maybe there are some more generic stats that can be got from the
> hardware that would help debugging that would suffice, like total flow
> rule hits/misses (i.e. not on a per flow rule basis).
> 
> You can get this from the software flow caches and it's widely used for
> debugging. e.g.
> 
> pmd thread numa_id 0 core_id 3:
> 	emc hits:0
> 	megaflow hits:0
> 	avg. subtable lookups per hit:0.00
> 	miss:0
> 

Perhaps a rule such as the following could do the trick:

 group: 42 (or priority 42)
 pattern: void
 actions: count / passthru

Assuming useful flow rules are defined with higher priorities (using lower
group ID or priority level) and provide a terminating action, this one would
count all packets that were not caught by them.

That is one example to illustrate how "global" counters can be requested by
applications.

Otherwise you could just make sure all rules contain mark / flag actions, in
which case mbufs would tell directly if they went through them or need
additional SW processing.

-- 
Adrien Mazarguil
6WIND