| Message ID | 20240118031512.298971-3-qde@naccy.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | David Ahern |
| Headers | show |
| Series | ss: pretty-printing BPF socket-local storage | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Hi Quentin, Thank you for working on that! On 18/01/2024 04:15, Quentin Deslandes wrote: > ss is able to print the map ID(s) for which a given socket has BPF > socket-local storage defined (using --bpf-maps or --bpf-map-id=). However, > the actual content of the map remains hidden. > > This change aims to pretty-print the socket-local storage content following > the socket details, similar to what `bpftool map dump` would do. The exact > output format is inspired by drgn, while the BTF data processing is similar > to bpftool's. > > ss will use libbpf's btf_dump__dump_type_data() to ease pretty-printing > of binary data. This requires out_bpf_sk_storage_print_fn() as a print > callback function used by btf_dump__dump_type_data(). vout() is also > introduced, which is similar to out() but accepts a va_list as > parameter. > > COL_SKSTOR's header is replaced with an empty string, as it doesn't need to > be printed anymore; it's used as a "virtual" column to refer to the > socket-local storage dump, which will be printed under the socket information. > The column's width is fixed to 1, so it doesn't mess up ss' output > (expect if --oneline is used). Do you really need this new column, then? Why not printing that "at the end", in the COL_EXT column, like many extra and optional bits: TCP Info, CGroups, memory, options, etc. Here, it seems a bit confusing: if I understand correctly, these extra and optional bits are handled first, then back to the previous column you added (COL_SKSTOR) to always iterate over the BPF storages, and maybe print more stuff only if the new option is given, optionally on new lines. Would it not print errors even if we didn't ask to display them, e.g. if the size is different from the expected one? Would it not be simpler to extend the last column? If you do that, you will naturally only fetch and iterate over the BPF storages if it is asked to print something, no? To be honest, it looks like there are too many options that can be displayed, and there are probably already enough columns. That's certainly why no other columns have been added for years. I don't know why there was an exception for the "Process" one, but OK. I do think it would be better to have a new "--json" option to structure the output and ease the parsing, than having workarounds to improve the output and ease parsing of some options. But that's a more important task :) Cheers, Matt
Hi Matthieu, Thanks for the feedback, and sorry for the delayed answer. On 2024-01-18 11:49, Matthieu Baerts wrote: > Hi Quentin, > > Thank you for working on that! > > On 18/01/2024 04:15, Quentin Deslandes wrote: >> ss is able to print the map ID(s) for which a given socket has BPF >> socket-local storage defined (using --bpf-maps or --bpf-map-id=). However, >> the actual content of the map remains hidden. >> >> This change aims to pretty-print the socket-local storage content following >> the socket details, similar to what `bpftool map dump` would do. The exact >> output format is inspired by drgn, while the BTF data processing is similar >> to bpftool's. >> >> ss will use libbpf's btf_dump__dump_type_data() to ease pretty-printing >> of binary data. This requires out_bpf_sk_storage_print_fn() as a print >> callback function used by btf_dump__dump_type_data(). vout() is also >> introduced, which is similar to out() but accepts a va_list as >> parameter. >> >> COL_SKSTOR's header is replaced with an empty string, as it doesn't need to >> be printed anymore; it's used as a "virtual" column to refer to the >> socket-local storage dump, which will be printed under the socket information. >> The column's width is fixed to 1, so it doesn't mess up ss' output >> (expect if --oneline is used). > > Do you really need this new column, then? > > Why not printing that "at the end", in the COL_EXT column, like many > extra and optional bits: TCP Info, CGroups, memory, options, etc. I think it's an early misunderstanding of ss on my side. I thought ss would contain the same number of bytes in each column. Hence, if a column contained a lot of data (BPF socket local storage for example), every line of output (even for socket without BPF local storage) would contain a very long and potentially empty column. This explains why I was settings column->width to 1 for BPF socket-local storage. > Here, it seems a bit confusing: if I understand correctly, these extra > and optional bits are handled first, then back to the previous column > you added (COL_SKSTOR) to always iterate over the BPF storages, and > maybe print more stuff only if the new option is given, optionally on > new lines. Would it not print errors even if we didn't ask to display > them, e.g. if the size is different from the expected one? > Would it not be simpler to extend the last column? > If you do that, you will naturally only fetch and iterate over the BPF > storages if it is asked to print something, no? Absolutely, I fixed the patches to reflect this: no more COL_SKSTOR, but appending to COL_EXT instead. If --oneline is used, the BPF map's content will be printed following the content of COL_EXT, on one line. If --oneline is not used, then each map is pretty-printed starting on a new line following the content of COL_EXT. I'll send a v7 very soon :) > To be honest, it looks like there are too many options that can be > displayed, and there are probably already enough columns. That's > certainly why no other columns have been added for years. I don't know > why there was an exception for the "Process" one, but OK. > I do think it would be better to have a new "--json" option to structure > the output and ease the parsing, than having workarounds to improve the > output and ease parsing of some options. But that's a more important task :) This was suggested at some point. JSON output would be great, but both features are not mutually exclusive :) > Cheers, > Matt Regards, Quentin
Hi Quentin, Thank you for your reply! On 12/02/2024 16:22, Quentin Deslandes wrote: > On 2024-01-18 11:49, Matthieu Baerts wrote: (...) >> Here, it seems a bit confusing: if I understand correctly, these extra >> and optional bits are handled first, then back to the previous column >> you added (COL_SKSTOR) to always iterate over the BPF storages, and >> maybe print more stuff only if the new option is given, optionally on >> new lines. Would it not print errors even if we didn't ask to display >> them, e.g. if the size is different from the expected one? >> Would it not be simpler to extend the last column? >> If you do that, you will naturally only fetch and iterate over the BPF >> storages if it is asked to print something, no? > > Absolutely, I fixed the patches to reflect this: no more COL_SKSTOR, but > appending to COL_EXT instead. If --oneline is used, the BPF map's content > will be printed following the content of COL_EXT, on one line. If --oneline is > not used, then each map is pretty-printed starting on a new line following > the content of COL_EXT. > > I'll send a v7 very soon :) Thanks, the v7 looks better! I will let BPF experts to look at this :) >> To be honest, it looks like there are too many options that can be >> displayed, and there are probably already enough columns. That's >> certainly why no other columns have been added for years. I don't know >> why there was an exception for the "Process" one, but OK. >> I do think it would be better to have a new "--json" option to structure >> the output and ease the parsing, than having workarounds to improve the >> output and ease parsing of some options. But that's a more important task :) > > This was suggested at some point. JSON output would be great, but both > features are not mutually exclusive :) Indeed. If the support was already there, it would have maybe eased the printing bit. We just need someone who is brave enough to add this feature :) Cheers, Matt
diff --git a/misc/ss.c b/misc/ss.c index fe0e966b..9ba7d846 100644 --- a/misc/ss.c +++ b/misc/ss.c @@ -53,7 +53,9 @@ #ifdef HAVE_LIBBPF #include <bpf/bpf.h> +#include <bpf/btf.h> #include <bpf/libbpf.h> +#include <linux/btf.h> #endif #if HAVE_RPC @@ -136,7 +138,7 @@ static struct column columns[] = { { ALIGN_RIGHT, "Peer Address:", " ", 0, 0, 0 }, { ALIGN_LEFT, "Port", "", 0, 0, 0 }, { ALIGN_LEFT, "Process", "", 0, 0, 0 }, - { ALIGN_LEFT, "Socket storage", "", 1, 0, 0 }, + { ALIGN_LEFT, "", "", 1, 0, 0 }, { ALIGN_LEFT, "", "", 0, 0, 0 }, }; @@ -1041,11 +1043,10 @@ static int buf_update(int len) } /* Append content to buffer as part of the current field */ -__attribute__((format(printf, 1, 2))) -static void out(const char *fmt, ...) +static void vout(const char *fmt, va_list args) { struct column *f = current_field; - va_list args; + va_list _args; char *pos; int len; @@ -1056,18 +1057,27 @@ static void out(const char *fmt, ...) buffer.head = buf_chunk_new(); again: /* Append to buffer: if we have a new chunk, print again */ + va_copy(_args, args); pos = buffer.cur->data + buffer.cur->len; - va_start(args, fmt); /* Limit to tail room. If we hit the limit, buf_update() will tell us */ - len = vsnprintf(pos, buf_chunk_avail(buffer.tail), fmt, args); - va_end(args); + len = vsnprintf(pos, buf_chunk_avail(buffer.tail), fmt, _args); if (buf_update(len)) goto again; } +__attribute__((format(printf, 1, 2))) +static void out(const char *fmt, ...) +{ + va_list args; + + va_start(args, fmt); + vout(fmt, args); + va_end(args); +} + static int print_left_spacing(struct column *f, int stored, int printed) { int s; @@ -1215,6 +1225,13 @@ static void render_calc_width(void) */ c->width = min(c->width, screen_width); + /* When printing BPF socket-local storage (without --oneline), + * set the BPF output column width to prevent ss from reserving + * space for it and messing up its output as the BPF data is not + * printed on the same line. */ + if (c == &columns[COL_SKSTOR] && !oneline) + c->width = 1; + if (c->width) first = 0; } @@ -3398,6 +3415,9 @@ static struct bpf_map_opts { struct bpf_sk_storage_map_info { unsigned int id; int fd; + struct bpf_map_info info; + struct btf *btf; + struct btf_dump *dump; } maps[MAX_NR_BPF_MAP_ID_OPTS]; bool show_all; } bpf_map_opts; @@ -3408,10 +3428,36 @@ static void bpf_map_opts_mixed_error(void) "ss: --bpf-maps and --bpf-map-id cannot be used together\n"); } +static int bpf_maps_opts_load_btf(struct bpf_map_info *info, struct btf **btf) +{ + if (info->btf_value_type_id) { + *btf = btf__load_from_kernel_by_id(info->btf_id); + if (!*btf) { + fprintf(stderr, "ss: failed to load BTF for map ID %u\n", + info->id); + return -1; + } + } else { + *btf = NULL; + } + + return 0; +} + +static void out_bpf_sk_storage_print_fn(void *ctx, const char *fmt, va_list args) +{ + vout(fmt, args); +} + static int bpf_map_opts_load_info(unsigned int map_id) { + struct btf_dump_opts dopts = { + .sz = sizeof(struct btf_dump_opts) + }; struct bpf_map_info info = {}; uint32_t len = sizeof(info); + struct btf_dump *dump; + struct btf *btf; int fd; int r; @@ -3447,8 +3493,25 @@ static int bpf_map_opts_load_info(unsigned int map_id) return -1; } + r = bpf_maps_opts_load_btf(&info, &btf); + if (r) { + close(fd); + return -1; + } + + dump = btf_dump__new(btf, out_bpf_sk_storage_print_fn, NULL, &dopts); + if (!dump) { + btf__free(btf); + close(fd); + fprintf(stderr, "Failed to create btf_dump object\n"); + return -1; + } + bpf_map_opts.maps[bpf_map_opts.nr_maps].id = map_id; - bpf_map_opts.maps[bpf_map_opts.nr_maps++].fd = fd; + bpf_map_opts.maps[bpf_map_opts.nr_maps].fd = fd; + bpf_map_opts.maps[bpf_map_opts.nr_maps].info = info; + bpf_map_opts.maps[bpf_map_opts.nr_maps].btf = btf; + bpf_map_opts.maps[bpf_map_opts.nr_maps++].dump = dump; return 0; } @@ -3500,8 +3563,11 @@ static void bpf_map_opts_destroy(void) { int i; - for (i = 0; i < bpf_map_opts.nr_maps; ++i) + for (i = 0; i < bpf_map_opts.nr_maps; ++i) { + btf_dump__free(bpf_map_opts.maps[i].dump); + btf__free(bpf_map_opts.maps[i].btf); close(bpf_map_opts.maps[i].fd); + } } static struct rtattr *bpf_map_opts_alloc_rta(void) @@ -3552,10 +3618,73 @@ static struct rtattr *bpf_map_opts_alloc_rta(void) return stgs_rta; } +static void out_bpf_sk_storage_oneline(struct bpf_sk_storage_map_info *info, + const void *data, size_t len) +{ + struct btf_dump_type_data_opts opts = { + .sz = sizeof(struct btf_dump_type_data_opts), + .emit_zeroes = 1, + .compact = 1 + }; + int r; + + out(" map_id: %d ", info->id); + r = btf_dump__dump_type_data(info->dump, info->info.btf_value_type_id, + data, len, &opts); + if (r < 0) + out("failed to dump data: %d", r); +} + +static void out_bpf_sk_storage_multiline(struct bpf_sk_storage_map_info *info, + const void *data, size_t len) +{ + struct btf_dump_type_data_opts opts = { + .sz = sizeof(struct btf_dump_type_data_opts), + .indent_level = 2, + .emit_zeroes = 1 + }; + int r; + + out("\n\tmap_id: %d [\n", info->id); + + r = btf_dump__dump_type_data(info->dump, info->info.btf_value_type_id, + data, len, &opts); + if (r < 0) + out("\t\tfailed to dump data: %d", r); + + out("\n\t]"); +} + +static void out_bpf_sk_storage(int map_id, const void *data, size_t len) +{ + struct bpf_sk_storage_map_info *map_info; + + map_info = bpf_map_opts_get_info(map_id); + if (!map_info) { + /* The kernel might return a map we can't get info for, skip + * it but print the other ones. */ + out("\n\tmap_id: %d failed to fetch info, skipping\n", + map_id); + return; + } + + if (map_info->info.value_size != len) { + fprintf(stderr, "map_id: %d: invalid value size, expecting %u, got %lu\n", + map_id, map_info->info.value_size, len); + return; + } + + if (oneline) + out_bpf_sk_storage_oneline(map_info, data, len); + else + out_bpf_sk_storage_multiline(map_info, data, len); +} + static void show_sk_bpf_storages(struct rtattr *bpf_stgs) { struct rtattr *tb[SK_DIAG_BPF_STORAGE_MAX + 1], *bpf_stg; - unsigned int rem; + unsigned int rem, map_id; + struct rtattr *value; for (bpf_stg = RTA_DATA(bpf_stgs), rem = RTA_PAYLOAD(bpf_stgs); RTA_OK(bpf_stg, rem); bpf_stg = RTA_NEXT(bpf_stg, rem)) { @@ -3567,8 +3696,11 @@ static void show_sk_bpf_storages(struct rtattr *bpf_stgs) (struct rtattr *)bpf_stg); if (tb[SK_DIAG_BPF_STORAGE_MAP_ID]) { - out("map_id:%u ", - rta_getattr_u32(tb[SK_DIAG_BPF_STORAGE_MAP_ID])); + map_id = rta_getattr_u32(tb[SK_DIAG_BPF_STORAGE_MAP_ID]); + value = tb[SK_DIAG_BPF_STORAGE_MAP_VALUE]; + + out_bpf_sk_storage(map_id, RTA_DATA(value), + RTA_PAYLOAD(value)); } } }
ss is able to print the map ID(s) for which a given socket has BPF socket-local storage defined (using --bpf-maps or --bpf-map-id=). However, the actual content of the map remains hidden. This change aims to pretty-print the socket-local storage content following the socket details, similar to what `bpftool map dump` would do. The exact output format is inspired by drgn, while the BTF data processing is similar to bpftool's. ss will use libbpf's btf_dump__dump_type_data() to ease pretty-printing of binary data. This requires out_bpf_sk_storage_print_fn() as a print callback function used by btf_dump__dump_type_data(). vout() is also introduced, which is similar to out() but accepts a va_list as parameter. COL_SKSTOR's header is replaced with an empty string, as it doesn't need to be printed anymore; it's used as a "virtual" column to refer to the socket-local storage dump, which will be printed under the socket information. The column's width is fixed to 1, so it doesn't mess up ss' output (expect if --oneline is used). ss' output remains unchanged unless --bpf-maps or --bpf-map-id= is used, in which case each socket containing BPF local storage will be followed by the content of the storage before the next socket's info is displayed. Signed-off-by: Quentin Deslandes <qde@naccy.de> --- misc/ss.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 144 insertions(+), 12 deletions(-) -- 2.43.0