Re: [dpdk-dev] [PATCH v2 1/3] librte_pipeline: add support for DSCP action

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

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.dumitrescu@intel.com>; Sheehan,
> Georgina <georgina.sheehan@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.sheehan@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.sheehan@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