[dpdk-dev] [PATCH] Allow -ve frame_overhead values

[dpdk-dev] [PATCH] Allow -ve frame_overhead values

[alanrobertsonatt]

From: Alan Robertson <alanrobertsonatt@gmail.com>

When forwarding traffic across a TDM network the ethernet header will
be replaced with a ML-PPP one thereby reducing the size of the packet.

Signed-off-by: Alan Robertson <alan.robertson@att.com>
---
 lib/librte_sched/rte_sched.c | 41 ++++++++++++++++++++++++++++++++++++-----
 lib/librte_sched/rte_sched.h |  2 +-
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index 7252f850d..5c88f1b62 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -216,7 +216,7 @@ struct rte_sched_port {
 	uint32_t n_pipes_per_subport;
 	uint32_t rate;
 	uint32_t mtu;
-	uint32_t frame_overhead;
+	int32_t frame_overhead;
 	uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
 	uint32_t n_pipe_profiles;
 	uint32_t pipe_tc3_rate_max;
@@ -643,7 +643,14 @@ rte_sched_port_config(struct rte_sched_port_params *params)
 	port->n_subports_per_port = params->n_subports_per_port;
 	port->n_pipes_per_subport = params->n_pipes_per_subport;
 	port->rate = params->rate;
-	port->mtu = params->mtu + params->frame_overhead;
+
+	/* Only add a +ve overhead.  A -ve overhead is to accommodate
+	 * for downstream TDM devices which won't have an ethernet header,
+	 * they obviously won't impact our local interface MTU size.
+	 */
+	port->mtu = params->mtu;
+	if (params->frame_overhead > 0)
+		port->mtu += params->frame_overhead;
 	port->frame_overhead = params->frame_overhead;
 	memcpy(port->qsize, params->qsize, sizeof(params->qsize));
 	port->n_pipe_profiles = params->n_pipe_profiles;
@@ -1613,13 +1620,21 @@ grinder_credits_check(struct rte_sched_port *port, uint32_t pos)
 	struct rte_sched_pipe *pipe = grinder->pipe;
 	struct rte_mbuf *pkt = grinder->pkt;
 	uint32_t tc_index = grinder->tc_index;
-	uint32_t pkt_len = pkt->pkt_len + port->frame_overhead;
+	uint32_t pkt_len;
+	int32_t tpkt_len;
 	uint32_t subport_tb_credits = subport->tb_credits;
 	uint32_t subport_tc_credits = subport->tc_credits[tc_index];
 	uint32_t pipe_tb_credits = pipe->tb_credits;
 	uint32_t pipe_tc_credits = pipe->tc_credits[tc_index];
 	int enough_credits;
 
+	/* Make sure we don't allow this to go -ve.  To accommodate
+	 * downstream TDM devices we may want to ignore the ethernet
+	 * header so allow -ve overhead values.
+	 */
+	tpkt_len = pkt->pkt_len + port->frame_overhead;
+	pkt_len = (tpkt_len < 0) ? 1 : tpkt_len;
+
 	/* Check queue credits */
 	enough_credits = (pkt_len <= subport_tb_credits) &&
 		(pkt_len <= subport_tc_credits) &&
@@ -1648,7 +1663,8 @@ grinder_credits_check(struct rte_sched_port *port, uint32_t pos)
 	struct rte_sched_pipe *pipe = grinder->pipe;
 	struct rte_mbuf *pkt = grinder->pkt;
 	uint32_t tc_index = grinder->tc_index;
-	uint32_t pkt_len = pkt->pkt_len + port->frame_overhead;
+	uint32_t pkt_len;
+	int32_t tpkt_len;
 	uint32_t subport_tb_credits = subport->tb_credits;
 	uint32_t subport_tc_credits = subport->tc_credits[tc_index];
 	uint32_t pipe_tb_credits = pipe->tb_credits;
@@ -1658,6 +1674,13 @@ grinder_credits_check(struct rte_sched_port *port, uint32_t pos)
 	uint32_t pipe_tc_ov_credits = pipe_tc_ov_mask1[tc_index];
 	int enough_credits;
 
+	/* Make sure we don't allow this to go -ve.  To accommodate
+	 * downstream TDM devices we may want to ignore the ethernet
+	 * header so allow -ve overhead values.
+	 */
+	tpkt_len = pkt->pkt_len + port->frame_overhead;
+	pkt_len = (tpkt_len < 0) ? 1 : tpkt_len;
+
 	/* Check pipe and subport credits */
 	enough_credits = (pkt_len <= subport_tb_credits) &&
 		(pkt_len <= subport_tc_credits) &&
@@ -1687,11 +1710,19 @@ grinder_schedule(struct rte_sched_port *port, uint32_t pos)
 	struct rte_sched_grinder *grinder = port->grinder + pos;
 	struct rte_sched_queue *queue = grinder->queue[grinder->qpos];
 	struct rte_mbuf *pkt = grinder->pkt;
-	uint32_t pkt_len = pkt->pkt_len + port->frame_overhead;
+	uint32_t pkt_len;
+	int32_t tpkt_len;
 
 	if (!grinder_credits_check(port, pos))
 		return 0;
 
+	/* Make sure we don't allow this to go -ve.  To accommodate
+	 * downstream TDM devices we may want to ignore the ethernet
+	 * header so allow -ve overhead values.
+	 */
+	tpkt_len = pkt->pkt_len + port->frame_overhead;
+	pkt_len = (tpkt_len < 0) ? 1 : tpkt_len;
+
 	/* Advance port time */
 	port->time += pkt_len;
 
diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
index e9c281726..63677eefc 100644
--- a/lib/librte_sched/rte_sched.h
+++ b/lib/librte_sched/rte_sched.h
@@ -219,7 +219,7 @@ struct rte_sched_port_params {
 	uint32_t mtu;                    /**< Maximum Ethernet frame size
 					  * (measured in bytes).
 					  * Should not include the framing overhead. */
-	uint32_t frame_overhead;         /**< Framing overhead per packet
+	int32_t frame_overhead;          /**< Framing overhead per packet
 					  * (measured in bytes) */
 	uint32_t n_subports_per_port;    /**< Number of subports */
 	uint32_t n_pipes_per_subport;    /**< Number of pipes per subport */
-- 
2.11.0

[dpdk-dev] [PATCH v2] Allow -ve frame_overhead values

[alanrobertsonatt]

From: Alan Robertson <ar771e@att.com>

When forwarding traffic across a TDM network the ethernet header will
be replaced with a ML-PPP one thereby reducing the size of the packet.

Signed-off-by: Alan Robertson <alan.robertson@att.com>
---
 lib/librte_sched/rte_sched.c | 14 +++++++++++++-
 lib/librte_sched/rte_sched.h |  5 +++--
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index ad2f7c6d5..c971fd0d4 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -187,7 +187,7 @@ struct rte_sched_port {
 	uint32_t n_pipes_per_subport;
 	uint32_t rate;
 	uint32_t mtu;
-	uint32_t frame_overhead;
+	int32_t frame_overhead;
 	uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
 	uint32_t n_pipe_profiles;
 	uint32_t pipe_tc3_rate_max;
@@ -1591,6 +1591,10 @@ grinder_credits_check(struct rte_sched_port *port, uint32_t pos)
 	uint32_t pipe_tc_credits = pipe->tc_credits[tc_index];
 	int enough_credits;
 
+#ifdef RTE_SCHED_DEBUG
+	assert((int)(pkt->pkt_len + port->frame_overhead) > 0);
+#endif /* RTE_SCHED_DEBUG */
+
 	/* Check queue credits */
 	enough_credits = (pkt_len <= subport_tb_credits) &&
 		(pkt_len <= subport_tc_credits) &&
@@ -1629,6 +1633,10 @@ grinder_credits_check(struct rte_sched_port *port, uint32_t pos)
 	uint32_t pipe_tc_ov_credits = pipe_tc_ov_mask1[tc_index];
 	int enough_credits;
 
+#ifdef RTE_SCHED_DEBUG
+	assert((int)(pkt->pkt_len + port->frame_overhead) > 0);
+#endif /* RTE_SCHED_DEBUG */
+
 	/* Check pipe and subport credits */
 	enough_credits = (pkt_len <= subport_tb_credits) &&
 		(pkt_len <= subport_tc_credits) &&
@@ -1663,6 +1671,10 @@ grinder_schedule(struct rte_sched_port *port, uint32_t pos)
 	if (!grinder_credits_check(port, pos))
 		return 0;
 
+#ifdef RTE_SCHED_DEBUG
+	assert((int)(pkt->pkt_len + port->frame_overhead) > 0);
+#endif /* RTE_SCHED_DEBUG */
+
 	/* Advance port time */
 	port->time += pkt_len;
 
diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
index 5d2a688dc..3e135c1e5 100644
--- a/lib/librte_sched/rte_sched.h
+++ b/lib/librte_sched/rte_sched.h
@@ -190,8 +190,9 @@ struct rte_sched_port_params {
 	uint32_t mtu;                    /**< Maximum Ethernet frame size
 					  * (measured in bytes).
 					  * Should not include the framing overhead. */
-	uint32_t frame_overhead;         /**< Framing overhead per packet
-					  * (measured in bytes) */
+	int32_t frame_overhead;
+	/**< Framing overhead per packet (measured in bytes).
+	 * Can have negative value. */
 	uint32_t n_subports_per_port;    /**< Number of subports */
 	uint32_t n_pipes_per_subport;    /**< Number of pipes per subport */
 	uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
-- 
2.11.0

Re: [dpdk-dev] [PATCH v2] Allow -ve frame_overhead values

[Dumitrescu, Cristian]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of
> alanrobertsonatt@gmail.com
> Sent: Tuesday, January 16, 2018 3:32 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; Alan Robertson <ar771e@att.com>; Alan Robertson
> <alan.robertson@att.com>
> Subject: [dpdk-dev] [PATCH v2] Allow -ve frame_overhead values
> 
> From: Alan Robertson <ar771e@att.com>
> 
> When forwarding traffic across a TDM network the ethernet header will
> be replaced with a ML-PPP one thereby reducing the size of the packet.
> 
> Signed-off-by: Alan Robertson <alan.robertson@att.com>
> ---
>  lib/librte_sched/rte_sched.c | 14 +++++++++++++-
>  lib/librte_sched/rte_sched.h |  5 +++--
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index ad2f7c6d5..c971fd0d4 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -187,7 +187,7 @@ struct rte_sched_port {
>  	uint32_t n_pipes_per_subport;
>  	uint32_t rate;
>  	uint32_t mtu;
> -	uint32_t frame_overhead;
> +	int32_t frame_overhead;
>  	uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
>  	uint32_t n_pipe_profiles;
>  	uint32_t pipe_tc3_rate_max;
> @@ -1591,6 +1591,10 @@ grinder_credits_check(struct rte_sched_port
> *port, uint32_t pos)
>  	uint32_t pipe_tc_credits = pipe->tc_credits[tc_index];
>  	int enough_credits;
> 
> +#ifdef RTE_SCHED_DEBUG
> +	assert((int)(pkt->pkt_len + port->frame_overhead) > 0);
> +#endif /* RTE_SCHED_DEBUG */
> +
>  	/* Check queue credits */
>  	enough_credits = (pkt_len <= subport_tb_credits) &&
>  		(pkt_len <= subport_tc_credits) &&
> @@ -1629,6 +1633,10 @@ grinder_credits_check(struct rte_sched_port
> *port, uint32_t pos)
>  	uint32_t pipe_tc_ov_credits = pipe_tc_ov_mask1[tc_index];
>  	int enough_credits;
> 
> +#ifdef RTE_SCHED_DEBUG
> +	assert((int)(pkt->pkt_len + port->frame_overhead) > 0);
> +#endif /* RTE_SCHED_DEBUG */
> +
>  	/* Check pipe and subport credits */
>  	enough_credits = (pkt_len <= subport_tb_credits) &&
>  		(pkt_len <= subport_tc_credits) &&
> @@ -1663,6 +1671,10 @@ grinder_schedule(struct rte_sched_port *port,
> uint32_t pos)
>  	if (!grinder_credits_check(port, pos))
>  		return 0;
> 
> +#ifdef RTE_SCHED_DEBUG
> +	assert((int)(pkt->pkt_len + port->frame_overhead) > 0);
> +#endif /* RTE_SCHED_DEBUG */
> +
>  	/* Advance port time */
>  	port->time += pkt_len;
> 
> diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
> index 5d2a688dc..3e135c1e5 100644
> --- a/lib/librte_sched/rte_sched.h
> +++ b/lib/librte_sched/rte_sched.h
> @@ -190,8 +190,9 @@ struct rte_sched_port_params {
>  	uint32_t mtu;                    /**< Maximum Ethernet frame size
>  					  * (measured in bytes).
>  					  * Should not include the framing
> overhead. */
> -	uint32_t frame_overhead;         /**< Framing overhead per packet
> -					  * (measured in bytes) */
> +	int32_t frame_overhead;
> +	/**< Framing overhead per packet (measured in bytes).
> +	 * Can have negative value. */
>  	uint32_t n_subports_per_port;    /**< Number of subports */
>  	uint32_t n_pipes_per_subport;    /**< Number of pipes per subport
> */
>  	uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> --
> 2.11.0
> 

Hi Alan,

This patch is straightforward now, but it has build failures, do you plan to send a v3 to fix them?

Essentially, you need to replace assert() calls from your code with RTE_ASSERT/RTE_VERIFY from rte_debug.h.

Also, I don't see the point in having the assert in grinder_credits_check(), as this is purely a check on packet length, not credits.  IMO we should have the assert in grinder_schedule() as the first instruction, before the call to grinder_credits_check(), as this is a sort of validating the inputs into the scheduling decision.

Thanks,
Cristian

Re: [dpdk-dev] [PATCH] Allow -ve frame_overhead values

[Dumitrescu, Cristian]

Hi Alan,

I agree, having a signed framing_overhead makes sense for several cases, including the TDM case you mention. In fact, this is the approach used by rte_tm.h.

Several issues to fix below.

> -----Original Message-----
> From: alanrobertsonatt@gmail.com [mailto:alanrobertsonatt@gmail.com]
> Sent: Monday, December 18, 2017 1:12 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; Alan Robertson <ar771e@att.com>; Alan Robertson
> <alan.robertson@att.com>
> Subject: [PATCH] Allow -ve frame_overhead values
> 
> From: Alan Robertson <ar771e@att.com>
> 
> When forwarding traffic across a TDM network the ethernet header will
> be replaced with a ML-PPP one thereby reducing the size of the packet.
> 
> Signed-off-by: Alan Robertson <alan.robertson@att.com>
> ---
>  lib/librte_sched/rte_sched.c | 41
> ++++++++++++++++++++++++++++++++++++-----
>  lib/librte_sched/rte_sched.h |  2 +-
>  2 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index 7252f850d..5c88f1b62 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -216,7 +216,7 @@ struct rte_sched_port {
>  	uint32_t n_pipes_per_subport;
>  	uint32_t rate;
>  	uint32_t mtu;
> -	uint32_t frame_overhead;
> +	int32_t frame_overhead;
>  	uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
>  	uint32_t n_pipe_profiles;
>  	uint32_t pipe_tc3_rate_max;
> @@ -643,7 +643,14 @@ rte_sched_port_config(struct
> rte_sched_port_params *params)
>  	port->n_subports_per_port = params->n_subports_per_port;
>  	port->n_pipes_per_subport = params->n_pipes_per_subport;
>  	port->rate = params->rate;
> -	port->mtu = params->mtu + params->frame_overhead;
> +
> +	/* Only add a +ve overhead.  A -ve overhead is to accommodate
> +	 * for downstream TDM devices which won't have an ethernet
> header,
> +	 * they obviously won't impact our local interface MTU size.
> +	 */

No need for this comment here. Also, we don't want to mention any specific protocols unless we want to create a cvasi-exhaustive list of protocols, which breaks the design guideline of having protocol agnostic scheduling library.

> +	port->mtu = params->mtu;
> +	if (params->frame_overhead > 0)
> +		port->mtu += params->frame_overhead;

This is not really correct, the MTU should always be:
	port->mtu = params->mtu + params->frame_overhead;

>  	port->frame_overhead = params->frame_overhead;
>  	memcpy(port->qsize, params->qsize, sizeof(params->qsize));
>  	port->n_pipe_profiles = params->n_pipe_profiles;
> @@ -1613,13 +1620,21 @@ grinder_credits_check(struct rte_sched_port
> *port, uint32_t pos)
>  	struct rte_sched_pipe *pipe = grinder->pipe;
>  	struct rte_mbuf *pkt = grinder->pkt;
>  	uint32_t tc_index = grinder->tc_index;
> -	uint32_t pkt_len = pkt->pkt_len + port->frame_overhead;
> +	uint32_t pkt_len;
> +	int32_t tpkt_len;

The variable tpkt_len is not really needed.

>  	uint32_t subport_tb_credits = subport->tb_credits;
>  	uint32_t subport_tc_credits = subport->tc_credits[tc_index];
>  	uint32_t pipe_tb_credits = pipe->tb_credits;
>  	uint32_t pipe_tc_credits = pipe->tc_credits[tc_index];
>  	int enough_credits;
> 
> +	/* Make sure we don't allow this to go -ve.  To accommodate
> +	 * downstream TDM devices we may want to ignore the ethernet
> +	 * header so allow -ve overhead values.
> +	 */

As stated above, no need for this comment.

> +	tpkt_len = pkt->pkt_len + port->frame_overhead;
> +	pkt_len = (tpkt_len < 0) ? 1 : tpkt_len;
> +

It has to be application responsibility to make sure this case does not happen, i.e. each input packet always contains the header(s) covered by the framing overhead value which are to be removed at some later point.

IMO this code is not correct, as it is more a cover-up than a fix, therefore it should be removed, so we always have:
	uint32_t pkt_len = pkt->pkt_len + port->frame_overhead;

To protect from misconfigurations, please replace this code with a debug assert/check under #ifdef RTE_SCHED_DEBUG.

>  	/* Check queue credits */
>  	enough_credits = (pkt_len <= subport_tb_credits) &&
>  		(pkt_len <= subport_tc_credits) &&
> @@ -1648,7 +1663,8 @@ grinder_credits_check(struct rte_sched_port
> *port, uint32_t pos)
>  	struct rte_sched_pipe *pipe = grinder->pipe;
>  	struct rte_mbuf *pkt = grinder->pkt;
>  	uint32_t tc_index = grinder->tc_index;
> -	uint32_t pkt_len = pkt->pkt_len + port->frame_overhead;
> +	uint32_t pkt_len;
> +	int32_t tpkt_len;
>  	uint32_t subport_tb_credits = subport->tb_credits;
>  	uint32_t subport_tc_credits = subport->tc_credits[tc_index];
>  	uint32_t pipe_tb_credits = pipe->tb_credits;
> @@ -1658,6 +1674,13 @@ grinder_credits_check(struct rte_sched_port
> *port, uint32_t pos)
>  	uint32_t pipe_tc_ov_credits = pipe_tc_ov_mask1[tc_index];
>  	int enough_credits;
> 
> +	/* Make sure we don't allow this to go -ve.  To accommodate
> +	 * downstream TDM devices we may want to ignore the ethernet
> +	 * header so allow -ve overhead values.
> +	 */
> +	tpkt_len = pkt->pkt_len + port->frame_overhead;
> +	pkt_len = (tpkt_len < 0) ? 1 : tpkt_len;
> +

Same comments from above apply here as well.

>  	/* Check pipe and subport credits */
>  	enough_credits = (pkt_len <= subport_tb_credits) &&
>  		(pkt_len <= subport_tc_credits) &&
> @@ -1687,11 +1710,19 @@ grinder_schedule(struct rte_sched_port *port,
> uint32_t pos)
>  	struct rte_sched_grinder *grinder = port->grinder + pos;
>  	struct rte_sched_queue *queue = grinder->queue[grinder->qpos];
>  	struct rte_mbuf *pkt = grinder->pkt;
> -	uint32_t pkt_len = pkt->pkt_len + port->frame_overhead;
> +	uint32_t pkt_len;
> +	int32_t tpkt_len;
> 
>  	if (!grinder_credits_check(port, pos))
>  		return 0;
> 
> +	/* Make sure we don't allow this to go -ve.  To accommodate
> +	 * downstream TDM devices we may want to ignore the ethernet
> +	 * header so allow -ve overhead values.
> +	 */
> +	tpkt_len = pkt->pkt_len + port->frame_overhead;
> +	pkt_len = (tpkt_len < 0) ? 1 : tpkt_len;
> +

Same here.

>  	/* Advance port time */
>  	port->time += pkt_len;
> 
> diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
> index e9c281726..63677eefc 100644
> --- a/lib/librte_sched/rte_sched.h
> +++ b/lib/librte_sched/rte_sched.h
> @@ -219,7 +219,7 @@ struct rte_sched_port_params {
>  	uint32_t mtu;                    /**< Maximum Ethernet frame size
>  					  * (measured in bytes).
>  					  * Should not include the framing
> overhead. */
> -	uint32_t frame_overhead;         /**< Framing overhead per packet
> +	int32_t frame_overhead;          /**< Framing overhead per packet
>  					  * (measured in bytes) */

I would augment the comment with:
	/**< Framing overhead per packet (measured in bytes). Can have negative value. */

>  	uint32_t n_subports_per_port;    /**< Number of subports */
>  	uint32_t n_pipes_per_subport;    /**< Number of pipes per subport
> */
> --
> 2.11.0

Regards,
Cristian