Message-ID: <b8297ad5-5962-4d9f-acbf-0bb70a3035da@naccy.de>
Date: Mon, 12 Feb 2024 16:22:22 +0100
From: Quentin Deslandes <qde@...cy.de>
To: Matthieu Baerts <matttbe@...nel.org>, netdev@...r.kernel.org
Cc: Stephen Hemminger <stephen@...workplumber.org>,
 David Ahern <dsahern@...il.com>, Martin KaFai Lau <martin.lau@...nel.org>,
 kernel-team@...a.com
Subject: Re: [RFC iproute2 v6 2/3] ss: pretty-print BPF socket-local storage

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

Powered by blists - more mailing lists