Hi Georgina,
Thanks for your patch set and sorry for my delayed reply!
> -----Original Message-----
> From: Sheehan, Georgina
> Sent: Sunday, February 11, 2018 1:29 PM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian <cristian.dumitre...@intel.com>; Sheehan,
> Georgina <georgina.shee...@intel.com>
> Subject: [PATCH v2 1/3] librte_pipeline: add support for DSCP action
According to DPDK policy, the title of a library patch should remove the
"librte_" prefix from the library name, i.e. the title of this patch should be:
"pipeline: add support for DSCP action".
>
> From: Georgina Sheehan <georgina.shee...@intel.com>
>
> This allows the application to change the DSCP value of incoming packets
>
> v2: Added in call of function parse_table_action_dscp in softnic cli file
>
> Signed-off-by: Georgina Sheehan <georgina.shee...@intel.com>
> ---
> lib/librte_pipeline/rte_table_action.c | 133 +++++++++++++++++++++++++
> lib/librte_pipeline/rte_table_action.h | 20 ++++
> 2 files changed, 153 insertions(+)
>
<snip>
> diff --git a/lib/librte_pipeline/rte_table_action.h
> b/lib/librte_pipeline/rte_table_action.h
> index c96061291..28db53303 100644
> --- a/lib/librte_pipeline/rte_table_action.h
> +++ b/lib/librte_pipeline/rte_table_action.h
> @@ -102,6 +102,9 @@ enum rte_table_action_type {
>
> /** Packet decapsulations. */
> RTE_TABLE_ACTION_DECAP,
> +
> + /** Differentiated Services Code Point (DSCP) **/
> + RTE_TABLE_ACTION_DSCP,
> };
>
> /** Common action configuration (per table action profile). */
> @@ -794,6 +797,23 @@ struct rte_table_action_decap_params {
> uint16_t n;
> };
>
> +/**
> + * RTE_TABLE_ACTION_DSCP
> + */
> +/** DSCP action configuration (per table action profile). */
> +struct rte_table_action_dscp_config {
> + /** dscp length */
> + uint8_t dscp_len;
What exactly is DSCP length?
> +
> +};
> +
> +/** DSCP action parameters (per table rule). */
> +struct rte_table_action_dscp_params {
> + /** dscp value to be set */
> + uint8_t dscp_val;
> +
> +};
> +
> /**
> * Table action profile.
> */
> --
> 2.17.1
I have a fundamental issue with this approach for DSCP marking action:
1/ This proposal seems to simply read a single pre-configured DSCP value from
the table entry and apply it to all the packets that hit this table entry.
2/ To me, this approach is not really useful, as in practice different packets
that belong to the same flow can be tagged with different DSCP values on the
way out: for example, the flow can represent all the traffic for a given BNG
subscriber, which can include management data, voice, real time video, high
priority best effort, low priority best effort, etc packets.
DSCP is typically used by a specific provide to tag the traffic class and the
priority of the incoming packets within its network. Packets from the same user
can belong to different traffic classes and drop precedences, such as described
in e.g. RFC 2598 and related. Therefore, what I suggest is:
1/ Replacing the single pre-defined DSCP value per table entry with an array
defined per table (not per table entry).
2/ The reason to define this array per table as opposed to table entry is that
the DSCP code points are typically defined per network (all users) rather than
per user. Similar approach is already used by other existing actions, such as
metering and traffic management.
3/ The array is a 2D array indexed by the traffic class
(mbuf->hash.sched.traffic_class) and color/drop precedence
(mbuf->hash.sched.color) of the current packet (mbuf). The values are DSCP
values for different traffic classes and packet colors.
4/ The number of traffic classes (n_traffic_classes) should be a configuration
parameter for this action, therefore the size of the DSCP array is
n_traffic_classes x RTE_COLORS.
Makes sense?
Thanks,
Cristian
