From mboxrd@z Thu Jan 1 00:00:00 1970 From: Prashant Bhole Subject: Re: [PATCH bpf-next 0/5] fix test_sockmap Date: Mon, 28 May 2018 13:16:24 +0900 Message-ID: References: <20180518071753.4768-1-bhole_prashant_q7@lab.ntt.co.jp> <9c41f494-fc95-e4d5-067f-e000eedc7d7f@gmail.com> <5e08aa3f-acfb-f6a9-e918-bca2393ea95f@lab.ntt.co.jp> <67df711c-75c0-b674-e394-148645353a5a@lab.ntt.co.jp> <85c79205-5bb6-f6c8-e4a1-abed059c2619@lab.ntt.co.jp> <28161781-9e02-bce4-501d-81dbbc24e1e8@gmail.com> <652cb724-b966-6ce3-7002-624e5e7f747e@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: "David S . Miller" , Shuah Khan , netdev@vger.kernel.org To: John Fastabend , Alexei Starovoitov , Daniel Borkmann Return-path: Received: from tama50.ecl.ntt.co.jp ([129.60.39.147]:48414 "EHLO tama50.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750710AbeE1ERp (ORCPT ); Mon, 28 May 2018 00:17:45 -0400 In-Reply-To: <652cb724-b966-6ce3-7002-624e5e7f747e@gmail.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 5/25/2018 11:01 PM, John Fastabend wrote: > On 05/25/2018 01:28 AM, Prashant Bhole wrote: >> >> >> On 5/24/2018 1:58 PM, John Fastabend wrote: >>> On 05/23/2018 09:47 PM, Prashant Bhole wrote: >>>> >>>> >>>> On 5/23/2018 6:44 PM, Prashant Bhole wrote: >>>>> >>>>> >>>>> On 5/22/2018 2:08 AM, John Fastabend wrote: >>>>>> On 05/20/2018 10:13 PM, Prashant Bhole wrote: >>>>>>> >>>>>>> >>>>>>> On 5/19/2018 1:42 AM, John Fastabend wrote: >>>>>>>> On 05/18/2018 12:17 AM, Prashant Bhole wrote: >>>>>>>>> This series fixes bugs in test_sockmap code. They weren't caught >>>>>>>>> previously because failure in RX/TX thread was not notified to the >>>>>>>>> main thread. >>>>>>>>> >>>>>>>>> Also fixed data verification logic and slightly improved test >>>>>>>>> output >>>>>>>>> such that parameters values (cork, apply, start, end) of failed >>>>>>>>> test >>>>>>>>> can be easily seen. >>>>>>>>> >>>>>>>> >>>>>>>> Great, this was on my list so thanks for taking care of it. >>>>>>>> >>>>>>>>> Note: Even after fixing above problems there are issues with tests >>>>>>>>> which set cork parameter. Tests fail (RX thread timeout) when cork >>>>>>>>> value is non-zero and overall data sent by TX thread isn't >>>>>>>>> multiples >>>>>>>>> of cork value. >>>>>>>> >>>>>>>> >>>>>>>> This is expected. When 'cork' is set the sender should only xmit >>>>>>>> the data when 'cork' bytes are available. If the user doesn't >>>>>>>> provide the N bytes the data is cork'ed waiting for the bytes and >>>>>>>> if the socket is closed the state is cleaned up. What these tests >>>>>>>> are testing is the cleanup path when a user doesn't provide the >>>>>>>> N bytes. In practice this is used to validate headers and prevent >>>>>>>> users from sending partial headers. We want to keep these tests >>>>>>>> because >>>>>>>> they verify a tear-down path in the code. >>>>>>> >>>>>>> Ok. >>>>>>> >>>>>>>> >>>>>>>> After your changes do these get reported as failures? If so we >>>>>>>> need to account for the above in the calculations. >>>>>>> >>>>>>> Yes, cork related test are reported as failures because of RX thread >>>>>>> timeout. >>>>>>> >>>>>>> So with your above description, I think we need to differentiate cork >>>>>>> tests with partial data and full data. In partial data test we can >>>>>>> have >>>>>>> something like "timeout_expected" flag. Any other way to fix it? >>>>>>> >>>>>> >>>>>> Adding a flag seems reasonable to me. Lets do this for now. Also I >>>>>> plan to add more negative tests so we can either use the same >>>>>> flag or a new one for those cases as well. >>>>>> >>>>> >>>>> John, >>>>> I worked on this for some time and noticed that the RX-timeout of >>>>> tests with cork parameter is dependent on various parameters. So we >>>>> can not set a flag like the way 'drop_expected' flag is set before >>>>> executing the test. >>>>> >>>>> So I decided to write a function which judges all parameters before >>>>> each test and decides whether a test with cork parameter will >>>>> timeout or not. Then the conditions in the function became >>>>> complicated. For example some tests fail if opt->rate < 17 (with >>>>> some other conditions). Here is 17 is related to FRAGS_PER_SKB. >>>>> Consider following two examples. >>>> I'm sorry. Correction: s/FRAGS_PER_SKB/MAX_SKB_FRAGS/ >>>> >>>>> >>>>> ./test_sockmap --cgroup /mnt/cgroup2 -r 16 -i 1 -l 30 -t sendpage >>>>> --txmsg --txmsg_cork 1024   # RX timeout occurs >>>>> >>>>> ./test_sockmap --cgroup /mnt/cgroup2 -r 17 -i 1 -l 30 -t sendpage >>>>> --txmsg --txmsg_cork 1024   # Success! >>>>> >>> >>> Ah yes this hits the buffer limit and flushes the queue. The kernel >>> side doesn't know how to merge those specific sendpage requests so >>> it gives each request its own buffer and when the limit is reached >>> we flush it. >>> >>>>> Do we need to keep such tests? if yes, then I will continue with >>>>> adding such conditions in the function. >>>>> >>> >>> Yes, these tests are needed because they are testing the edge cases. >>> These are probably the most important tests because my normal usage >>> will catch any issues in the "good" cases its these types of things >>> that can go unnoticed (at least for a short while) if we don't have >>> specific tests for them. >> >> I tried but it is difficult to come up with a right set of conditions >> which lead to test failure. >> > > Agreed, it can be yes. How about adding your logic for all tests except > "cork" cases. If there is a flag to set if the timeout is expected we > can always manually set it in the test invocation. Might not be as > nice as automatically learning the expected results but possibly easier > than building some complicated logic to figure it out. > > Would you mind submitting your series again without the "cork" tests > being tracked? And if you want add a bit to tell if the "cork" tests are > going to timeout or not setting it per test manually. But I think > your series can just omit the cork test for now and still be useful. Ok. I will submit the series again. Without any change in actual patches, but cover letter reorganized. Thanks. -Prashant