From mboxrd@z Thu Jan 1 00:00:00 1970 From: Okash Khawaja Subject: Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality Date: Fri, 22 Jun 2018 12:17:52 +0100 Message-ID: <20180622111751.GB3050@w1t1fb> References: <20180620203051.223156973@fb.com> <20180620203703.101156292@fb.com> <20180621145935.41ff8974@cakuba.netronome.com> <20180621225117.dhrkrtmkfbeihbe4@kafai-mbp.dhcp.thefacebook.com> <20180621160719.2cfb4b58@cakuba.netronome.com> <20180621235746.dfq6kdtkogftw3ws@kafai-mbp.dhcp.thefacebook.com> <20180621172523.6cd00ed1@cakuba.netronome.com> <20180622012052.htkvholi674x6i4f@kafai-mbp.dhcp.thefacebook.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: Jakub Kicinski , Daniel Borkmann , Alexei Starovoitov , Yonghong Song , Quentin Monnet , "David S. Miller" , , , To: Martin KaFai Lau Return-path: Content-Disposition: inline In-Reply-To: <20180622012052.htkvholi674x6i4f@kafai-mbp.dhcp.thefacebook.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On Thu, Jun 21, 2018 at 06:20:52PM -0700, Martin KaFai Lau wrote: > On Thu, Jun 21, 2018 at 05:25:23PM -0700, Jakub Kicinski wrote: > > On Thu, 21 Jun 2018 16:58:15 -0700, Martin KaFai Lau wrote: > > > On Thu, Jun 21, 2018 at 04:07:19PM -0700, Jakub Kicinski wrote: > > > > On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote: > > > > > On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote: > > > > > > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote: > > > > > > > $ sudo bpftool map dump -p id 14 > > > > > > > [{ > > > > > > > "key": 0 > > > > > > > },{ > > > > > > > "value": { > > > > > > > "m": 1, > > > > > > > "n": 2, > > > > > > > "o": "c", > > > > > > > "p": [15,16,17,18,15,16,17,18 > > > > > > > ], > > > > > > > "q": [[25,26,27,28,25,26,27,28 > > > > > > > ],[35,36,37,38,35,36,37,38 > > > > > > > ],[45,46,47,48,45,46,47,48 > > > > > > > ],[55,56,57,58,55,56,57,58 > > > > > > > ] > > > > > > > ], > > > > > > > "r": 1, > > > > > > > "s": 0x7ffff6f70568, > > > > > > > "t": { > > > > > > > "x": 5, > > > > > > > "y": 10 > > > > > > > }, > > > > > > > "u": 100, > > > > > > > "v": 20, > > > > > > > "w1": 0x7, > > > > > > > "w2": 0x3 > > > > > > > } > > > > > > > } > > > > > > > ] > > > > > > > > > > > > I don't think this format is okay, JSON output is an API you shouldn't > > > > > > break. You can change the non-JSON output whatever way you like, but > > > > > > JSON must remain backwards compatible. > > > > > > > > > > > > The dump today has object per entry, e.g.: > > > > > > > > > > > > { > > > > > > "key":["0x00","0x00","0x00","0x00", > > > > > > ], > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00" > > > > > > ] > > > > > > } > > > > > > > > > > > > This format must remain, you may only augment it with new fields. E.g.: > > > > > > > > > > > > { > > > > > > "key":["0x00","0x00","0x00","0x00", > > > > > > ], > > > > > > "key_struct":{ > > > > > > "index":0 > > > > > > }, > Got a few questions. > > When we support hashtab later, the key could be int > but reusing the name as "index" is weird. > The key could also be a struct (e.g. a struct to describe ip:port). > Can you suggest how the "key_struct" will look like? > > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00" > > > > > > ], > > > > > > "value_struct":{ > > > > > > "src_ip":2, > If for the same map the user changes the "src_ip" to an array of int[4] > later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4]. > Is it breaking backward compat? > i.e. > struct five_tuples { > - int src_ip; > + int src_ip[4]; > /* ... */ > }; > > > > > > > "dst_ip:0 > > > > > > } > > > > > > } > > > > > I am not sure how useful to have both "key|value" and "(key|value)_struct" > > > > > while most people would prefer "key_struct"/"value_struct" if it is > > > > > available. > > > > > > > > Agreed, it's not that useful, especially with the string-hex debacle :( > > > > It's just about the backwards compat. > > > > > > > > > How about introducing a new option, like "-b", to print the > > > > > map with BTF (if available) such that it won't break the existing > > > > > one (-j or -p) while the "-b" output can keep using the "key" > > > > > and "value". > > > > > > > > > > The existing json can be kept as is. > > > > > > > > That was my knee jerk reaction too, but on reflection it doesn't sound > > > > that great. We expect people with new-enough bpftool to use btf, so it > > > > should be available in the default output, without hiding it behind a > > > > switch. We could add a switch to hide the old output, but that doesn't > > > > give us back the names... What about Key and Value or k and v? Or > > > > key_fields and value_fields? > > > I thought the current default output is "plain" ;) > > > Having said that, yes, the btf is currently printed in json. > > > > > > Ideally, the default json output should do what most people want: > > > print btf and btf only (if it is available). > > > but I don't see a way out without new option if we need to > > > be backward compat :( > > > > > > Agree that showing the btf in the existing json output will be useful (e.g. > > > to hint people that BTF is available). If btf is showing in old json, > > > also agree that the names should be the same with the new json. > > > key_fields and value_fields may hint it has >1 fields though. > > > May be "formatted_key" and "formatted_value"? > > > > SGTM! Or even maybe as a "formatted" object?: > > > > { > > "key":["0x00","0x00","0x00","0x00", > > ], > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00" > > ], > > "formatted":{ > > "key":{ > > "index":0 > > }, > > "value":{ > > "src_ip":2, > > "dst_ip:0 > > } > > } > hmm... that is an extra indentation (keep in mind that the "value" could > already have a few nested structs which itself consumes a few indentations) > but I guess adding another one may be ok-ish. > > > } > > > > > > > > The name XYZ_struct may not be the best, perhaps you can come up with a > > > > > > better one? > > > > > > > > > > > > Does that make sense? Am I missing what you're doing here? > > > > > > > > > > > > One process note - please make sure you run checkpatch.pl --strict on > > > > > > bpftool patches before posting. > > > > > > > > > > > > Thanks for working on this! > > Hi, While I agree on the point of backward compatibility, I think printing two overlapping pieces of information side-by-side will make the interface less clear. Having separate outputs for the two will keep the interface clear and readable. Is there a major downside to adding a new flag for BTF output? Thanks, Okash