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

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

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