[PATCH v6 2/2] ethdev: add quota flow action and item

[Gregory Etelson]

Hello Andrew,

[]

> > +  Quota action sets packet metadata according to a number of remaining
> > +  tokens number:
> > +  * ``PASS`` - remaining tokens number is non-negative.
> > +  * ``BLOCK`` - remaining tokens number is negative.
> > +
> > +  Quota flow item matches on that metadata.
> > +
> > +  - ``RTE_FLOW_ACTION_TYPE_QUOTA``
> > +  - ``RTE_FLOW_ITEM_TYPE_QUOTA``
> 
> Quota description should be a part of action/item
> documentation, not release notes.
>

Will fix in v7.
 
> > +
> > +* **Updated testpmd to support quota flow action and item.**
> > +
> > +  Added tokens to ``token_list[]`` for flow quota action and item support.
> 
> IMHO 'token_list' is inapropriate in release notes. It is too
> deep technical detail.
> 

Will fix in v7.

> > +
> > +
> > +
> >   Removed Items
> >   -------------
> >
> 
> > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> > index 280567a3ae..0068fc275b 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -624,7 +624,45 @@ enum rte_flow_item_type {
> >        * See struct rte_flow_item_meter_color.
> >        */
> >       RTE_FLOW_ITEM_TYPE_METER_COLOR,
> > +
> > +     /**
> > +      * Match Quota state
> > +      *
> > +      * @see struct rte_flow_item_quota
> > +      */
> > +      RTE_FLOW_ITEM_TYPE_QUOTA,
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * QUOTA state.
> > + *
> > + * @see struct rte_flow_item_quota
> > + */
> > +enum rte_flow_quota_state {
> > +     RTE_FLOW_QUOTA_STATE_PASS, /** PASS quota state */
> > +     RTE_FLOW_QUOTA_STATE_BLOCK /** BLOCK quota state */
> 
> Enum memeber comments should start from /**< since it is a
> documentation after documented code.
>

Will fix in v7.
 
> > +};
> > +
> > +/**
> > + * RTE_FLOW_ITEM_TYPE_QUOTA
> > + *
> > + * Matches QUOTA state
> > + */
> > +struct rte_flow_item_quota {
> > +     enum rte_flow_quota_state state;
> > +};
> > +
> > +/**
> > + * Default mask for RTE_FLOW_ITEM_TYPE_QUOTA
> > + */
> > +#ifndef __cplusplus
> > +static const struct rte_flow_item_quota rte_flow_item_quota_mask = {
> > +     .state = (enum rte_flow_quota_state)0xff
> 
> I don't understand why it is just 0xff, not 0x1, not 0xf,
> not 0xffff, not 0xffffffff.
> 
> Isn't it better to make 'state' -> 'state_mask' and use
> PASS and BLOCK as corresponding bits in a mask. If so,
> here should be a value with all known bits set.
> 

Static default masks in rte_flow.h set all relevant bits.
That's the general cross-hardware convention.

> >   };
> > +#endif
> >
> >   /**
> >    *
> > @@ -2736,6 +2774,81 @@ enum rte_flow_action_type {
> >        * No associated configuration structure.
> >        */
> >       RTE_FLOW_ACTION_TYPE_SEND_TO_KERNEL,
> > +
> > +     /**
> > +      * Apply the quota verdict (PASS or BLOCK) to a flow.
> > +      *
> > +      * @see struct rte_flow_action_quota
> > +      * @see struct rte_flow_query_quota
> > +      * @see struct rte_flow_update_quota
> > +      */
> > +      RTE_FLOW_ACTION_TYPE_QUOTA,
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * QUOTA operational mode.
> > + *
> > + * @see struct rte_flow_action_quota
> > + */
> > +enum rte_flow_quota_mode {
> > +     RTE_FLOW_QUOTA_MODE_PACKET = 1, /** Count packets */
> 
> /**<
> 
> > +     RTE_FLOW_QUOTA_MODE_L2 = 2, /** Count packet bytes starting
> from L2 */
> 
> /**<
> 
> > +     RTE_FLOW_QUOTA_MODE_L3 = 3, /** Count packet bytes starting
> from L3 */
> 
> /**<
> 
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Create QUOTA action.
> > + *
> > + * @see RTE_FLOW_ACTION_TYPE_QUOTA
> > + */
> > +struct rte_flow_action_quota {
> > +     enum rte_flow_quota_mode mode; /** quota operational mode */
> 
> /**<
> 
> > +     int64_t quota;                 /** quota value */
> 
> /**<
> 
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Query indirect QUOTA action.
> > + *
> > + * @see RTE_FLOW_ACTION_TYPE_QUOTA
> > + *
> > + */
> > +struct rte_flow_query_quota {
> > +     int64_t quota; /** quota value */
> 
> /**<
> 
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Indirect QUOTA update operations.
> > + *
> > + * @see struct rte_flow_update_quota
> > + */
> > +enum rte_flow_update_quota_op {
> > +     RTE_FLOW_UPDATE_QUOTA_SET, /** set new quota value */
> 
> /**<
> 
> > +     RTE_FLOW_UPDATE_QUOTA_ADD, /** increase existing quota with
> new value */
> 
> /**<
> 
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * @see RTE_FLOW_ACTION_TYPE_QUOTA
> > + *
> > + * Update indirect QUOTA action.
> > + */
> > +struct rte_flow_update_quota {
> > +     enum rte_flow_update_quota_op op; /** update operation */
> 
> /**<
> 
> > +     int64_t quota;                    /** quota value */
> 
> /**<
> 
> >   };
> >
> >   /**
> > @@ -4854,6 +4967,11 @@ struct rte_flow_port_info {
> >        * @see RTE_FLOW_ACTION_TYPE_CONNTRACK
> >        */
> >       uint32_t max_nb_conn_tracks;
> > +     /**
> > +      * Maximum number of quota actions.
> > +      * @see RTE_FLOW_ACTION_TYPE_QUOTA
> > +      */
> > +     uint32_t max_nb_quotas;
> >       /**
> >        * Port supported flags (RTE_FLOW_PORT_FLAG_*).
> >        */
> > @@ -4932,6 +5050,11 @@ struct rte_flow_port_attr {
> >        * @see RTE_FLOW_ACTION_TYPE_CONNTRACK
> >        */
> >       uint32_t nb_conn_tracks;
> > +     /**
> > +      * Maximum number of quota actions.
> > +      * @see RTE_FLOW_ACTION_TYPE_QUOTA
> > +      */
> > +     uint32_t nb_quotas;
> >       /**
> >        * Port flags (RTE_FLOW_PORT_FLAG_*).
> >        */