New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

limit-rate: When the download speed is limited to 15M per second, it cannot be limited! #2386

Closed
liupeidong0620 opened this Issue Mar 15, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@liupeidong0620

liupeidong0620 commented Mar 15, 2018

I did this (Just for download speed test)

Download file size:

[root@california src]# du -sh dyninst-testsuite-9.3.1-1.el7.x86_64.rpm
17M	dyninst-testsuite-9.3.1-1.el7.x86_64.rpm

This post-submission test(#2371)

[root@california src]# ./curl -V
curl 7.59.0-DEV (x86_64-pc-linux-gnu) libcurl/7.59.0-DEV OpenSSL/1.0.2h
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS IPv6 Largefile NTLM NTLM_WB SSL TLS-SRP UnixSockets HTTPS-proxy

[root@california src]# time ./curl http://linux.mirrors.es.net/centos/7.4.1708/os/x86_64/Packages/dyninst-testsuite-9.3.1-1.el7.x86_64.rpm -O  --limit-rate 15M
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 16.2M  100 16.2M    0     0  48.5M      0 --:--:-- --:--:-- --:--:-- 48.5M

real	0m0.384s
user	0m0.046s
sys	0m0.117s

Real download time: real 0m0.384s(Test a few more times you will find!)

I expected the following

Correct performance

[root@california src]# curl -V
curl 7.35.0 (i686-pc-linux-gnu) libcurl/7.35.0 OpenSSL/1.0.2m zlib/1.2.7 c-ares/1.12.0 libidn/0.6.5
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smtp smtps telnet tftp
Features: AsynchDNS IDN Largefile NTLM NTLM_WB SSL libz TLS-SRP

[root@california src]# time curl http://linux.mirrors.es.net/centos/7.4.1708/os/x86_64/Packages/dyninst-testsuite-9.3.1-1.el7.x86_64.rpm -O  --limit-rate 15M
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 16.2M  100 16.2M    0     0  14.9M      0  0:00:01  0:00:01 --:--:-- 14.9M

real	0m1.123s
user	0m0.010s
sys	0m0.032s

Real download time: real 0m1.123s

Modify the code:

void Curl_pgrsSetDownloadCounter(struct Curl_easy *data, curl_off_t size)
{
  struct curltime now = Curl_now();

  data->progress.downloaded = size;

  /* download speed limit */
  if((data->set.max_recv_speed > 0) &&
     (Curl_pgrsLimitWaitTime(data->progress.downloaded,
                             data->progress.dl_limit_size,
                             data->set.max_recv_speed,
                             data->progress.dl_limit_start,
                             now) == 0)) {
    /* ====================== Added code  ==================*/
    if((size > 0) && \
       ((size - data->progress.dl_limit_size) < data->set.max_recv_speed)) {
       return;
    }
    /* ====================================================*/
    data->progress.dl_limit_start = now;
    data->progress.dl_limit_size = size;
  }
}

Limit frequent update parameters.what do you think?


void Curl_pgrsSetUploadCounter(struct Curl_easy *data, curl_off_t size)
Upload speed limit code should also be modified.)

execute the program:

[root@california src]# ./curl -V
curl 7.59.0-DEV (x86_64-pc-linux-gnu) libcurl/7.59.0-DEV OpenSSL/1.0.2h
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS IPv6 Largefile NTLM NTLM_WB SSL TLS-SRP UnixSockets HTTPS-proxy


[root@california src]# time ./curl http://linux.mirrors.es.net/centos/7.4.1708/os/x86_64/Packages/dyninst-testsuite-9.3.1-1.el7.x86_64.rpm -O  --limit-rate 15M
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 16.2M  100 16.2M    0     0  14.9M      0  0:00:01  0:00:01 --:--:-- 14.9M

real	0m1.136s
user	0m0.011s
sys	0m0.034s

Real download time: real 0m1.136s

operating system

Linux california 4.13.2-1.el7.elrepo.x86_64 #1 SMP Wed Sep 13 18:48:00 EDT 2017 x86_64 x86_64 x86_64 GNU/Linux
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 15, 2018

Member

I cannot reproduce with current master (7.59.0 release basically, in particular it includes commit 72a0f62) using a 17MB or 8GB file, downloaded from localhost.

Member

bagder commented Mar 15, 2018

I cannot reproduce with current master (7.59.0 release basically, in particular it includes commit 72a0f62) using a 17MB or 8GB file, downloaded from localhost.

@liupeidong0620

This comment has been minimized.

Show comment
Hide comment
@liupeidong0620

liupeidong0620 Mar 15, 2018

Repeat the test several times:

There is such a situation:

[root@california src]# ./curl -V
curl 7.59.0 (x86_64-pc-linux-gnu) libcurl/7.59.0 OpenSSL/1.0.2h
Release-Date: 2018-03-14
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS IPv6 Largefile NTLM NTLM_WB SSL TLS-SRP UnixSockets HTTPS-proxy

[root@california src]# time ./curl http://linux.mirrors.es.net/centos/7.4.1708/os/x86_64/Packages/dyninst-testsuite-9.3.1-1.el7.x86_64.rpm -O  --limit-rate 15M
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 16.2M  100 16.2M    0     0  52.4M      0 --:--:-- --:--:-- --:--:-- 52.4M

real	0m0.358s
user	0m0.087s
sys	0m0.063s

Repeat the test several times:

There is such a situation:

[root@california src]# ./curl -V
curl 7.59.0 (x86_64-pc-linux-gnu) libcurl/7.59.0 OpenSSL/1.0.2h
Release-Date: 2018-03-14
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS IPv6 Largefile NTLM NTLM_WB SSL TLS-SRP UnixSockets HTTPS-proxy

[root@california src]# time ./curl http://linux.mirrors.es.net/centos/7.4.1708/os/x86_64/Packages/dyninst-testsuite-9.3.1-1.el7.x86_64.rpm -O  --limit-rate 15M
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 16.2M  100 16.2M    0     0  52.4M      0 --:--:-- --:--:-- --:--:-- 52.4M

real	0m0.358s
user	0m0.087s
sys	0m0.063s
@liupeidong0620

This comment has been minimized.

Show comment
Hide comment
@liupeidong0620

liupeidong0620 Mar 15, 2018

You said I tested,Really cannot be reproduce. There is indeed this problem on the network

You said I tested,Really cannot be reproduce. There is indeed this problem on the network

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 15, 2018

Member

I think this happens because dl_limit_start is updated too often so it checks very very small pieces of the transfer against the rate and that's not very good. I think we should make sure that we always compare against a slightly longer time period, like perhaps a minimum of 3 seconds. PR coming.

Member

bagder commented Mar 15, 2018

I think this happens because dl_limit_start is updated too often so it checks very very small pieces of the transfer against the rate and that's not very good. I think we should make sure that we always compare against a slightly longer time period, like perhaps a minimum of 3 seconds. PR coming.

@bagder bagder closed this in f5700ea Mar 16, 2018

greearb added a commit to greearb/curl that referenced this issue Mar 19, 2018

rate-limit: use three second window to better handle high speeds
Due to very frequent updates of the rate limit "window", it could
attempt to rate limit within the same milliseconds and that then made
the calculations wrong, leading to it not behaving correctly on very
fast transfers.

This new logic updates the rate limit "window" to be no shorter than the
last three seconds and only updating the timestamps for this when
switching between the states TOOFAST/PERFORM.

Reported-by: 刘佩东
Fixes #2386
Closes #2388

@lock lock bot locked as resolved and limited conversation to collaborators Jun 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.