Skip to content

Commit 82f4302

Browse files
introduce map.lookup_or_try_init() (iovisor#2577)
Previously, map.lookup_or_init() may cause unexpected return from the function when lookup finds no element and init failed e.g. due to unlikely racy update or sometimes hash table full. This has caught surprise from many users. So, the commit iovisor@ba64f03 attempts to remove the early return in map.lookup_or_init(). But then since NULL pointer could be returned, user will need to change their bpf program to check return value, otherwise, verifier will reject the program. As described in the above, such an API behavior change may cause verifier failure and reject previously loadable bpf programs. bcc should try to maintain API stability, esp. to avoid subtle API behavior change. This patch propose to restore the behavior of map.lookup_or_init() and introduce a new one map.lookup_or_try_init(), which will avoid unexpected return. The name is suggested by Alexei to reflect that init may fail. map.lookup_or_try_init() will be formally documented and used in bcc. A warning will be generated if map.lookup_or_init() is used. Documentation will make it clear that map.lookup_or_try_init() is preferred over map.lookup_or_init(). ``` -bash-4.4$ sudo ./syscount.py /virtual/main.c:71:11: warning: lookup_or_init() may return from the function, use loopup_or_try_init() instead. val = data.lookup_or_init(&key, &zero); ^ 1 warning generated. Tracing syscalls, printing top 10... Ctrl+C to quit. ... ``` All uses in examples and tools are converted to use lookup_or_try_init(). Most tests are converted to use lookup_or_try_init() too except test_trace_maxactive.py and test_tracepoint.py to test lookup_or_init() functionality.
1 parent eb1a2f6 commit 82f4302

39 files changed

+75
-73
lines changed

docs/reference_guide.md

+11-8
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ This guide is incomplete. If something feels missing, check the bcc and kernel s
4545
- [10. BPF_DEVMAP](#10-bpf_devmap)
4646
- [11. BPF_CPUMAP](#11-bpf_cpumap)
4747
- [12. map.lookup()](#12-maplookup)
48-
- [13. map.lookup_or_init()](#13-maplookup_or_init)
48+
- [13. map.lookup_or_try_init()](#13-maplookup_or_try_init)
4949
- [14. map.delete()](#14-mapdelete)
5050
- [15. map.update()](#15-mapupdate)
5151
- [16. map.insert()](#16-mapinsert)
@@ -537,7 +537,7 @@ Syntax: ```BPF_TABLE(_table_type, _key_type, _leaf_type, _name, _max_entries)```
537537

538538
Creates a map named ```_name```. Most of the time this will be used via higher-level macros, like BPF_HASH, BPF_HIST, etc.
539539

540-
Methods (covered later): map.lookup(), map.lookup_or_init(), map.delete(), map.update(), map.insert(), map.increment().
540+
Methods (covered later): map.lookup(), map.lookup_or_try_init(), map.delete(), map.update(), map.insert(), map.increment().
541541

542542
Examples in situ:
543543
[search /examples](https://github.com/iovisor/bcc/search?q=BPF_TABLE+path%3Aexamples&type=Code),
@@ -570,7 +570,7 @@ BPF_HASH(start, struct request *);
570570

571571
This creates a hash named ```start``` where the key is a ```struct request *```, and the value defaults to u64. This hash is used by the disksnoop.py example for saving timestamps for each I/O request, where the key is the pointer to struct request, and the value is the timestamp.
572572

573-
Methods (covered later): map.lookup(), map.lookup_or_init(), map.delete(), map.update(), map.insert(), map.increment().
573+
Methods (covered later): map.lookup(), map.lookup_or_try_init(), map.delete(), map.update(), map.insert(), map.increment().
574574

575575
Examples in situ:
576576
[search /examples](https://github.com/iovisor/bcc/search?q=BPF_HASH+path%3Aexamples&type=Code),
@@ -705,7 +705,7 @@ BPF_LPM_TRIE(trie, struct key_v6);
705705

706706
This creates an LPM trie map named `trie` where the key is a `struct key_v6`, and the value defaults to u64.
707707

708-
Methods (covered later): map.lookup(), map.lookup_or_init(), map.delete(), map.update(), map.insert(), map.increment().
708+
Methods (covered later): map.lookup(), map.lookup_or_try_init(), map.delete(), map.update(), map.insert(), map.increment().
709709

710710
Examples in situ:
711711
[search /examples](https://github.com/iovisor/bcc/search?q=BPF_LPM_TRIE+path%3Aexamples&type=Code),
@@ -766,15 +766,18 @@ Examples in situ:
766766
[search /examples](https://github.com/iovisor/bcc/search?q=lookup+path%3Aexamples&type=Code),
767767
[search /tools](https://github.com/iovisor/bcc/search?q=lookup+path%3Atools&type=Code)
768768

769-
### 13. map.lookup_or_init()
769+
### 13. map.lookup_or_try_init()
770770

771-
Syntax: ```*val map.lookup_or_init(&key, &zero)```
771+
Syntax: ```*val map.lookup_or_try_init(&key, &zero)```
772772

773773
Lookup the key in the map, and return a pointer to its value if it exists, else initialize the key's value to the second argument. This is often used to initialize values to zero. If the key cannot be inserted (e.g. the map is full) then NULL is returned.
774774

775775
Examples in situ:
776-
[search /examples](https://github.com/iovisor/bcc/search?q=lookup_or_init+path%3Aexamples&type=Code),
777-
[search /tools](https://github.com/iovisor/bcc/search?q=lookup_or_init+path%3Atools&type=Code)
776+
[search /examples](https://github.com/iovisor/bcc/search?q=lookup_or_try_init+path%3Aexamples&type=Code),
777+
[search /tools](https://github.com/iovisor/bcc/search?q=lookup_or_try_init+path%3Atools&type=Code)
778+
779+
Note: The old map.lookup_or_init() may cause return from the function, so lookup_or_try_init() is recommended as it
780+
does not have this side effect.
778781

779782
### 14. map.delete()
780783

docs/tutorial_bcc_python_developer.md

+2-2
Original file line numberDiff line numberDiff line change
@@ -558,7 +558,7 @@ int count(struct pt_regs *ctx) {
558558
559559
bpf_probe_read(&key.c, sizeof(key.c), (void *)PT_REGS_PARM1(ctx));
560560
// could also use `counts.increment(key)`
561-
val = counts.lookup_or_init(&key, &zero);
561+
val = counts.lookup_or_try_init(&key, &zero);
562562
if (val) {
563563
(*val)++;
564564
}
@@ -679,7 +679,7 @@ int count_sched(struct pt_regs *ctx, struct task_struct *prev) {
679679
key.prev_pid = prev->pid;
680680

681681
// could also use `stats.increment(key);`
682-
val = stats.lookup_or_init(&key, &zero);
682+
val = stats.lookup_or_try_init(&key, &zero);
683683
if (val) {
684684
(*val)++;
685685
}

examples/cpp/LLCStat.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ int on_cache_miss(struct bpf_perf_event_data *ctx) {
4242
get_key(&key);
4343
4444
u64 zero = 0, *val;
45-
val = miss_count.lookup_or_init(&key, &zero);
45+
val = miss_count.lookup_or_try_init(&key, &zero);
4646
if (val) {
4747
(*val) += ctx->sample_period;
4848
}
@@ -55,7 +55,7 @@ int on_cache_ref(struct bpf_perf_event_data *ctx) {
5555
get_key(&key);
5656
5757
u64 zero = 0, *val;
58-
val = ref_count.lookup_or_init(&key, &zero);
58+
val = ref_count.lookup_or_try_init(&key, &zero);
5959
if (val) {
6060
(*val) += ctx->sample_period;
6161
}

examples/cpp/TCPSendStack.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ int on_tcp_send(struct pt_regs *ctx) {
3838
key.user_stack = stack_traces.get_stackid(ctx, BPF_F_USER_STACK);
3939
4040
u64 zero = 0, *val;
41-
val = counts.lookup_or_init(&key, &zero);
41+
val = counts.lookup_or_try_init(&key, &zero);
4242
if (val) {
4343
(*val)++;
4444
}

examples/cpp/UseExternalMap.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ int on_sched_switch(struct tracepoint__sched__sched_switch *args) {
5555
key.next_pid = args->next_pid;
5656
__builtin_memcpy(&key.prev_comm, args->prev_comm, 16);
5757
__builtin_memcpy(&key.next_comm, args->next_comm, 16);
58-
val = counts.lookup_or_init(&key, &zero);
58+
val = counts.lookup_or_try_init(&key, &zero);
5959
if (val) {
6060
(*val)++;
6161
}

examples/lua/offcputime.lua

+1-1
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ int oncpu(struct pt_regs *ctx, struct task_struct *prev) {
6464
bpf_get_current_comm(&key.name, sizeof(key.name));
6565
key.stack_id = stack_traces.get_stackid(ctx, stack_flags);
6666
67-
val = counts.lookup_or_init(&key, &zero);
67+
val = counts.lookup_or_try_init(&key, &zero);
6868
if (val) {
6969
(*val) += delta;
7070
}

examples/lua/task_switch.lua

+1-1
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ int count_sched(struct pt_regs *ctx, struct task_struct *prev) {
3232
key.curr_pid = bpf_get_current_pid_tgid();
3333
key.prev_pid = prev->pid;
3434
35-
val = stats.lookup_or_init(&key, &zero);
35+
val = stats.lookup_or_try_init(&key, &zero);
3636
if (val) {
3737
(*val)++;
3838
}

examples/networking/distributed_bridge/tunnel.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ int handle_ingress(struct __sk_buff *skb) {
3636
if (ifindex) {
3737
//bpf_trace_printk("ingress tunnel_id=%d ifindex=%d\n", tkey.tunnel_id, *ifindex);
3838
struct vni_key vk = {ethernet->src, *ifindex, 0};
39-
struct host *src_host = mac2host.lookup_or_init(&vk,
39+
struct host *src_host = mac2host.lookup_or_try_init(&vk,
4040
&(struct host){tkey.tunnel_id, tkey.remote_ipv4, 0, 0});
4141
if (src_host) {
4242
lock_xadd(&src_host->rx_pkts, 1);

examples/networking/http_filter/http-parse-complete.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ int http_filter(struct __sk_buff *skb) {
141141
//keep the packet and send it to userspace retruning -1
142142
HTTP_MATCH:
143143
//if not already present, insert into map <Key, Leaf>
144-
sessions.lookup_or_init(&key,&zero);
144+
sessions.lookup_or_try_init(&key,&zero);
145145

146146
//send packet to userspace returning -1
147147
KEEP:

examples/networking/tunnel_monitor/monitor.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ ip: ;
125125
if (key.outer_dip < key.outer_sip)
126126
swap_ipkey(&key);
127127
struct counters zleaf = {0};
128-
struct counters *leaf = stats.lookup_or_init(&key, &zleaf);
128+
struct counters *leaf = stats.lookup_or_try_init(&key, &zleaf);
129129
if (leaf) {
130130
if (is_ingress) {
131131
lock_xadd(&leaf->rx_pkts, 1);

examples/networking/vlan_learning/vlan_learning.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ int handle_phys2virt(struct __sk_buff *skb) {
3232
// auto-program reverse direction table
3333
int out_ifindex = leaf->out_ifindex;
3434
struct ifindex_leaf_t zleaf = {0};
35-
struct ifindex_leaf_t *out_leaf = egress.lookup_or_init(&out_ifindex, &zleaf);
35+
struct ifindex_leaf_t *out_leaf = egress.lookup_or_try_init(&out_ifindex, &zleaf);
3636
if (out_leaf) {
3737
// to capture potential configuration changes
3838
out_leaf->out_ifindex = skb->ifindex;

examples/tracing/mallocstacks.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@
4646
4747
// could also use `calls.increment(key, size);`
4848
u64 zero = 0, *val;
49-
val = calls.lookup_or_init(&key, &zero);
49+
val = calls.lookup_or_try_init(&key, &zero);
5050
if (val) {
5151
(*val) += size;
5252
}

examples/tracing/strlen_count.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
3434
bpf_probe_read(&key.c, sizeof(key.c), (void *)PT_REGS_PARM1(ctx));
3535
// could also use `counts.increment(key)`
36-
val = counts.lookup_or_init(&key, &zero);
36+
val = counts.lookup_or_try_init(&key, &zero);
3737
if (val) {
3838
(*val)++;
3939
}

examples/tracing/task_switch.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ int count_sched(struct pt_regs *ctx, struct task_struct *prev) {
1515
key.prev_pid = prev->pid;
1616

1717
// could also use `stats.increment(key);`
18-
val = stats.lookup_or_init(&key, &zero);
18+
val = stats.lookup_or_try_init(&key, &zero);
1919
if (val) {
2020
(*val)++;
2121
}

examples/usdt_sample/scripts/lat_avg.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@
7878
start_hash.delete(&operation_id);
7979
8080
struct hash_leaf_t zero = {};
81-
struct hash_leaf_t* hash_leaf = lat_hash.lookup_or_init(&hash_key, &zero);
81+
struct hash_leaf_t* hash_leaf = lat_hash.lookup_or_try_init(&hash_key, &zero);
8282
if (0 == hash_leaf) {
8383
return 0;
8484
}

src/cc/export/helpers.h

+1
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ struct _name##_table_t { \
7878
_leaf_type leaf; \
7979
_leaf_type * (*lookup) (_key_type *); \
8080
_leaf_type * (*lookup_or_init) (_key_type *, _leaf_type *); \
81+
_leaf_type * (*lookup_or_try_init) (_key_type *, _leaf_type *); \
8182
int (*update) (_key_type *, _leaf_type *); \
8283
int (*insert) (_key_type *, _leaf_type *); \
8384
int (*delete) (_key_type *); \

src/cc/frontends/clang/b_frontend_action.cc

+8-2
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,8 @@ bool ProbeVisitor::assignsExtPtr(Expr *E, int *nbAddrOf) {
291291
if (!A->getName().startswith("maps"))
292292
return false;
293293

294-
if (memb_name == "lookup" || memb_name == "lookup_or_init") {
294+
if (memb_name == "lookup" || memb_name == "lookup_or_init" ||
295+
memb_name == "lookup_or_try_init") {
295296
if (m_.find(Ref->getDecl()) != m_.end()) {
296297
// Retrieved an ext. pointer from a map, mark LHS as ext. pointer.
297298
// Pointers from maps always need a single dereference to get the
@@ -772,7 +773,7 @@ bool BTypeVisitor::VisitCallExpr(CallExpr *Call) {
772773
string txt;
773774
auto rewrite_start = GET_BEGINLOC(Call);
774775
auto rewrite_end = GET_ENDLOC(Call);
775-
if (memb_name == "lookup_or_init") {
776+
if (memb_name == "lookup_or_init" || memb_name == "lookup_or_try_init") {
776777
string name = Ref->getDecl()->getName();
777778
string arg0 = rewriter_.getRewrittenText(expansionRange(Call->getArg(0)->getSourceRange()));
778779
string arg1 = rewriter_.getRewrittenText(expansionRange(Call->getArg(1)->getSourceRange()));
@@ -782,6 +783,11 @@ bool BTypeVisitor::VisitCallExpr(CallExpr *Call) {
782783
txt += "if (!leaf) {";
783784
txt += " " + update + ", " + arg0 + ", " + arg1 + ", BPF_NOEXIST);";
784785
txt += " leaf = " + lookup + ", " + arg0 + ");";
786+
if (memb_name == "lookup_or_init") {
787+
warning(GET_BEGINLOC(Call), "lookup_or_init() may cause return from the function, "
788+
"use lookup_or_try_init() instead.");
789+
txt += " if (!leaf) return 0;";
790+
}
785791
txt += "}";
786792
txt += "leaf;})";
787793
} else if (memb_name == "increment") {

tests/cc/test_bpf_table.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ TEST_CASE("test bpf stack table", "[bpf_stack_table]") {
181181
int on_sys_getuid(void *ctx) {
182182
int stack_id = stack_traces.get_stackid(ctx, BPF_F_REUSE_STACKID);
183183
int zero = 0, *val;
184-
val = id.lookup_or_init(&zero, &stack_id);
184+
val = id.lookup_or_try_init(&zero, &stack_id);
185185
if (val) {
186186
(*val) = stack_id;
187187
}
@@ -234,7 +234,7 @@ TEST_CASE("test bpf stack_id table", "[bpf_stack_table]") {
234234
int on_sys_getuid(void *ctx) {
235235
int stack_id = stack_traces.get_stackid(ctx, BPF_F_USER_STACK);
236236
int zero = 0, *val;
237-
val = id.lookup_or_init(&zero, &stack_id);
237+
val = id.lookup_or_try_init(&zero, &stack_id);
238238
if (val) {
239239
(*val) = stack_id;
240240
}

tests/lua/test_clang.lua

+1-1
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ int kprobe__finish_task_switch(struct pt_regs *ctx, struct task_struct *prev) {
243243
key.curr_pid = bpf_get_current_pid_tgid();
244244
key.prev_pid = prev->pid;
245245
246-
val = stats.lookup_or_init(&key, &zero);
246+
val = stats.lookup_or_try_init(&key, &zero);
247247
if (val) {
248248
(*val)++;
249249
}

tests/python/test_clang.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -396,7 +396,7 @@ def test_task_switch(self):
396396
key.curr_pid = bpf_get_current_pid_tgid();
397397
key.prev_pid = prev->pid;
398398
399-
val = stats.lookup_or_init(&key, &zero);
399+
val = stats.lookup_or_try_init(&key, &zero);
400400
if (val) {
401401
(*val)++;
402402
}

tests/python/test_license.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ class TestLicense(unittest.TestCase):
4242
key.uid = uid & 0xFFFFFFFF;
4343
bpf_get_current_comm(&(key.comm), 16);
4444
45-
val = counts.lookup_or_init(&key, &zero); // update counter
45+
val = counts.lookup_or_try_init(&key, &zero); // update counter
4646
if (val) {
4747
(*val)++;
4848
}

tests/python/test_lru.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ def test_lru_percpu_hash(self):
2626
int hello_world(void *ctx) {
2727
u32 key=0;
2828
u32 value = 0, *val;
29-
val = stats.lookup_or_init(&key, &value);
29+
val = stats.lookup_or_try_init(&key, &value);
3030
if (val) {
3131
*val += 1;
3232
}

tests/python/test_percpu.py

+3-3
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ def test_u64(self):
2929
int hello_world(void *ctx) {
3030
u32 key=0;
3131
u64 value = 0, *val;
32-
val = stats.lookup_or_init(&key, &value);
32+
val = stats.lookup_or_try_init(&key, &value);
3333
if (val) {
3434
*val += 1;
3535
}
@@ -61,7 +61,7 @@ def test_u32(self):
6161
int hello_world(void *ctx) {
6262
u32 key=0;
6363
u32 value = 0, *val;
64-
val = stats.lookup_or_init(&key, &value);
64+
val = stats.lookup_or_try_init(&key, &value);
6565
if (val) {
6666
*val += 1;
6767
}
@@ -97,7 +97,7 @@ def test_struct_custom_func(self):
9797
int hello_world(void *ctx) {
9898
u32 key=0;
9999
counter value = {0,0}, *val;
100-
val = stats.lookup_or_init(&key, &value);
100+
val = stats.lookup_or_try_init(&key, &value);
101101
if (val) {
102102
val->c1 += 1;
103103
val->c2 += 1;

tests/python/test_stat1.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ int on_packet(struct __sk_buff *skb) {
4747
tx = 1;
4848
}
4949
struct IPLeaf zleaf = {0};
50-
struct IPLeaf *leaf = stats.lookup_or_init(&key, &zleaf);
50+
struct IPLeaf *leaf = stats.lookup_or_try_init(&key, &zleaf);
5151
if (leaf) {
5252
lock_xadd(&leaf->rx_pkts, rx);
5353
lock_xadd(&leaf->tx_pkts, tx);

tests/python/test_trace2.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ BPF_HASH(stats, struct Ptr, struct Counters, 1024);
88
int count_sched(struct pt_regs *ctx) {
99
struct Ptr key = {.ptr = PT_REGS_PARM1(ctx)};
1010
struct Counters zleaf = {0};
11-
struct Counters *val = stats.lookup_or_init(&key, &zleaf);
11+
struct Counters *val = stats.lookup_or_try_init(&key, &zleaf);
1212
if (val) {
1313
val->stat1++;
1414
}

tests/python/test_trace2.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
struct Counters zleaf;
2020
2121
memset(&zleaf, 0, sizeof(zleaf));
22-
struct Counters *val = stats.lookup_or_init(&key, &zleaf);
22+
struct Counters *val = stats.lookup_or_try_init(&key, &zleaf);
2323
if (val) {
2424
val->stat1++;
2525
}

tests/python/test_trace3.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ int probe_blk_update_request(struct pt_regs *ctx) {
4747
index = SLOTS - 1;
4848

4949
u64 zero = 0;
50-
u64 *val = latency.lookup_or_init(&index, &zero);
50+
u64 *val = latency.lookup_or_try_init(&index, &zero);
5151
if (val) {
5252
lock_xadd(val, 1);
5353
}

tests/python/test_trace4.py

+2-2
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,14 @@ def setUp(self):
1414
typedef struct { u64 val; } Val;
1515
BPF_HASH(stats, Key, Val, 3);
1616
int hello(void *ctx) {
17-
Val *val = stats.lookup_or_init(&(Key){1}, &(Val){0});
17+
Val *val = stats.lookup_or_try_init(&(Key){1}, &(Val){0});
1818
if (val) {
1919
val->val++;
2020
}
2121
return 0;
2222
}
2323
int goodbye(void *ctx) {
24-
Val *val = stats.lookup_or_init(&(Key){2}, &(Val){0});
24+
Val *val = stats.lookup_or_try_init(&(Key){2}, &(Val){0});
2525
if (val) {
2626
val->val++;
2727
}

tests/python/test_trace_maxactive.py

+2-6
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,12 @@ def setUp(self):
1515
BPF_HASH(stats, Key, Val, 3);
1616
int hello(void *ctx) {
1717
Val *val = stats.lookup_or_init(&(Key){1}, &(Val){0});
18-
if (val) {
19-
val->val++;
20-
}
18+
val->val++;
2119
return 0;
2220
}
2321
int goodbye(void *ctx) {
2422
Val *val = stats.lookup_or_init(&(Key){2}, &(Val){0});
25-
if (val) {
26-
val->val++;
27-
}
23+
val->val++;
2824
return 0;
2925
}
3026
""")

0 commit comments

Comments
 (0)