Hi,
On 8/29/2022 6:39 AM, KP Singh wrote:
> On Sat, Aug 27, 2022 at 11:43 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>> From: Hou Tao <houtao1@xxxxxxxxxx>
>>
>> One test demonstrates the reentrancy of hash map update fails, and
>> another one shows concureently updates of the same hash map bucket
> "concurrent updates of the same hashmap"
>
> This is just a description of what the test does but not why?
Will elaborate the description.
>
> What's the expected behaviour? Was it broken?
>
> I think your whole series will benefit from a cover letter to explain
> the stuff you are fixing.
Yes. There is a patch 0 for v2, but I forge to send it out.
>
>> succeed.
>>
>> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
>> ---
>> .../selftests/bpf/prog_tests/htab_update.c | 126 ++++++++++++++++++
>> .../testing/selftests/bpf/progs/htab_update.c | 29 ++++
>> 2 files changed, 155 insertions(+)
>> create mode 100644 tools/testing/selftests/bpf/prog_tests/htab_update.c
>> create mode 100644 tools/testing/selftests/bpf/progs/htab_update.c
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/htab_update.c b/tools/testing/selftests/bpf/prog_tests/htab_update.c
>> new file mode 100644
>> index 000000000000..e2a4034daa79
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/htab_update.c
>> @@ -0,0 +1,126 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (C) 2022. Huawei Technologies Co., Ltd */
>> +#define _GNU_SOURCE
>> +#include <sched.h>
>> +#include <stdbool.h>
>> +#include <test_progs.h>
>> +#include "htab_update.skel.h"
>> +
>> +struct htab_update_ctx {
>> + int fd;
>> + int loop;
>> + bool stop;
>> +};
>> +
>> +static void test_reenter_update(void)
>> +{
>> + struct htab_update *skel;
>> + unsigned int key, value;
>> + int err;
>> +
>> + skel = htab_update__open();
>> + if (!ASSERT_OK_PTR(skel, "htab_update__open"))
>> + return;
>> +
>> + /* lookup_elem_raw() may be inlined and find_kernel_btf_id() will return -ESRCH */
>> + bpf_program__set_autoload(skel->progs.lookup_elem_raw, true);
>> + err = htab_update__load(skel);
>> + if (!ASSERT_TRUE(!err || err == -ESRCH, "htab_update__load") || err)
>> + goto out;
>> +
>> + skel->bss->pid = getpid();
>> + err = htab_update__attach(skel);
>> + if (!ASSERT_OK(err, "htab_update__attach"))
>> + goto out;
>> +
>> + /* Will trigger the reentrancy of bpf_map_update_elem() */
>> + key = 0;
>> + value = 0;
> nit: just move these initializations to the top.
>
>> + err = bpf_map_update_elem(bpf_map__fd(skel->maps.htab), &key, &value, 0);
>> + if (!ASSERT_OK(err, "add element"))
>> + goto out;
>> +
>> + ASSERT_EQ(skel->bss->update_err, -EBUSY, "no reentrancy");
>> +out:
>> + htab_update__destroy(skel);
>> +}
>> +
>> +static void *htab_update_thread(void *arg)
>> +{
>> + struct htab_update_ctx *ctx = arg;
>> + cpu_set_t cpus;
>> + int i;
>> +
>> + /* Pin on CPU 0 */
>> + CPU_ZERO(&cpus);
>> + CPU_SET(0, &cpus);
>> + pthread_setaffinity_np(pthread_self(), sizeof(cpus), &cpus);
>> +
>> + i = 0;
>> + while (i++ < ctx->loop && !ctx->stop) {
> nit: for loop?
>
>
>> + unsigned int key = 0, value = 0;
>> + int err;
>> +
>> + err = bpf_map_update_elem(ctx->fd, &key, &value, 0);
>> + if (err) {
>> + ctx->stop = true;
>> + return (void *)(long)err;
>> + }
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static void test_concurrent_update(void)
>> +{
>> + struct htab_update_ctx ctx;
>> + struct htab_update *skel;
>> + unsigned int i, nr;
>> + pthread_t *tids;
>> + int err;
>> +
>> + skel = htab_update__open_and_load();
>> + if (!ASSERT_OK_PTR(skel, "htab_update__open_and_load"))
>> + return;
>> +
>> + ctx.fd = bpf_map__fd(skel->maps.htab);
>> + ctx.loop = 1000;
>> + ctx.stop = false;
>> +
>> + nr = 4;
>> + tids = calloc(nr, sizeof(*tids));
>> + if (!ASSERT_NEQ(tids, NULL, "no mem"))
>> + goto out;
>> +
>> + for (i = 0; i < nr; i++) {
>> + err = pthread_create(&tids[i], NULL, htab_update_thread, &ctx);
>> + if (!ASSERT_OK(err, "pthread_create")) {
>> + unsigned int j;
>> +
>> + ctx.stop = true;
>> + for (j = 0; j < i; j++)
>> + pthread_join(tids[j], NULL);
>> + goto out;
>> + }
>> + }
>> +
>> + for (i = 0; i < nr; i++) {
>> + void *thread_err = NULL;
>> +
>> + pthread_join(tids[i], &thread_err);
>> + ASSERT_EQ(thread_err, NULL, "update error");
>> + }
>> +
>> +out:
>> + if (tids)
>> + free(tids);
>> + htab_update__destroy(skel);
>> +}
>> +
>> +void test_htab_update(void)
>> +{
>> + if (test__start_subtest("reenter_update"))
>> + test_reenter_update();
>> + if (test__start_subtest("concurrent_update"))
>> + test_concurrent_update();
>> +}
>> diff --git a/tools/testing/selftests/bpf/progs/htab_update.c b/tools/testing/selftests/bpf/progs/htab_update.c
>> new file mode 100644
>> index 000000000000..7481bb30b29b
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/htab_update.c
>> @@ -0,0 +1,29 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (C) 2022. Huawei Technologies Co., Ltd */
>> +#include <linux/bpf.h>
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +struct {
>> + __uint(type, BPF_MAP_TYPE_HASH);
>> + __uint(max_entries, 1);
>> + __uint(key_size, sizeof(__u32));
>> + __uint(value_size, sizeof(__u32));
>> +} htab SEC(".maps");
>> +
>> +int pid = 0;
>> +int update_err = 0;
>> +
>> +SEC("?fentry/lookup_elem_raw")
>> +int lookup_elem_raw(void *ctx)
>> +{
>> + __u32 key = 0, value = 1;
>> +
>> + if ((bpf_get_current_pid_tgid() >> 32) != pid)
>> + return 0;
>> +
>> + update_err = bpf_map_update_elem(&htab, &key, &value, 0);
>> + return 0;
>> +}
>> --
>> 2.29.2
>>
> .
