Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 22 Jun 2018 12:17:52 +0100
From:   Okash Khawaja <osk@fb.com>
To:     Martin KaFai Lau <kafai@fb.com>
CC:     Jakub Kicinski <jakub.kicinski@netronome.com>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Alexei Starovoitov <ast@kernel.org>,
        Yonghong Song <yhs@fb.com>,
        Quentin Monnet <quentin.monnet@netronome.com>,
        "David S. Miller" <davem@davemloft.net>, <netdev@vger.kernel.org>,
        <kernel-team@fb.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
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"
Content-Disposition: inline
In-Reply-To: <20180622012052.htkvholi674x6i4f@kafai-mbp.dhcp.thefacebook.com>
User-Agent: Mutt/1.9.5 (2018-04-13)
Received-SPF: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Jun 2018 11:18:02.1996 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 334aca44-78b7-4a15-f9db-08d5d831d2b7
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB2501
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-06-22_03:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@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