[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/4] Introduce new OOM testing support



On 09/25/2013 11:23 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> The previous OOM testing support would re-run the entire "main"
> method each iteration, failing a different malloc each time.
> When a test suite has 'n' allocations, the number of repeats
> requires is  (n * (n + 1) ) / 2.  This gets very large, very
> quickly.
> 
> This new OOM testing support instead integrates at the
> virtTestRun level, so each individual test case gets repeated,
> instead of the entire test suite. This means the values of
> 'n' are orders of magnitude smaller.

Still O(n^2) - but yes, a smaller n makes for a NOTICEABLE speedup [that
is, 30 alloc's per test on a main() that runs 30 tests is much nicer
than 900 alloc's per main()] :)

> 
> The simple usage is
> 
>    $ VIR_TEST_OOM=1 ./qemuxml2argvtest
>    ...
>    29) QEMU XML-2-ARGV clock-utc                                         ... OK
>        Test OOM for nalloc=36 .................................... OK
>    30) QEMU XML-2-ARGV clock-localtime                                   ... OK
>        Test OOM for nalloc=36 .................................... OK
>    31) QEMU XML-2-ARGV clock-france                                      ... OK
>        Test OOM for nalloc=38 ...................................... OK
>    ...
> 
> the second lines reports how many mallocs have to be failed, and thus
> how many repeats of the test wil be run.

s/wil/will/

> 
> If it crashes, then running under valgrind will often show the problem
> 
>   $ VIR_TEST_OOM=1 ../run valgrind ./qemuxml2argvtest
> 
> When debugging problems it is also helpful to select an individual
> test case
> 
>   $ VIR_TEST_RANGE=30 VIR_TEST_OOM=1 ../run valgrind ./qemuxml2argvtest
> 
> When things get really tricky, it is possible to request that just
> specific allocs are failed. eg to fail allocs 5 -> 12, use
> 
>   $ VIR_TEST_RANGE=30 VIR_TEST_OOM=1:5-12 ../run valgrind ./qemuxml2argvtest

Please document VIR_TEST_OOM in docs/hacking.html.in (and thus HACKING).
 Does it require building with './configure --enable-test-oom'?

> 
> In the worse case, you might want to know the stack trace of the
> alloc which was failed then VIR_TEST_OOM_TRACE can be set. If it
> is set to 1 then it will only print if it things a mistake happened.

s/things/thinks/

> This is often not reliable, so setting it to 2 will make it print
> the stack trace for every alloc that is failed.
> 
>   $ VIR_TEST_OOM_TRACE=2 VIR_TEST_RANGE=30 VIR_TEST_OOM=1:5-5 ../run valgrind ./qemuxml2argvtest
>   30) QEMU XML-2-ARGV clock-localtime                                   ... OK
>       Test OOM for nalloc=36 !virAllocN
>   /home/berrange/src/virt/libvirt/src/util/viralloc.c:180
>   virHashCreateFull
>   /home/berrange/src/virt/libvirt/src/util/virhash.c:144
>   virDomainDefParseXML
>   /home/berrange/src/virt/libvirt/src/conf/domain_conf.c:11745
>   virDomainDefParseNode
>   /home/berrange/src/virt/libvirt/src/conf/domain_conf.c:12646
>   virDomainDefParse
>   /home/berrange/src/virt/libvirt/src/conf/domain_conf.c:12590
>   testCompareXMLToArgvFiles
>   /home/berrange/src/virt/libvirt/tests/qemuxml2argvtest.c:106
>   virtTestRun
>   /home/berrange/src/virt/libvirt/tests/testutils.c:250
>   mymain
>   /home/berrange/src/virt/libvirt/tests/qemuxml2argvtest.c:418 (discriminator 2)
>   virtTestMain
>   /home/berrange/src/virt/libvirt/tests/testutils.c:750
>   ??
>   ??:0
>   _start
>   ??:?
>    FAILED
> 
> Signed-off-by: Daniel P. Berrange <berrange redhat com>
> ---
>  tests/cputest.c          |   3 +-
>  tests/qemuargv2xmltest.c |  14 ++--
>  tests/qemuxml2argvtest.c |  12 ++-
>  tests/qemuxmlnstest.c    |  18 +++--
>  tests/testutils.c        | 189 ++++++++++++++++++++++++++++++++++++++++++++++-
>  tests/testutils.h        |   2 +
>  6 files changed, 216 insertions(+), 22 deletions(-)
> 
> diff --git a/tests/cputest.c b/tests/cputest.c
> index 8e3640b..17cb6af 100644
> --- a/tests/cputest.c
> +++ b/tests/cputest.c
> @@ -476,7 +476,8 @@ cpuTestRun(const char *name, const struct data *data)
>          if (virTestGetDebug()) {
>              char *log;
>              if ((log = virtTestLogContentAndReset()) &&
> -                 strlen(log) > 0)
> +                log != NULL &&
> +                strlen(log) > 0)

Spurious change adding dead code.  You already checked that log was
non-null in the left half of the &&.

> +++ b/tests/testutils.c
> @@ -47,6 +47,9 @@
>  #include "virprocess.h"
>  #include "virstring.h"
>  
> +#include <dlfcn.h>
> +#include <execinfo.h>

Needs to be conditional (these are not universal headers; gnulib won't
help you).

> +#ifdef TEST_OOM
> +static unsigned int testOOM = 0;
> +static unsigned int testOOMStart = -1;
> +static unsigned int testOOMEnd = -1;
> +static unsigned int testOOMTrace = 0;
> +# ifdef TEST_OOM_TRACE

Why the two conditions?  Or is TEST_OOM_TRACE based on whether
<execinfo.h> is present?

