Re: [RFC] Generic flow director/filtering/classification API

["Chandran, Sugesh" <sugesh.chandran@intel.com>]

Hi Adrien,

Thank you for your response,
Please see my comments inline.

Regards
_Sugesh


> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Friday, July 8, 2016 2:03 PM
> To: Chandran, Sugesh <sugesh.chandran@intel.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas.monjalon@6wind.com>;
> Zhang, Helin <helin.zhang@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Rasesh Mody <rasesh.mody@qlogic.com>; Ajit
> Khaparde <ajit.khaparde@broadcom.com>; Rahul Lakkireddy
> <rahul.lakkireddy@chelsio.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> Jan Medala <jan@semihalf.com>; John Daley <johndale@cisco.com>; Chen,
> Jing D <jing.d.chen@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Matej Vido <matejvido@gmail.com>;
> Alejandro Lucero <alejandro.lucero@netronome.com>; Sony Chacko
> <sony.chacko@qlogic.com>; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Olga Shern <olgas@mellanox.com>
> Subject: Re: [dpdk-dev] [RFC] Generic flow director/filtering/classification
> API
> 
> Hi Sugesh,
> 
> On Thu, Jul 07, 2016 at 11:15:07PM +0000, Chandran, Sugesh wrote:
> > Hi Adrien,
> >
> > Thank you for proposing this. It would be really useful for application such
> as OVS-DPDK.
> > Please find my comments and questions inline below prefixed with
> [Sugesh]. Most of them are from the perspective of enabling these APIs in
> application such as OVS-DPDK.
> 
> Thanks, I'm replying below.
> 
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien
> Mazarguil
> > > Sent: Tuesday, July 5, 2016 7:17 PM
> > > To: dev@dpdk.org
> > > Cc: Thomas Monjalon <thomas.monjalon@6wind.com>; Zhang, Helin
> > > <helin.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Rasesh
> > > Mody <rasesh.mody@qlogic.com>; Ajit Khaparde
> > > <ajit.khaparde@broadcom.com>; Rahul Lakkireddy
> > > <rahul.lakkireddy@chelsio.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>;
> > > Jan Medala <jan@semihalf.com>; John Daley <johndale@cisco.com>;
> Chen,
> > > Jing D <jing.d.chen@intel.com>; Ananyev, Konstantin
> > > <konstantin.ananyev@intel.com>; Matej Vido <matejvido@gmail.com>;
> > > Alejandro Lucero <alejandro.lucero@netronome.com>; Sony Chacko
> > > <sony.chacko@qlogic.com>; Jerin Jacob
> > > <jerin.jacob@caviumnetworks.com>; De Lara Guarch, Pablo
> > > <pablo.de.lara.guarch@intel.com>; Olga Shern <olgas@mellanox.com>
> > > Subject: [dpdk-dev] [RFC] Generic flow director/filtering/classification
> API
> > >

<<<<<----Snipped out ---->>>>>
> > > Flow director
> > > -------------
> > >
> > > Flow director (FDIR) is the name of the most capable filter type, which
> > > covers most features offered by others. As such, it is the most
> widespread
> > > in PMDs that support filtering (i.e. all of them besides **e1000**).
> > >
> > > It is also the only type that allows an arbitrary 32 bits value provided by
> > > applications to be attached to a filter and returned with matching packets
> > > instead of relying on the destination queue to recognize flows.
> > >
> > > Unfortunately, even FDIR requires applications to be aware of low-level
> > > capabilities and limitations (most of which come directly from **ixgbe**
> and
> > > **i40e**):
> > >
> > > - Bitmasks are set globally per device (port?), not per filter.
> > [Sugesh] This means application cannot define filters that matches on
> arbitrary different offsets?
> > If that’s the case, I assume the application has to program bitmask in
> advance. Otherwise how
> > the API framework deduce this bitmask information from the rules?? Its
> not very clear to me
> > that how application pass down the bitmask information for multiple filters
> on same port?
> 
> This is my understanding of how flow director currently works, perhaps
> someome more familiar with it can answer this question better than I could.
> 
> Let me take an example, if particular device can only handle a single IPv4
> mask common to all flow rules (say only to match destination addresses),
> updating that mask to also match the source address affects all defined and
> future flow rules simultaneously.
> 
> That is how FDIR currently works and I think it is wrong, as it penalizes
> devices that do support individual bit-masks per rule, and is a little
> awkward from an application point of view.
> 
> What I suggest for the new API instead is the ability to specify one
> bit-mask per rule, and let the PMD deal with HW limitations by automatically
> configuring global bitmasks from the first added rule, then refusing to add
> subsequent rules if they specify a conflicting bit-mask. Existing rules
> remain unaffected that way, and applications do not have to be extra
> cautious.
> 
[Sugesh] The issue with that approach is, the hardware simply discards the rule
when it is a super set of first one eventhough the hardware is capable of 
handling it. How its guaranteed the first rule will set the bitmask for all the
subsequent rules. 
How about having a CLASSIFER_TYPE for the classifier. Every port can have 
set of supported flow types(for eg: L3_TYPE, L4_TYPE, L4_TYPE_8BYTE_FLEX,
L4_TYPE_16BYTE_FLEX) based on the underlying FDIR support. Application can query 
this and set the type accordingly while initializing the port. This way the first rule need 
not set all the bits that may needed in the future rules. 
 
