[PATCH net] net: cbs: Fix software cbs to consider packet

[PATCH net] net: cbs: Fix software cbs to consider packet

[Zh-yuan Ye]

Currently the software CBS does not consider the packet sending time
when depleting the credits. It caused the throughput to be
Idleslope[kbps] * (Port transmit rate[kbps] / |Sendslope[kbps]|) where
Idleslope * (Port transmit rate / (Idleslope + |Sendslope|)) is expected.
In order to fix the issue above, this patch takes the time when the
packet sending completes into account by moving the anchor time variable
"last" ahead to the send completion time upon transmission and adding
wait when the next dequeue request comes before the send completion time
of the previous packet.

Signed-off-by: Zh-yuan Ye <ye.zh-yuan@socionext.com>
---
 net/sched/sch_cbs.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
index b2905b03a432..a78b8a750bd9 100644
--- a/net/sched/sch_cbs.c
+++ b/net/sched/sch_cbs.c
@@ -71,6 +71,7 @@ struct cbs_sched_data {
 	int queue;
 	atomic64_t port_rate; /* in bytes/s */
 	s64 last; /* timestamp in ns */
+	s64 send_completed; /* timestamp in ns */
 	s64 credits; /* in bytes */
 	s32 locredit; /* in bytes */
 	s32 hicredit; /* in bytes */
@@ -181,6 +182,10 @@ static struct sk_buff *cbs_dequeue_soft(struct Qdisc *sch)
 	s64 credits;
 	int len;
 
+	if (now < q->send_completed) {
+		qdisc_watchdog_schedule_ns(&q->watchdog, q->send_completed);
+		return NULL;
+	}
 	if (q->credits < 0) {
 		credits = timediff_to_credits(now - q->last, q->idleslope);
 
@@ -192,7 +197,6 @@ static struct sk_buff *cbs_dequeue_soft(struct Qdisc *sch)
 
 			delay = delay_from_credits(q->credits, q->idleslope);
 			qdisc_watchdog_schedule_ns(&q->watchdog, now + delay);
-
 			q->last = now;
 
 			return NULL;
@@ -212,7 +216,9 @@ static struct sk_buff *cbs_dequeue_soft(struct Qdisc *sch)
 	credits += q->credits;
 
 	q->credits = max_t(s64, credits, q->locredit);
-	q->last = now;
+	q->send_completed = now + div64_s64(len * NSEC_PER_SEC,
+					    atomic64_read(&q->port_rate));
+	q->last = q->send_completed;
 
 	return skb;
 }
-- 
2.20.1

Re: [PATCH net] net: cbs: Fix software cbs to consider packet

[Vinicius Costa Gomes]

Hi,

Zh-yuan Ye <ye.zh-yuan@socionext.com> writes:

> Currently the software CBS does not consider the packet sending time
> when depleting the credits. It caused the throughput to be
> Idleslope[kbps] * (Port transmit rate[kbps] / |Sendslope[kbps]|) where
> Idleslope * (Port transmit rate / (Idleslope + |Sendslope|)) is expected.
> In order to fix the issue above, this patch takes the time when the
> packet sending completes into account by moving the anchor time variable
> "last" ahead to the send completion time upon transmission and adding
> wait when the next dequeue request comes before the send completion time
> of the previous packet.
>
> Signed-off-by: Zh-yuan Ye <ye.zh-yuan@socionext.com>
> ---

You raise good points here.

What I am thinking is that perhaps we could replace 'q->last' by this
'send_completed' idea, then we could have a more precise software mode
when we take into account when we dequeue the "last byte" of the packet.

>  net/sched/sch_cbs.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
> index b2905b03a432..a78b8a750bd9 100644
> --- a/net/sched/sch_cbs.c
> +++ b/net/sched/sch_cbs.c
> @@ -71,6 +71,7 @@ struct cbs_sched_data {
>  	int queue;
>  	atomic64_t port_rate; /* in bytes/s */
>  	s64 last; /* timestamp in ns */
> +	s64 send_completed; /* timestamp in ns */

So, my suggestion is to replace 'last' by the 'send_completed' concept.

And as an optional suggestion, I think that changing the 'last' name by
something like 'last_byte' with a comment saying "estimate of the
transmission of the last byte of the packet, in ns" could be worth
thinking about.

Do you see any problems?


Cheers,
-- 
Vinicius

RE: [PATCH net] net: cbs: Fix software cbs to consider packet

[ye.zh-yuan]

Hi Vinicius,

Thanks for your comment, 
I'll provide v2 patch as soon as possible.

BR
--
Ye
-----Original Message-----
From: Vinicius Costa Gomes <vinicius.gomes@intel.com> 
Sent: Friday, March 20, 2020 2:10 AM
To: Ye, Zh-Yuan/葉 致遠 <ye.zh-yuan@socionext.com>; netdev@vger.kernel.org
Cc: Okamoto, Satoru/岡本 諭 <okamoto.satoru@socionext.com>; Kojima, Masahisa/小島 雅久 <kojima.masahisa@socionext.com>; Ye, Zh-Yuan/葉 致遠 <ye.zh-yuan@socionext.com>
Subject: Re: [PATCH net] net: cbs: Fix software cbs to consider packet

Hi,

Zh-yuan Ye <ye.zh-yuan@socionext.com> writes:

> Currently the software CBS does not consider the packet sending time 
> when depleting the credits. It caused the throughput to be 
> Idleslope[kbps] * (Port transmit rate[kbps] / |Sendslope[kbps]|) where 
> Idleslope * (Port transmit rate / (Idleslope + |Sendslope|)) is expected.
> In order to fix the issue above, this patch takes the time when the 
> packet sending completes into account by moving the anchor time 
> variable "last" ahead to the send completion time upon transmission 
> and adding wait when the next dequeue request comes before the send 
> completion time of the previous packet.
>
> Signed-off-by: Zh-yuan Ye <ye.zh-yuan@socionext.com>
> ---

You raise good points here.

What I am thinking is that perhaps we could replace 'q->last' by this 'send_completed' idea, then we could have a more precise software mode when we take into account when we dequeue the "last byte" of the packet.

>  net/sched/sch_cbs.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c index 
> b2905b03a432..a78b8a750bd9 100644
> --- a/net/sched/sch_cbs.c
> +++ b/net/sched/sch_cbs.c
> @@ -71,6 +71,7 @@ struct cbs_sched_data {
>  	int queue;
>  	atomic64_t port_rate; /* in bytes/s */
>  	s64 last; /* timestamp in ns */
> +	s64 send_completed; /* timestamp in ns */

So, my suggestion is to replace 'last' by the 'send_completed' concept.

And as an optional suggestion, I think that changing the 'last' name by something like 'last_byte' with a comment saying "estimate of the transmission of the last byte of the packet, in ns" could be worth thinking about.

Do you see any problems?


Cheers,
--
Vinicius