[v2,1/3] librte_pipeline: add support for DSCP action
Checks
Commit Message
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(+)
Comments
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
@@ -2119,6 +2119,87 @@ pkt4_work_decap(struct rte_mbuf *mbuf0,
mbuf3->pkt_len = pkt_len3 - n3;
}
+/**
+ * RTE_TABLE_ACTION_DSCP
+ */
+
+struct dscp_data {
+ uint8_t dscp_val;
+} __attribute__((__packed__));
+
+static int
+dscp_apply(void *data,
+ struct rte_table_action_dscp_params *p)
+{
+ struct dscp_data *d = data;
+
+ d->dscp_val = (p->dscp_val & 0x3f) << 2;
+
+ return 0;
+}
+
+static __rte_always_inline uint16_t
+dscp_ipv4_checksum_update(uint16_t cksum0,
+ uint8_t dscp_old,
+ uint8_t dscp_new)
+{
+ int32_t cksum1;
+ cksum1 = cksum0;
+ cksum1 = ~cksum1 & 0xFFFF;
+
+ /* Subtract ip0 (one's complement logic) */
+ cksum1 -= ((uint16_t)dscp_old & 0xFFFF);
+ cksum1 = (cksum1 & 0xFFFF) + (cksum1 >> 16);
+ cksum1 = (cksum1 & 0xFFFF) + (cksum1 >> 16);
+
+ /* Add ip1 (one's complement logic) */
+ cksum1 += ((uint16_t)dscp_new & 0xFFFF);
+ cksum1 = (cksum1 & 0xFFFF) + (cksum1 >> 16);
+ cksum1 = (cksum1 & 0xFFFF) + (cksum1 >> 16);
+
+ return (uint16_t)(~cksum1);
+}
+
+static __rte_always_inline void
+pkt_ipv4_work_dscp(struct ipv4_hdr *ip, struct dscp_data *data)
+{
+ uint16_t ip_cksum;
+ uint8_t dscp;
+
+ ip->hdr_checksum = ntohs(ip->hdr_checksum);
+
+ dscp = ip->type_of_service & 0x3;
+ dscp = ip->type_of_service | data->dscp_val;
+
+ ip_cksum = dscp_ipv4_checksum_update(ip->hdr_checksum,
+ ip->type_of_service, dscp);
+
+ ip->type_of_service = dscp;
+
+ ip_cksum = ntohs(ip_cksum);
+
+ ip->hdr_checksum = ip_cksum;
+}
+
+static __rte_always_inline void
+pkt_ipv6_work_dscp(struct ipv6_hdr *ip, struct dscp_data *data)
+{
+ ip->vtc_flow = rte_ntohl(ip->vtc_flow);
+
+ uint32_t dscp = data->dscp_val;
+
+ dscp = dscp << 20;
+
+ uint32_t vtc_flow;
+
+ vtc_flow = ip->vtc_flow & 0xF03FFFFF;
+ vtc_flow = ip->vtc_flow | dscp;
+
+ vtc_flow = rte_htonl(vtc_flow);
+
+ ip->vtc_flow = vtc_flow;
+}
+
/**
* Action profile
*/
@@ -2138,6 +2219,7 @@ action_valid(enum rte_table_action_type action)
case RTE_TABLE_ACTION_SYM_CRYPTO:
case RTE_TABLE_ACTION_TAG:
case RTE_TABLE_ACTION_DECAP:
+ case RTE_TABLE_ACTION_DSCP:
return 1;
default:
return 0;
@@ -2158,6 +2240,7 @@ struct ap_config {
struct rte_table_action_ttl_config ttl;
struct rte_table_action_stats_config stats;
struct rte_table_action_sym_crypto_config sym_crypto;
+ struct rte_table_action_dscp_config dscp;
};
static size_t
@@ -2180,6 +2263,8 @@ action_cfg_size(enum rte_table_action_type action)
return sizeof(struct rte_table_action_stats_config);
case RTE_TABLE_ACTION_SYM_CRYPTO:
return sizeof(struct rte_table_action_sym_crypto_config);
+ case RTE_TABLE_ACTION_DSCP:
+ return sizeof(struct rte_table_action_dscp_config);
default:
return 0;
}
@@ -2213,6 +2298,9 @@ action_cfg_get(struct ap_config *ap_config,
case RTE_TABLE_ACTION_SYM_CRYPTO:
return &ap_config->sym_crypto;
+
+ case RTE_TABLE_ACTION_DSCP:
+ return &ap_config->dscp;
default:
return NULL;
}
@@ -2278,6 +2366,9 @@ action_data_size(enum rte_table_action_type action,
case RTE_TABLE_ACTION_DECAP:
return sizeof(struct decap_data);
+ case RTE_TABLE_ACTION_DSCP:
+ return sizeof(struct dscp_data);
+
default:
return 0;
}
@@ -2543,6 +2634,10 @@ rte_table_action_apply(struct rte_table_action *action,
return decap_apply(action_data,
action_params);
+ case RTE_TABLE_ACTION_DSCP:
+ return dscp_apply(action_data,
+ action_params);
+
default:
return -EINVAL;
}
@@ -2939,6 +3034,17 @@ pkt_work(struct rte_mbuf *mbuf,
pkt_work_tag(mbuf, data);
}
+ if (cfg->action_mask & (1LLU << RTE_TABLE_ACTION_DSCP)) {
+ void *data =
+ action_data_get(table_entry, action,
+ RTE_TABLE_ACTION_DSCP);
+
+ if (cfg->common.ip_version)
+ pkt_ipv4_work_dscp(ip, data);
+ else
+ pkt_ipv6_work_dscp(ip, data);
+ }
+
return drop_mask;
}
@@ -3283,6 +3389,33 @@ pkt4_work(struct rte_mbuf **mbufs,
data0, data1, data2, data3);
}
+ if (cfg->action_mask & (1LLU << RTE_TABLE_ACTION_DSCP)) {
+ void *data0 =
+ action_data_get(table_entry0, action,
+ RTE_TABLE_ACTION_DSCP);
+ void *data1 =
+ action_data_get(table_entry1, action,
+ RTE_TABLE_ACTION_DSCP);
+ void *data2 =
+ action_data_get(table_entry2, action,
+ RTE_TABLE_ACTION_DSCP);
+ void *data3 =
+ action_data_get(table_entry3, action,
+ RTE_TABLE_ACTION_DSCP);
+
+ if (cfg->common.ip_version) {
+ pkt_ipv4_work_dscp(ip0, data0);
+ pkt_ipv4_work_dscp(ip1, data1);
+ pkt_ipv4_work_dscp(ip2, data2);
+ pkt_ipv4_work_dscp(ip3, data3);
+ } else {
+ pkt_ipv6_work_dscp(ip0, data0);
+ pkt_ipv6_work_dscp(ip1, data1);
+ pkt_ipv6_work_dscp(ip2, data2);
+ pkt_ipv6_work_dscp(ip3, data3);
+ }
+ }
+
return drop_mask0 |
(drop_mask1 << 1) |
(drop_mask2 << 2) |
@@ -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;
+
+};
+
+/** DSCP action parameters (per table rule). */
+struct rte_table_action_dscp_params {
+ /** dscp value to be set */
+ uint8_t dscp_val;
+
+};
+
/**
* Table action profile.
*/