> > > ``PASSTHRU``
> > > ^^^^^^^^^^^^
> > >
> > > Leaves packets up for additional processing by subsequent flow rules.
> This
> > > is the default when a rule does not contain a terminating action, but can
> be
> > > specified to force a rule to become non-terminating.
> > >
> > > - No configurable property.
> > >
> > > +---------------+
> > > | PASSTHRU      |
> > > +===============+
> > > | no properties |
> > > +---------------+
> > >
> > > Example to copy a packet to a queue and continue processing by
> subsequent
> > > flow rules:
> > [Sugesh] If a packet get copied to a queue, it’s a termination action.
> > How can its possible to do subsequent action after the packet already
> > moved to the queue. ?How it differs from DUP action?
> >  Am I missing anything here?
> 
> Devices may not support the combination of QUEUE + PASSTHRU (i.e. making
> QUEUE non-terminating). However these same devices may expose the
> ability to
> copy a packet to another (sniffer) queue all while keeping the rule
> terminating (QUEUE + DUP but no PASSTHRU).
> 
> DUP with two rules, assuming priorties and PASSTRHU are supported:
> 
> - pattern X, priority 0; actions: QUEUE 5, PASSTHRU (non-terminating)
> 
> - pattern X, priority 1; actions: QUEUE 6 (terminating)
> 
> DUP with two actions on a single rule and a single priority:
> 
> - pattern X, priority 0; actions: DUP 5, QUEUE 6 (terminating)
> 
> If supported, from an application point of view the end result is similar in
> both cases (note the second case may be implemented by the PMD using
> two HW
> rules internally).
> 
> However the second case does not waste a priority level and clearly states
> the intent to the PMD which is more likely to be supported. If HW supports
> DUP directly it is even faster since there is a single rule. That is why I
> thought having DUP as an action would be useful.
[Sugesh] Thank you for the clarification. It make sense to me now.
> 
> > > +--------------------------+
> > > | Copy to queue 8          |
> > > +==========+===============+
> > > | PASSTHRU |               |
> > > +----------+-----------+---+
> > > | QUEUE    | ``queue`` | 8 |
> > > +----------+-----------+---+
> > >
> > > ``ID``
> > > ^^^^^^
> > >
> > > Attaches a 32 bit value to packets.
> > >
> > > +----------------------------------------------+
> > > | ID                                           |
> > > +========+=====================================+
> > > | ``id`` | 32 bit value to return with packets |
> > > +--------+-------------------------------------+
> > >
> > [Sugesh] I assume the application has to program the flow
> > with a unique ID and matching packets are stamped with this ID
> > when reporting to the software. The uniqueness of ID is NOT
> > guaranteed by the API framework. Correct me if I am wrong here.
> 
> You are right, if the way I wrote it is not clear enough, I'm open to
> suggestions to improve it.
[Sugesh] I guess its fine and would like to confirm the same. Perhaps
it would be nice to mention that the IDs are application defined.

