On Wed, Nov 2, 2022 at 8:35 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > An update for libbpf's hashmap interface from void* -> void* to > uintptr_t -> uintptr_t. Removes / simplifies some type casts when > hashmap keys or values are 32-bit integers. In libbpf hashmap is more > often used with integral keys / values rather than with pointer keys / > values. > > This is a follow up for [1]. > > [1] https://lore.kernel.org/bpf/af1facf9-7bc8-8a3d-0db4-7b3f333589a2@xxxxxxxx/T/#m65b28f1d6d969fcd318b556db6a3ad499a42607d > > Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx> > --- I think this is a pretty clear win. Keys are almost always (if not always) integers, so this is very natural. And pointer values are relatively infrequent and casts there look quite clean anyways (there was a place where you were casting to some long-named struct, in C we can cast to void * and rely on compiler to coalesce pointer types, so even such casts can be short). But you forgot to update perf, so it's build is badly broken with these changes, please convert perf as well. Also cc Arnaldo (acme@xxxxxxxxxx) on this patch set, please. > tools/bpf/bpftool/btf.c | 23 ++++++++------------ > tools/bpf/bpftool/common.c | 10 ++++----- > tools/bpf/bpftool/gen.c | 19 +++++++---------- > tools/bpf/bpftool/link.c | 8 +++---- > tools/bpf/bpftool/main.h | 4 ++-- > tools/bpf/bpftool/map.c | 8 +++---- > tools/bpf/bpftool/pids.c | 16 +++++++------- > tools/bpf/bpftool/prog.c | 8 +++---- > tools/lib/bpf/btf.c | 43 +++++++++++++++++++------------------- > tools/lib/bpf/btf_dump.c | 16 +++++++------- > tools/lib/bpf/hashmap.c | 16 +++++++------- > tools/lib/bpf/hashmap.h | 35 ++++++++++++++++--------------- > tools/lib/bpf/libbpf.c | 18 ++++++---------- > tools/lib/bpf/strset.c | 24 ++++++++++----------- > tools/lib/bpf/usdt.c | 31 +++++++++++++-------------- > 15 files changed, 127 insertions(+), 152 deletions(-) > > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c > index 68a70ac03c80..ccb3b8b0378b 100644 > --- a/tools/bpf/bpftool/btf.c > +++ b/tools/bpf/bpftool/btf.c > @@ -815,8 +815,7 @@ build_btf_type_table(struct hashmap *tab, enum bpf_obj_type type, > if (!btf_id) > continue; > > - err = hashmap__append(tab, u32_as_hash_field(btf_id), > - u32_as_hash_field(id)); > + err = hashmap__append(tab, btf_id, id); > if (err) { > p_err("failed to append entry to hashmap for BTF ID %u, object ID %u: %s", > btf_id, id, strerror(-err)); > @@ -875,17 +874,15 @@ show_btf_plain(struct bpf_btf_info *info, int fd, > printf("size %uB", info->btf_size); > > n = 0; > - hashmap__for_each_key_entry(btf_prog_table, entry, > - u32_as_hash_field(info->id)) { > + hashmap__for_each_key_entry(btf_prog_table, entry, info->id) { > printf("%s%u", n++ == 0 ? " prog_ids " : ",", > - hash_field_as_u32(entry->value)); > + (__u32)entry->value); so if we make key/value as long, we won't need such casts, we can just do %lu in printfs I still prefer long vs uintptr_t, and this is one reason for this. I don't understand how uintptr_t simplifies anything with 32-bit integers as uintptr_t is 64-bit on 64-architecture, so you'd still have to handle 32-to-64 conversions. If you are worried about unsigned vs signed conversions, I don't think it's a problem: $ cat t.c #include <stdio.h> int main(){ int x = -2; int lx = (long)x; int xx = (int)lx; unsigned ux = 3000000000; long ulx = (long)ux; unsigned ulxx = (unsigned)ulx; printf("%d %x %ld %lx %d %x\n", x, x, lx, lx, xx, xx); printf("%u %x %ld %lx %u %x\n", ux, ux, ulx, ulx, ulxx, ulxx); } $ cc t.c $ ./a.out -2 fffffffe 4294967294 fffffffe -2 fffffffe 3000000000 b2d05e00 3000000000 b2d05e00 3000000000 b2d05e00 So please clarify what won't work. > } > > n = 0; > - hashmap__for_each_key_entry(btf_map_table, entry, > - u32_as_hash_field(info->id)) { > + hashmap__for_each_key_entry(btf_map_table, entry, info->id) { > printf("%s%u", n++ == 0 ? " map_ids " : ",", > - hash_field_as_u32(entry->value)); > + (__u32)entry->value); > } > > emit_obj_refs_plain(refs_table, info->id, "\n\tpids "); [...]
