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] [day] [month] [year] [list]

Date:   Mon, 23 Mar 2020 00:28:01 +0000
From:   <ye.zh-yuan@...ionext.com>
To:     <vinicius.gomes@...el.com>, <netdev@...r.kernel.org>
CC:     <okamoto.satoru@...ionext.com>, <kojima.masahisa@...ionext.com>
Subject: RE: [PATCH net] net: cbs: Fix software cbs to consider packet

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@...el.com> 
Sent: Friday, March 20, 2020 2:10 AM
To: Ye, Zh-Yuan/葉 致遠 <ye.zh-yuan@...ionext.com>; netdev@...r.kernel.org
Cc: Okamoto, Satoru/岡本 諭 <okamoto.satoru@...ionext.com>; Kojima, Masahisa/小島 雅久 <kojima.masahisa@...ionext.com>; Ye, Zh-Yuan/葉 致遠 <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