> 
> > [Sugesh] Is it a limitation to use only 32 bit ID? Is it possible to have a
> > 64 bit ID? So that application can use the control plane flow pointer
> > Itself as an ID. Does it make sense?
> 
> I've specified a 32 bit ID for now because this is what FDIR supports and
> also what existing devices can report today AFAIK (i40e and mlx5).
> 
> We could use 64 bit for future-proofness in a separate action like "ID64"
> when at least one device supports it.
> 
> To PMD maintainers: please comment if you know devices that support
> tagging
> matching packets with more than 32 bits of user-provided data!
[Sugesh] I guess the flow director ID is 64 bit , The XL710 datasheet says so.
And in the 'rte_mbuf' structure the 64 bit FDIR-ID is shared with rss hash. This can be
a software driver limitation that expose only 32 bit. Possibly because of cache 
alignment issues? Since the hardware can support 64 bit, I feel it make sense 
to support 64 bit as well.
> 
> > > .. raw:: pdf
> > >
> > >    PageBreak
> > >
> > > ``QUEUE``
> > > ^^^^^^^^^
> > >
> > > Assigns packets to a given queue index.
> > >
> > > - Terminating by default.
> > >
> > > +--------------------------------+
> > > | QUEUE                          |
> > > +===========+====================+
> > > | ``queue`` | queue index to use |
> > > +-----------+--------------------+
> > >
> > > ``DROP``
> > > ^^^^^^^^
> > >
> > > Drop packets.
> > >
> > > - No configurable property.
> > > - Terminating by default.
> > > - PASSTHRU overrides this action if both are specified.
> > >
> > > +---------------+
> > > | DROP          |
> > > +===============+
> > > | no properties |
> > > +---------------+
> > >
> > > ``COUNT``
> > > ^^^^^^^^^
> > >
> > [Sugesh] Should we really have to set count action explicitly for every rule?
> > IMHO it would be great to be an implicit action. Most of the application
> would be
> > interested in the stats of almost all the filters/flows .
> 
> I can see why, but no, it must be explicitly requested because you may want
> to know in advance when it is not supported. Also considering it is
> something else to be done by HW (a separate action), we can assume
> enabling
> this may slow things down a bit.
> 
> HW limitations may also prevent you from having as many flow counters as
> you
> want, in which case you probably want to carefully pick which rules have
> them.
> 
> I think this target is most useful with DROP, VF and PF actions since
> those are currently the only ones where SW may not see the related
> packets.
> 
[Sugesh] Agreed and thanks for the clarification.