> +virTestShowTrace(void)
> +{
> +    size_t j;
> +    for (j = 2; j < ntestAllocStack; j++) {
> +        Dl_info info;
> +        char *cmd;
> +
> +        dladdr(testAllocStack[j], &info);
> +        if (info.dli_fname &&
> +            strstr(info.dli_fname, ".so")) {
> +            if (virAsprintf(&cmd, "addr2line -f -e %s %p",
> +                            info.dli_fname,
> +                            ((void*)((unsigned long long)testAllocStack[j]
> +                                     - (unsigned long long)info.dli_fbase))) < 0)
> +                continue;
> +        } else {
> +            if (virAsprintf(&cmd, "addr2line -f -e %s %p",
> +                            (char*)(info.dli_fname ? info.dli_fname : "<unknown>"),
> +                            testAllocStack[j]) < 0)
> +                continue;
> +        }
> +        ignore_value(system(cmd));

configure.ac defined TEST_OOM_TRACE solely based on whether
'backtrace()' exists; but as you are also using addr2line and looking
for .so files, which starts to feel specific to Linux, is it more
conservative to disable this option on non-Linux?

> @@ -168,7 +226,7 @@ virtTestRun(const char *title,
>              !((testCounter-1) % 40)) {
>              fprintf(stderr, " %-3zu\n", (testCounter-1));
>              fprintf(stderr, "      ");
> -            }
> +        }

Missed in patch 3/4?


> +        testOOMActive = true;
> +        for (i = start; i < end; i++) {
> +            bool missingFail = false;
> +# ifdef TEST_OOM_TRACE
> +            memset(testAllocStack, 0, ARRAY_CARDINALITY(testAllocStack));
> +            ntestAllocStack = 0;
> +# endif
> +            virAllocTestOOM(i+1, 1);

spaces around +

> +            oomret = body(data);
> +
> +            /* fprintf() disabled because XML parsing APIs don't allow
> +             * distinguish between element / attribute not present
> +             * in the XML (which is non-fatal), vs OOM / malformed
> +             * which should be fatal. Thus error reporting for
> +             * optionally present XML is mostly broken.
> +             */
> +            if (oomret == 0) {
> +                missingFail = true;
> +# if 0
> +                fprintf(stderr, " alloc %zu failed but no err status\n", i + 1);
> +# endif
> +            } else {
> +                virErrorPtr lerr = virGetLastError();
> +                if (!lerr) {
> +# if 0
> +                    fprintf(stderr, " alloc %zu failed but no error report\n", i + 1);
> +# endif
> +                    missingFail = true;
> +                }
> +            }
> +            if ((missingFail && testOOMTrace) || (testOOMTrace > 1)) {

Safe to drop () around 'testOOMTrace > 1'

> +                fprintf(stderr, "%s", "!");
> +# ifdef TEST_OOM_TRACE
> +                virTestShowTrace();
> +# endif
> +                ret = -1;
> +            } else {
> +                fprintf(stderr, "%s", ".");
> +            }
> +        }
> +        testOOMActive = false;
> +        if (ret == 0)
> +            fprintf(stderr, " OK\n");
> +        else
> +            fprintf(stderr, " FAILED\n");
> +        virAllocTestInit();
> +    }
> +#endif /* TEST_OOM */
> +
>      return ret;
>  }
>  
> @@ -205,7 +334,7 @@ virtTestLoadFile(const char *file, char **buf)
>      tmplen = buflen = st.st_size + 1;
>  
>      if (VIR_ALLOC_N(*buf, buflen) < 0) {
> -        fprintf(stderr, "%s: larger than available memory (> %d)\n", file, buflen);
> +        //fprintf(stderr, "%s: larger than available memory (> %d)\n", file, buflen);

Is this hunk intentional?

>          VIR_FORCE_FCLOSE(fp);
>          return -1;
>      }
> @@ -481,7 +610,8 @@ virtTestLogOutput(virLogSource source ATTRIBUTE_UNUSED,
>  {
>      struct virtTestLogData *log = data;
>      virCheckFlags(VIR_LOG_STACK_TRACE,);
> -    virBufferAsprintf(&log->buf, "%s: %s", timestamp, str);
> +    if (!testOOMActive)
> +        virBufferAsprintf(&log->buf, "%s: %s", timestamp, str);
>  }
>  
>  static void
> @@ -550,6 +680,9 @@ int virtTestMain(int argc,
>      int ret;
>      bool abs_srcdir_cleanup = false;
>      char *testRange = NULL;
> +#ifdef TEST_OOM
> +    char *oomstr;
> +#endif
>  
>      abs_srcdir = getenv("abs_srcdir");
>      if (!abs_srcdir) {
> @@ -610,6 +743,56 @@ int virtTestMain(int argc,
>          }
>      }
>  
> +#ifdef TEST_OOM
> +    if ((oomstr = getenv("VIR_TEST_OOM")) != NULL) {
> +        char *next;
> +        if (testDebug == -1)
> +            testDebug = 1;
> +        testOOM = 1;
> +        if (oomstr[0] != '\0' &&
> +            oomstr[1] == ':') {
> +            if (virStrToLong_ui(oomstr + 2, &next, 10, &testOOMStart) < 0) {
> +                fprintf(stderr, "Cannot parse range %s\n", oomstr);
> +                return EXIT_FAILURE;
> +            }
> +            if (*next != '-') {
> +                fprintf(stderr, "Cannot parse range %s\n", oomstr);
> +                return EXIT_FAILURE;
> +            }

Worth accepting VIR_TEST_OOM=1:1 as shorthand for 1:1-1?

I like where it's headed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]