lkml.org 
[lkml]   [2024]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] selftests/bpf: Add ring_buffer__consume_n test.
    On Sun, Apr 21, 2024 at 10:11:33PM +0200, Jiri Olsa wrote:
    ..
    > > static struct test_ringbuf_map_key_lskel *skel_map_key;
    > > +static struct test_ringbuf_n_lskel *skel_n;
    >
    > seems like there's no need for this to be static variable

    Can you elaborate more? I think we want these pointers to be static to
    limit the scope to this file, no?

    >
    > > static struct test_ringbuf_lskel *skel;
    > > static struct ring_buffer *ringbuf;
    > >
    > > @@ -326,6 +328,67 @@ static void ringbuf_subtest(void)
    > > test_ringbuf_lskel__destroy(skel);
    > > }
    > >
    > > +/*
    > > + * Test ring_buffer__consume_n() by producing N_TOT_SAMPLES samples in the ring
    > > + * buffer, via getpid(), and consuming them in chunks of N_SAMPLES.
    > > + */
    > > +#define N_TOT_SAMPLES 32
    > > +#define N_SAMPLES 4
    > > +
    > > +/* Sample value to verify the callback validity */
    > > +#define SAMPLE_VALUE 42L
    > > +
    > > +static int process_n_sample(void *ctx, void *data, size_t len)
    > > +{
    > > + struct sample *s = data;
    > > +
    > > + CHECK(s->value != SAMPLE_VALUE,
    > > + "sample_value", "exp %ld, got %ld\n", SAMPLE_VALUE, s->value);
    >
    > I think we should use ASSERT macros instead in the new code

    Good catch, I'll change this to an ASSERT_EQ().

    >
    > > +
    > > + return 0;
    > > +}
    > > +
    > > +static void ringbuf_n_subtest(void)
    > > +{
    > > + int err, i;
    > > +
    > > + skel_n = test_ringbuf_n_lskel__open();
    > > + if (!ASSERT_OK_PTR(skel_n, "test_ringbuf_n_lskel__open"))
    > > + return;
    > > +
    > > + skel_n->maps.ringbuf.max_entries = getpagesize();
    > > + skel_n->bss->pid = getpid();
    > > +
    > > + err = test_ringbuf_n_lskel__load(skel_n);
    > > + if (!ASSERT_OK(err, "test_ringbuf_n_lskel__load"))
    > > + goto cleanup;
    > > +
    > > + ringbuf = ring_buffer__new(skel_n->maps.ringbuf.map_fd,
    > > + process_n_sample, NULL, NULL);
    > > + if (!ASSERT_OK_PTR(ringbuf, "ring_buffer__new"))
    > > + goto cleanup;
    > > +
    > > + err = test_ringbuf_n_lskel__attach(skel_n);
    > > + if (!ASSERT_OK(err, "test_ringbuf_n_lskel__attach"))
    > > + goto cleanup_ringbuf;
    > > +
    > > + /* Produce N_TOT_SAMPLES samples in the ring buffer by calling getpid() */
    > > + skel->bss->value = SAMPLE_VALUE;
    >
    > skel_n ?

    Absolutely... I'm suprised that it works actually, I guess pure luck
    (unluck) to reuse the old pointer and have value mapped to the same
    location. Anyway, I'll fix this.

    >
    > > + for (i = 0; i < N_TOT_SAMPLES; i++)
    > > + syscall(__NR_getpgid);
    > > +
    > > + /* Consume all samples from the ring buffer in batches of N_SAMPLES */
    > > + for (i = 0; i < N_TOT_SAMPLES; i += err) {
    > > + err = ring_buffer__consume_n(ringbuf, N_SAMPLES);
    > > + ASSERT_EQ(err, N_SAMPLES, "rb_consume");
    > > + }
    > > +
    >
    > SNIP
    >
    > > diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_n.c b/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
    > > new file mode 100644
    > > index 000000000000..b98b5bb20699
    > > --- /dev/null
    > > +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
    > > @@ -0,0 +1,52 @@
    > > +// SPDX-License-Identifier: GPL-2.0
    > > +// Copyright (c) 2024 Andrea Righi <andrea.righi@canonical.com>
    > > +
    > > +#include <linux/bpf.h>
    > > +#include <sched.h>
    > > +#include <unistd.h>
    > > +#include <bpf/bpf_helpers.h>
    > > +#include "bpf_misc.h"
    > > +
    > > +char _license[] SEC("license") = "GPL";
    > > +
    > > +#define TASK_COMM_LEN 16
    > > +
    > > +struct sample {
    > > + int pid;
    > > + int seq;
    >
    > seq does not seem to be checked, is it needed?

    seq is not used at all, I can definitely drop it.

    Thanks for the review! I'll send a v2.

    -Andrea

    \
     
     \ /
      Last update: 2024-05-27 18:02    [W:3.817 / U:0.840 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site