On Thu, Apr 14, 2022 at 10:07:09AM +0800, Wu Zongyong wrote:
> On Wed, Apr 13, 2022 at 12:30:51PM -0700, Andrii Nakryiko wrote:
> > On Wed, Apr 13, 2022 at 12:08 PM Wu Zongyong
> > <wuzongyong@xxxxxxxxxxxxxxxxx> wrote:
> > >
> > > Hi,
> > >
> > > I tried to count when tracepoint qdisc/qdisc_dequeue hit each time, then read
> > > the count value from userspace by bpf_map_lookup_elem().
> > > With bpftrace, I can see this tracepoint is hit about 700 times, but the count
> > > I get from the bpf map is below 20. It's weird. Then I tried to add a bpf_printk()
> > > to the program, and the count I get is normal which is about 700.
> > >
> > > The bpf program codes like that:
> > >
> > > struct qdisc_dequeue_ctx {
> > > __u64 __pad;
> > > __u64 qdisc;
> > > __u64 txq;
> > > int packets;
> > > __u64 skbaddr;
> > > int ifindex;
> > > __u32 handle;
> > > __u32 parent;
> > > unsigned long txq_state;
> > > };
> > >
> > > struct {
> > > __uint(type, BPF_MAP_TYPE_HASH);
> > > __type(key, int);
> > > __type(value, __u32);
> > > __uint(max_entries, 1);
> > > __uint(pinning, LIBBPF_PIN_BY_NAME);
> > > } count_map SEC(".maps");
> > >
> > > SEC("tracepoint/qdisc/qdisc_dequeue")
> > > int trace_dequeue(struct qdisc_dequeue_ctx *ctx)
> > > {
> > > int key = 0;
> > > __u32 *value;
> > > __u32 init = 0;
> > >
> > > value = bpf_map_lookup_elem(&count_map, &key);
> > > if (value) {
> > > *value += 1;
> > > } else {
> > > bpf_printk("value reset");
> > > bpf_map_update_elem(&count_map, &key, &init, 0);
> > > }
> > > return 0;
> > > }
> > >
> > > Any suggestion is appreciated!
> > >
> >
> > First, why do you use HASH map for single-key map? ARRAY would make
> > more sense for keys that are small integers. But I assume your real
> > world use case needs bigger and more random keys, right?
> >
> Yes, this just is a simple test.
>
> >
> > Second, you have two race conditions. One, you overwrite the value in
> > the map with this bpf_map_update_elem(..., 0). Use BPF_NOEXISTS for
> > initialization to avoid overwriting something that another CPU already
> > set. Another one is your *value += 1 is non-atomic, so you are loosing
> > updates as well. Use __sync_fetch_and_add(value, 1) for atomic
> > increment.
>
> __sync_fetch_and_add do solve my problem. Thanks!
Oh, sorry!
The count value is about 700 when I do a bpf_printk() in my bpf program
and with a background command "cat /sys/kernel/debug/tracing/trace_pipe".
If I remove the bpf_printk() or don't read the trace_pipe, the count
value shows abnormal, for example, about 10.
As your suggestion, the code now is:
value = bpf_map_lookup_elem(&count_map, &key);
if (!value) {
bpf_map_update_elem(&count_map, &key, &init, BPF_NOEXIST);
value = bpf_map_lookup_elem(&count_map, &key);
}
if (!value)
return 0;
bpf_printk("hello"); // I don't know why this affect the count value read from userspace
__sync_fetch_and_add(value, 1);
>
> I have tried to use spinlock to prevent race conditions, but it seems
> that spinlock cannot be used in tracepoint.
>
> >
> > Something like this:
> >
> > value = bpf_map_lookup_elem(&count_map, &key);
> > if (!value) {
> > /* BPF_NOEXIST won't allow to override the value that's already set */
> > bpf_map_update_elem(&count_map, &key, &init, BPF_NOEXISTS);
> > value = bpf_map_lookup_elem(&count_map, &key);
> > }
> > if (!value)
> > return 0;
> >
> > __sync_fetch_and_add(value, 1);
> >
> >
> > > Thanks,
> > > Wu Zongyong
