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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87lfnwfeyw.fsf@intel.com>
Date:   Thu, 19 Mar 2020 10:10:15 -0700
From:   Vinicius Costa Gomes <vinicius.gomes@...el.com>
To:     Zh-yuan Ye <ye.zh-yuan@...ionext.com>, netdev@...r.kernel.org
Cc:     okamoto.satoru@...ionext.com, kojima.masahisa@...ionext.com,
        Zh-yuan Ye <ye.zh-yuan@...ionext.com>
Subject: Re: [PATCH net] net: cbs: Fix software cbs to consider packet

Hi,

Zh-yuan Ye <ye.zh-yuan@...ionext.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@...ionext.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