On 04/01/2015 08:25 PM, Rosenboim, Leonid wrote:
Taras,
It is partly my fault, I did not realize the exact scope of the changes you
proposed,
and in some aspect I thought you may be proposing to get rid of the single-rule
support.
Here are some comments addressing your patch directly, hope this time I am
being helpful.
- Leo
Combine odp_pmr_t and odp_pmr_set_t into one type.
---
include/odp/api/classification.h | 58 ++++------------------------------------
1 file changed, 5 insertions(+), 53 deletions(-)
diff --git a/include/odp/api/classification.h b/include/odp/api/classification.h
index 97c872e..a03128c 100644
--- a/include/odp/api/classification.h
+++ b/include/odp/api/classification.h
@@ -185,7 +185,8 @@ int odp_cos_with_l3_qos(odp_pktio_t pktio_in,
/**
* @typedef odp_pmr_t
* PMR - Packet Matching Rule
- * Up to 32 bit of ternary matching of one of the available header fields
+ * Has one or more matching terms. Each term is up to 32 bit of ternary
+ * matching of one of the available header fields.
*/
I disagree. This type is intended to represent a *single" match rule that uses
a single 32-bit
ternary hardware resource.
So disagreement comes from the intention to map odp_pmr_t directly to a
HW resource? I think it is not a correct intention.
1. odp_pmr_t is an opaque handle of Packet Matching Rule and we do not
(and should not) specify how it is mapped to hardware. Especially we
should not say it is up to 32 bits. odp_pmr_term defines a rule type
and its length.
2. On many platforms there won't be 1:1 mapping between odp_pmr_t and
HW classifier rule. Let's take a case when the same odp_pmr_t is used
twice (attached to two Pktio interfaces or pktio and CoS). On TI
platform (as far as I know on Cavium too) this PMR will map to two
separate HW rules in classifier.
As long as a rule incorporates a single set of value and mask, the meaning of a
plurality of terms
is unclear at best.
Also, some platforms which are capable of matching one header field at a time,
may not be able
to match multiple header fields against a single value/mask at the same time.
On platforms that are capable of matching a single value/mask with multiple
header fields,
it creates an ambiguity regarding the action to be taken of only one of the
terms of a term set
matches the value/mask.
Each term has its own value/mask. Description doesn't say there is a
single value/mask set. It can be emphasized if needed.
/**
@@ -318,57 +319,20 @@ typedef struct odp_pmr_match_t {
} odp_pmr_match_t;
/**
- * @typedef odp_pmr_set_t
- * An opaque handle to a composite packet match rule-set
- */
-
In other words, a single-PMR and a PMR-set are two distinct abstractions,
and I disagree with the idea to combine them.
While they may be implemented differently at implementation level,
from user perspective they are both packet 'filters'. They have the same
usage, so no need to introduce a redundant abstraction.
-/**
* Create a composite packet match rule
*
* @param[in] num_terms Number of terms in the match rule.
* @param[in] terms Array of num_terms entries, one entry per
* term desired.
- * @param[out] pmr_set_id Returned handle to the composite rule set.
+ * @param[out] pmr_set Returned handle to the composite PMR.
*
* @return the number of terms elements
* that have been successfully mapped to the
* underlying platform classification engine
* @retval <0 on failure
*/
-int odp_pmr_match_set_create(int num_terms, odp_pmr_match_t *terms,
- odp_pmr_set_t *pmr_set_id);
-
-/**
- * Function to delete a composite packet match rule set
- * Depending on the implementation details, destroying a rule-set
- * may not guarantee the availability of hardware resources to create the
- * same or essentially similar rule-set.
- *
- * All of the resources pertaining to the match set associated with the
- * class-of-service will be released, but the class-of-service will
- * remain intact.
- *
- * @param[in] pmr_set_id A composite rule-set handle
- * returned when created.
- *
- * @retval 0 on success
- * @retval <0 on failure
- */
-int odp_pmr_match_set_destroy(odp_pmr_set_t pmr_set_id);
-
-/**
- * Apply a PMR Match Set to a pktio to assign a CoS.
- *
- * @param[in] pmr_set_id PMR match set to be activated
- * @param[in] src_pktio pktio to which this PMR match
- * set is to be applied
- * @param[in] dst_cos CoS to be assigned by this PMR match set
- *
- * @retval 0 on success
- * @retval <0 on failure
- */
-int odp_pktio_pmr_match_set_cos(odp_pmr_set_t pmr_set_id, odp_pktio_t
src_pktio,
- odp_cos_t dst_cos);
+int odp_pmr_set_create(int num_terms, odp_pmr_match_t *terms,
+ odp_pmr_t *pmr_set);
While I have confidence that several platforms should be capable supporting the
single-PMR
abstraction that I originally proposed, I have doubts that many platforms will
be able to implement
the PMR-set abstraction, and for this reason I prefer to keep them separate.
Of course if there is a platform that can support PMR-set, then it should be
able to implement the single-PMR
as well.
An opposite statement is also true: if platform supports single PMRs
and their chaining, then it should be able to support PMR-sets.
Because each PMR-set can be represented as cascaded single PMRs with
intermediate CoS queues pointing to 'drop' queue (from your example).
If platform has some limitations then you can hit them either by
cascading single PMRs or creating PMR-sets. So I don't see how a
separate abstraction for PMR-set helps here.
--
Taras Kondratiuk
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