> > > Enables hits counter for this rule.
> > >
> > > This counter can be retrieved and reset through ``rte_flow_query()``, see
> > > ``struct rte_flow_query_count``.
> > >
> > > - Counters can be retrieved with ``rte_flow_query()``.
> > > - No configurable property.
> > >
> > > +---------------+
> > > | COUNT         |
> > > +===============+
> > > | no properties |
> > > +---------------+
> > >
> > > Query structure to retrieve and reset the flow rule hits counter:
> > >
> > > +------------------------------------------------+
> > > | COUNT query                                    |
> > > +===========+=====+==============================+
> > > | ``reset`` | in  | reset counter after query    |
> > > +-----------+-----+------------------------------+
> > > | ``hits``  | out | number of hits for this flow |
> > > +-----------+-----+------------------------------+
> > >
<<<<<<<<Snipped out >>>>
> > > ::
> > >
> > >  struct rte_flow *
> > >  rte_flow_create(uint8_t port_id,
> > >                  const struct rte_flow_pattern *pattern,
> > >                  const struct rte_flow_actions *actions);
> > >
> > > Arguments:
> > >
> > > - ``port_id``: port identifier of Ethernet device.
> > > - ``pattern``: pattern specification to add.
> > > - ``actions``: actions associated with the flow definition.
> > >
> > > Return value:
> > >
> > > A valid flow pointer in case of success, NULL otherwise and ``rte_errno`` is
> > > set to the positive version of one of the error codes defined for
> > > ``rte_flow_validate()``.
> > [Sugesh] : Kind of implementation specific query. What if application
> > try to add duplicate rules? Does the API create new flow entry for every
> > API call?
> 
> If an application adds duplicate rules at a given priority level, the second
> one may return an error depending on the PMD. Collisions are sometimes
> trivial to detect (such as the same pattern twice), others not so much (one
> matching an Ethernet header only, the other one matching an IP header
> only).
> 
> Either way if a packet is matched by two rules at a given priority level,
> what happens is described in 3.3 (High level design) and 4.4.1 (Priorities).
> 
> Applications are responsible for not relying on the PMD to detect these, or
> should use a single priority level for each rule to make things clear.
> 
> However since the number of HW priority levels is finite and possibly small,
> they must also make sure not to waste them. My advice is to only use
> priority levels when it cannot be proven that rules do not collide.
> 
> If all you have is perfect matching rules without wildcards and all of them
> match the same number of layers, a single priority level is fine.
> 
[Sugesh] Make sense. Its fine from my prespective.
> > [Sugesh] Another concern is the cost and time of installing these rules
> > in the hardware. Can we make these APIs time bound(or at least an option
> to
> > set the time limit to execute these APIs), so that
> > Application doesn’t have to wait so long when installing and deleting flows
> with
> > slow hardware/NIC. What do you think? Most of the datapath flow
> installations are
> > dynamic and triggered only when there is
> > an ingress traffic. Delay in flow insertion/deletion have unpredictable
> consequences.
> 
> This API is (currently) aimed at the control path only, and must indeed be
> assumed to be slow. Creating million of rules may take quite long as it may
> involve syscalls and other time-consuming synchronization things on the
> PMD
> side.
> 
> So currently there is no plan to have rules added from the data path with
> time constraints. I think it would be implemented through a different set of
> functions anyway.
> 
> I do not think adding time limits is practical, even specifying in the API
> that creating a single flow rule must take less than a maximum number of
> seconds in order to be effective is too much of a constraint (applications
> that create all flows during init may not care after all).
> 
> You should consider in any case that modifying flow rules will always be
> slower than receiving packets, there is no way around that. Applications
> have to live with it and provide a software fallback for incoming packets
> while managing flow rules.
> 
> Moreover, think about what happens when you hit the maximum number of
> flow
> rules and cannot create any more. Applications need to implement some
> kind
> of fallback in their data path.
> 
> Offloading flows in HW is also only useful if they live much longer than the
> time taken to create and delete them. Perhaps applications may choose to
> do
> so after detecting long lived flows such as TCP sessions.
> 
> You may have one separate control thread dedicated to manage flows and
> keep your normal control thread unaffected by delays. Several threads can
> even be dedicated, one per device.
[Sugesh] I agree that the flow insertion cannot be as fast as the packet receiving 
rate.  From application point of view the problem will be when hardware flow 
insertion takes longer than software flow insertion. At least application has to know
the cost of inserting/deleting a rule in hardware beforehand. Otherwise how application
can choose the right flow candidate for hardware. My point here is application is expecting 
a deterministic behavior from a classifier while inserting and deleting rules.
> 
> > [Sugesh] Another query is on the synchronization part. What if same rules
> are
> > handled from different threads? Is application responsible for handling the
> concurrent
> > hardware programming?
> 
> Like most (if not all) DPDK APIs, applications are responsible for managing
> locking issues as decribed in 4.3 (Behavior). Since this is a control path
> API and applications usually have a single control thread, locking should
> not be necessary in most cases.
> 
> Regarding my above comment about using several control threads to
> manage
> different devices, section 4.3 says:
> 
>  "There is no provision for reentrancy/multi-thread safety, although nothing
>  should prevent different devices from being configured at the same
>  time. PMDs may protect their control path functions accordingly."
> 
> I'd like to emphasize it is not "per port" but "per device", since in a few
> cases a configurable resource is shared by several ports. It may be
> difficult for applications to determine which ports are shared by a given
> device but this falls outside the scope of this API.
> 
> Do you think adding the guarantee that it is always safe to configure two
> different ports simultaneously without locking from the application side is
> necessary? In which case the PMD would be responsible for locking shared
> resources.
[Sugesh] This would be little bit complicated when some of ports are not under 
DPDK itself(what if one port is managed by Kernel) Or ports are tied by 
different application. Locking in PMD helps when the ports are accessed by 
multiple DPDK application. However what if the port itself not under DPDK?
> 
> > > Destruction
> > > ~~~~~~~~~~~
> > >
> > > Flow rules destruction is not automatic, and a queue should not be
> released
> > > if any are still attached to it. Applications must take care of performing
> > > this step before releasing resources.
> > >
> > > ::
> > >
> > >  int
> > >  rte_flow_destroy(uint8_t port_id,
> > >                   struct rte_flow *flow);
> > >
> > >
> > [Sugesh] I would suggest having a clean-up API is really useful as the
> releasing of
> > Queue(is it applicable for releasing of port too?) is not guaranteeing the
> automatic flow
> > destruction.
> 
> Would something like rte_flow_flush(port_id) do the trick? I wanted to
> emphasize in this first draft that applications should really keep the flow
> pointers around in order to manage/destroy them. It is their responsibility,
> not PMD's.
[Sugesh] Thanks, I think the flush call will do.
> 
> > This way application can initialize the port,
> > clean-up all the existing rules and create new rules  on a clean slate.
> 
> No resource can be released as long as a flow rule is using it (bad things
> may happen otherwise), all flow rules must be destroyed first, thus none can
> possibly remain after initializing a port. It is assumed that PMDs do
> automatic clean up during init if necessary to ensure this.
[Sugesh] That will do.
> 
> > > Failure to destroy a flow rule 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 flow rules are destroyed in
> > > reverse order of their creation.
> > >
> > > Arguments:
> > >
> > > - ``port_id``: port identifier of Ethernet device.
> > > - ``flow``: flow rule to destroy.
> > >
> > > Return value:
> > >
> > > - **0** on success, a negative errno value otherwise and ``rte_errno`` is
> > >   set.
> > >
> > > .. raw:: pdf
> > >
> > >    PageBreak
> > >
> > > Query
> > > ~~~~~
> > >
> > > Query an existing flow rule.
> > >
> > > This function allows retrieving flow-specific data such as counters. Data
> > > is gathered by special actions which must be present in the flow rule
> > > definition.
> > >
> > > ::
> > >
> > >  int
> > >  rte_flow_query(uint8_t port_id,
> > >                 struct rte_flow *flow,
> > >                 enum rte_flow_action_type action,
> > >                 void *data);
> > >
> > > Arguments:
> > >
> > > - ``port_id``: port identifier of Ethernet device.
> > > - ``flow``: flow rule to query.
> > > - ``action``: action type to query.
> > > - ``data``: pointer to storage for the associated query data type.
> > >
> > > Return value:
> > >
> > > - **0** on success, a negative errno value otherwise and ``rte_errno`` is
> > >   set.
> > >
> > > .. raw:: pdf
> > >
> > >    PageBreak
> > >
> > > Behavior
> > > --------
> > >
> > > - API operations are synchronous and blocking (``EAGAIN`` cannot be
> > >   returned).
> > >
> > > - There is no provision for reentrancy/multi-thread safety, although
> nothing
> > >   should prevent different devices from being configured at the same
> > >   time. PMDs may protect their control path functions accordingly.
> > >
> > > - Stopping the data path (TX/RX) should not be necessary when managing
> > > flow
> > >   rules. If this cannot be achieved naturally or with workarounds (such as
> > >   temporarily replacing the burst function pointers), an appropriate error
> > >   code must be returned (``EBUSY``).
> > >
> > > - PMDs, not applications, are responsible for maintaining flow rules
> > >   configuration when stopping and restarting a port or performing other
> > >   actions which may affect them. They can only be destroyed explicitly.
> > >
> > > .. raw:: pdf
> > >
> > >    PageBreak
> > >
> > [Sugesh] Query all the rules for a specific port/queue?? Useful when
> adding and
> > deleting ports and queues dynamically according to the need. I am not sure
> > what are the other  different usecases for these APIs. But I feel it makes
> much easier to
> > manage flows from the application. What do you think?
> 
> Not sure, that seems to fall out of the scope of this API. As described,
> applications already store the related rte_flow pointers. Accordingly, they
> know how many rules are associated to a given port. They need both a port
> ID
> and a flow rule pointer to destroy them after all.
> 
> Now perhaps something to convert back an existing rte_flow to a pattern
> and
> a list of actions, however I cannot see an immediate use case for it.
> 
> What you describe seems to be doable through a front-end API, I think
> keeping this one as low-level as possible with only basic actions is better
> right now. I'll keep your suggestion in mind.
[Sugesh] Sure, That will be fine.
> 
> > > Compatibility
> > > -------------
> > >
> > > No known hardware implementation supports all the features described
> in
> > > this
> > > document.
> > >
> > > Unsupported features or combinations are not expected to be fully
> > > emulated
> > > in software by PMDs for performance reasons. Partially supported
> features
> > > may be completed in software as long as hardware performs most of the
> > > work
> > > (such as queue redirection and packet recognition).
> > >
> > > However PMDs are expected to do their best to satisfy application
> requests
> > > by working around hardware limitations as long as doing so does not
> affect
> > > the behavior of existing flow rules.
> > >
> > > The following sections provide a few examples of such cases, they are
> based
> Adrien Mazarguil
> 6WIND