All of lore.kernel.org
 help / color / mirror / Atom feed
From: John Fastabend <john.fastabend@gmail.com>
To: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	"David S . Miller" <davem@davemloft.net>,
	Shuah Khan <shuah@kernel.org>,
	netdev@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH bpf v3 3/5] selftests/bpf: test_sockmap, fix test timeout
Date: Fri, 1 Jun 2018 07:03:07 -0700	[thread overview]
Message-ID: <f69d6775-b56f-2b47-21ce-51f9ef30d797@gmail.com> (raw)
In-Reply-To: <634dc36e-f299-631d-f501-d12453fa0b98@lab.ntt.co.jp>

On 05/30/2018 09:13 PM, Prashant Bhole wrote:
> 
> 
> On 5/31/2018 4:59 AM, John Fastabend wrote:
>> On 05/30/2018 12:29 PM, Alexei Starovoitov wrote:
>>> On Wed, May 30, 2018 at 02:56:09PM +0900, Prashant Bhole wrote:
>>>> In order to reduce runtime of tests, recently timout for select() call
>>>> was reduced from 1sec to 10usec. This was causing many tests failures.
>>>> It was caught with failure handling commits in this series.
>>>>
>>>> Restoring the timeout from 10usec to 1sec
>>>>
>>>> Fixes: a18fda1a62c3 ("bpf: reduce runtime of test_sockmap tests")
>>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>>>> ---
>>>>   tools/testing/selftests/bpf/test_sockmap.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/test_sockmap.c
>>>> b/tools/testing/selftests/bpf/test_sockmap.c
>>>> index 64f9e25c451f..9d01f5c2abe2 100644
>>>> --- a/tools/testing/selftests/bpf/test_sockmap.c
>>>> +++ b/tools/testing/selftests/bpf/test_sockmap.c
>>>> @@ -345,8 +345,8 @@ static int msg_loop(int fd, int iov_count, int
>>>> iov_length, int cnt,
>>>>           if (err < 0)
>>>>               perror("recv start time: ");
>>>>           while (s->bytes_recvd < total_bytes) {
>>>> -            timeout.tv_sec = 0;
>>>> -            timeout.tv_usec = 10;
>>>> +            timeout.tv_sec = 1;
>>>> +            timeout.tv_usec = 0;
>>>
>>> I've applied the set, but had to revert it, since it takes too long.
>>>
>>> real    1m40.124s
>>> user    0m0.375s
>>> sys    0m14.521s
>>>
>>
>> Dang, I thought it would be a bit longer but not minutes.
>>
>>> Myself and Daniel run the test semi-manually when we apply patches.>
>>> Adding 2 extra minutes of wait time is unnecessary.
>>
>> Yep.
>>
>>> Especially since most of it is idle time.
>>> Please find a way to fix tests differently.
>>> btw I don't see any failures today. Not sure what is being fixed
>>> by incresing a timeout.
>>>
>>
>> Calling these fixes is a bit much, they are primarily improvements.
>>
>> The background is, when I originally wrote the tests my goal was to
>> exercise the kernel code paths. Because of this I didn't really care if
>> the tests actually sent/recv all bytes in the test. (I have long
>> running tests using netperf/wrk/apached/etc. for that) But, the manual
>> tests do have an option to verify the data if specified. The 'verify'
>> option is a bit fragile in that with the right tests (e.g. drop)
>> or the certain options (e.g. cork) it can fail which is expected.
>>
>> What Prashant added was support to actually verify the data correctly.
>> And also fix a few cgroup handling and some pretty printing as well.
>> He noticed the low timeout causing issue in these cases though so
>> increased it.
>>
>> @Prashant, how about increasing this less dramatically because now
>> all cork tests are going to stall for 1s unless perfectly aligned.
>> How about 100us? Or even better we can conditionally set it based
>> on if tx_cork is set. If tx_cork is set use 1us otherwise use 200us
>> or something. (1s is really to high in any cases for lo)
>>
>> Also capturing some of the above in the cover letter would help
>> folks understand the context a bit better.
>>
> 
> I did trial and error for timeout values. Currently 1000us for corked
> tests and 1 sec for other tests works fine. I observed broken-pipe error
> at tx side when timeout was < 1000us.
> 
> Also tests with apply=1 and higher number of iterations were taking
> time, so reducing iterations reduces the test run time drastically.
> 

Yep, sending 1B at a time is slow.

> real    0m12.968s
> user    0m0.219s
> sys     0m14.337s
> 
> Also I will try to explain background in the cover letter of next series.
> 
Seems more reasonable to me now. Thanks.

> -Prashant
> 
> 

WARNING: multiple messages have this Message-ID
From: john.fastabend at gmail.com (John Fastabend)
Subject: [PATCH bpf v3 3/5] selftests/bpf: test_sockmap, fix test timeout
Date: Fri, 1 Jun 2018 07:03:07 -0700	[thread overview]
Message-ID: <f69d6775-b56f-2b47-21ce-51f9ef30d797@gmail.com> (raw)
In-Reply-To: <634dc36e-f299-631d-f501-d12453fa0b98@lab.ntt.co.jp>

On 05/30/2018 09:13 PM, Prashant Bhole wrote:
> 
> 
> On 5/31/2018 4:59 AM, John Fastabend wrote:
>> On 05/30/2018 12:29 PM, Alexei Starovoitov wrote:
>>> On Wed, May 30, 2018 at 02:56:09PM +0900, Prashant Bhole wrote:
>>>> In order to reduce runtime of tests, recently timout for select() call
>>>> was reduced from 1sec to 10usec. This was causing many tests failures.
>>>> It was caught with failure handling commits in this series.
>>>>
>>>> Restoring the timeout from 10usec to 1sec
>>>>
>>>> Fixes: a18fda1a62c3 ("bpf: reduce runtime of test_sockmap tests")
>>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7 at lab.ntt.co.jp>
>>>> ---
>>>>   tools/testing/selftests/bpf/test_sockmap.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/test_sockmap.c
>>>> b/tools/testing/selftests/bpf/test_sockmap.c
>>>> index 64f9e25c451f..9d01f5c2abe2 100644
>>>> --- a/tools/testing/selftests/bpf/test_sockmap.c
>>>> +++ b/tools/testing/selftests/bpf/test_sockmap.c
>>>> @@ -345,8 +345,8 @@ static int msg_loop(int fd, int iov_count, int
>>>> iov_length, int cnt,
>>>>           if (err < 0)
>>>>               perror("recv start time: ");
>>>>           while (s->bytes_recvd < total_bytes) {
>>>> -            timeout.tv_sec = 0;
>>>> -            timeout.tv_usec = 10;
>>>> +            timeout.tv_sec = 1;
>>>> +            timeout.tv_usec = 0;
>>>
>>> I've applied the set, but had to revert it, since it takes too long.
>>>
>>> real    1m40.124s
>>> user    0m0.375s
>>> sys    0m14.521s
>>>
>>
>> Dang, I thought it would be a bit longer but not minutes.
>>
>>> Myself and Daniel run the test semi-manually when we apply patches.>
>>> Adding 2 extra minutes of wait time is unnecessary.
>>
>> Yep.
>>
>>> Especially since most of it is idle time.
>>> Please find a way to fix tests differently.
>>> btw I don't see any failures today. Not sure what is being fixed
>>> by incresing a timeout.
>>>
>>
>> Calling these fixes is a bit much, they are primarily improvements.
>>
>> The background is, when I originally wrote the tests my goal was to
>> exercise the kernel code paths. Because of this I didn't really care if
>> the tests actually sent/recv all bytes in the test. (I have long
>> running tests using netperf/wrk/apached/etc. for that) But, the manual
>> tests do have an option to verify the data if specified. The 'verify'
>> option is a bit fragile in that with the right tests (e.g. drop)
>> or the certain options (e.g. cork) it can fail which is expected.
>>
>> What Prashant added was support to actually verify the data correctly.
>> And also fix a few cgroup handling and some pretty printing as well.
>> He noticed the low timeout causing issue in these cases though so
>> increased it.
>>
>> @Prashant, how about increasing this less dramatically because now
>> all cork tests are going to stall for 1s unless perfectly aligned.
>> How about 100us? Or even better we can conditionally set it based
>> on if tx_cork is set. If tx_cork is set use 1us otherwise use 200us
>> or something. (1s is really to high in any cases for lo)
>>
>> Also capturing some of the above in the cover letter would help
>> folks understand the context a bit better.
>>
> 
> I did trial and error for timeout values. Currently 1000us for corked
> tests and 1 sec for other tests works fine. I observed broken-pipe error
> at tx side when timeout was < 1000us.
> 
> Also tests with apply=1 and higher number of iterations were taking
> time, so reducing iterations reduces the test run time drastically.
> 

Yep, sending 1B at a time is slow.

> real    0m12.968s
> user    0m0.219s
> sys     0m14.337s
> 
> Also I will try to explain background in the cover letter of next series.
> 
Seems more reasonable to me now. Thanks.

> -Prashant
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARNING: multiple messages have this Message-ID
From: john.fastabend@gmail.com (John Fastabend)
Subject: [PATCH bpf v3 3/5] selftests/bpf: test_sockmap, fix test timeout
Date: Fri, 1 Jun 2018 07:03:07 -0700	[thread overview]
Message-ID: <f69d6775-b56f-2b47-21ce-51f9ef30d797@gmail.com> (raw)
Message-ID: <20180601140307.rUqVtOtMmruS0jnT3Qhb70gNMRNP5seSENusyLfBmjk@z> (raw)
In-Reply-To: <634dc36e-f299-631d-f501-d12453fa0b98@lab.ntt.co.jp>

On 05/30/2018 09:13 PM, Prashant Bhole wrote:
> 
> 
> On 5/31/2018 4:59 AM, John Fastabend wrote:
>> On 05/30/2018 12:29 PM, Alexei Starovoitov wrote:
>>> On Wed, May 30, 2018@02:56:09PM +0900, Prashant Bhole wrote:
>>>> In order to reduce runtime of tests, recently timout for select() call
>>>> was reduced from 1sec to 10usec. This was causing many tests failures.
>>>> It was caught with failure handling commits in this series.
>>>>
>>>> Restoring the timeout from 10usec to 1sec
>>>>
>>>> Fixes: a18fda1a62c3 ("bpf: reduce runtime of test_sockmap tests")
>>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7 at lab.ntt.co.jp>
>>>> ---
>>>>   tools/testing/selftests/bpf/test_sockmap.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/test_sockmap.c
>>>> b/tools/testing/selftests/bpf/test_sockmap.c
>>>> index 64f9e25c451f..9d01f5c2abe2 100644
>>>> --- a/tools/testing/selftests/bpf/test_sockmap.c
>>>> +++ b/tools/testing/selftests/bpf/test_sockmap.c
>>>> @@ -345,8 +345,8 @@ static int msg_loop(int fd, int iov_count, int
>>>> iov_length, int cnt,
>>>>           if (err < 0)
>>>>               perror("recv start time: ");
>>>>           while (s->bytes_recvd < total_bytes) {
>>>> -            timeout.tv_sec = 0;
>>>> -            timeout.tv_usec = 10;
>>>> +            timeout.tv_sec = 1;
>>>> +            timeout.tv_usec = 0;
>>>
>>> I've applied the set, but had to revert it, since it takes too long.
>>>
>>> real    1m40.124s
>>> user    0m0.375s
>>> sys    0m14.521s
>>>
>>
>> Dang, I thought it would be a bit longer but not minutes.
>>
>>> Myself and Daniel run the test semi-manually when we apply patches.>
>>> Adding 2 extra minutes of wait time is unnecessary.
>>
>> Yep.
>>
>>> Especially since most of it is idle time.
>>> Please find a way to fix tests differently.
>>> btw I don't see any failures today. Not sure what is being fixed
>>> by incresing a timeout.
>>>
>>
>> Calling these fixes is a bit much, they are primarily improvements.
>>
>> The background is, when I originally wrote the tests my goal was to
>> exercise the kernel code paths. Because of this I didn't really care if
>> the tests actually sent/recv all bytes in the test. (I have long
>> running tests using netperf/wrk/apached/etc. for that) But, the manual
>> tests do have an option to verify the data if specified. The 'verify'
>> option is a bit fragile in that with the right tests (e.g. drop)
>> or the certain options (e.g. cork) it can fail which is expected.
>>
>> What Prashant added was support to actually verify the data correctly.
>> And also fix a few cgroup handling and some pretty printing as well.
>> He noticed the low timeout causing issue in these cases though so
>> increased it.
>>
>> @Prashant, how about increasing this less dramatically because now
>> all cork tests are going to stall for 1s unless perfectly aligned.
>> How about 100us? Or even better we can conditionally set it based
>> on if tx_cork is set. If tx_cork is set use 1us otherwise use 200us
>> or something. (1s is really to high in any cases for lo)
>>
>> Also capturing some of the above in the cover letter would help
>> folks understand the context a bit better.
>>
> 
> I did trial and error for timeout values. Currently 1000us for corked
> tests and 1 sec for other tests works fine. I observed broken-pipe error
> at tx side when timeout was < 1000us.
> 
> Also tests with apply=1 and higher number of iterations were taking
> time, so reducing iterations reduces the test run time drastically.
> 

Yep, sending 1B at a time is slow.

> real    0m12.968s
> user    0m0.219s
> sys     0m14.337s
> 
> Also I will try to explain background in the cover letter of next series.
> 
Seems more reasonable to me now. Thanks.

> -Prashant
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  reply	other threads:[~2018-06-01 14:03 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-30  5:56 [PATCH bpf v3 0/5] fix test_sockmap Prashant Bhole
2018-05-30  5:56 ` Prashant Bhole
2018-05-30  5:56 ` bhole_prashant_q7
2018-05-30  5:56 ` [PATCH bpf v3 1/5] selftests/bpf: test_sockmap, check test failure Prashant Bhole
2018-05-30  5:56   ` Prashant Bhole
2018-05-30  5:56   ` bhole_prashant_q7
2018-05-30 13:26   ` John Fastabend
2018-05-30 13:26     ` John Fastabend
2018-05-30 13:26     ` john.fastabend
2018-05-30  5:56 ` [PATCH bpf v3 2/5] selftests/bpf: test_sockmap, join cgroup in selftest mode Prashant Bhole
2018-05-30  5:56   ` Prashant Bhole
2018-05-30  5:56   ` bhole_prashant_q7
2018-05-30  5:56 ` [PATCH bpf v3 3/5] selftests/bpf: test_sockmap, fix test timeout Prashant Bhole
2018-05-30  5:56   ` Prashant Bhole
2018-05-30  5:56   ` bhole_prashant_q7
2018-05-30 13:31   ` John Fastabend
2018-05-30 13:31     ` John Fastabend
2018-05-30 13:31     ` john.fastabend
2018-05-30 19:29   ` Alexei Starovoitov
2018-05-30 19:29     ` Alexei Starovoitov
2018-05-30 19:29     ` alexei.starovoitov
2018-05-30 19:59     ` John Fastabend
2018-05-30 19:59       ` John Fastabend
2018-05-30 19:59       ` john.fastabend
2018-05-31  4:13       ` Prashant Bhole
2018-05-31  4:13         ` Prashant Bhole
2018-05-31  4:13         ` bhole_prashant_q7
2018-06-01 14:03         ` John Fastabend [this message]
2018-06-01 14:03           ` John Fastabend
2018-06-01 14:03           ` john.fastabend
2018-05-30  5:56 ` [PATCH bpf v3 4/5] selftests/bpf: test_sockmap, fix data verification Prashant Bhole
2018-05-30  5:56   ` Prashant Bhole
2018-05-30  5:56   ` bhole_prashant_q7
2018-05-30  5:56 ` [PATCH bpf v3 5/5] selftests/bpf: test_sockmap, print additional test options Prashant Bhole
2018-05-30  5:56   ` Prashant Bhole
2018-05-30  5:56   ` bhole_prashant_q7
2018-05-30 13:32 ` [PATCH bpf v3 0/5] fix test_sockmap John Fastabend
2018-05-30 13:32   ` John Fastabend
2018-05-30 13:32   ` john.fastabend

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f69d6775-b56f-2b47-21ce-51f9ef30d797@gmail.com \
    --to=john.fastabend@gmail.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=ast@kernel.org \
    --cc=bhole_prashant_q7@lab.ntt.co.jp \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=shuah@kernel.org \
    --subject='Re: [PATCH bpf v3 3/5] selftests/bpf: test_sockmap, fix test timeout' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.