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:   Tue, 26 Jun 2018 17:48:22 +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: <20180626164820.GA12981@w1t1fb>
References: <20180621160719.2cfb4b58@cakuba.netronome.com>
 <20180621235746.dfq6kdtkogftw3ws@kafai-mbp.dhcp.thefacebook.com>
 <20180621172523.6cd00ed1@cakuba.netronome.com>
 <20180622012052.htkvholi674x6i4f@kafai-mbp.dhcp.thefacebook.com>
 <20180622114032.162b2a76@cakuba.netronome.com>
 <20180622205535.c6vjhdwt5er4wc32@kafai-mbp.dhcp.thefacebook.com>
 <20180622142743.2b890d0f@cakuba.netronome.com>
 <20180622225408.jor7lpvsksnwiiec@kafai-mbp.dhcp.thefacebook.com>
 <20180622163200.20564ec4@cakuba.netronome.com>
 <20180623002639.h4qxy7aakypi6t7b@kafai-mbp.dhcp.thefacebook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20180623002639.h4qxy7aakypi6t7b@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: 26 Jun 2018 16:48:37.1976 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: cd7007c3-da73-41ca-1502-08d5db84aa9e
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR15MB2507
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-06-26_08:,,
 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 Fri, Jun 22, 2018 at 05:26:39PM -0700, Martin KaFai Lau wrote:
> On Fri, Jun 22, 2018 at 04:32:00PM -0700, Jakub Kicinski wrote:
> > On Fri, 22 Jun 2018 15:54:08 -0700, Martin KaFai Lau wrote:
> > > > > > > > > > > >         "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];
> > > > > > > /* ... */
> > > > > > > };    
> > > > > > 
> > > > > > Well, it is breaking backward compat, but it's the program doing it,
> > > > > > not bpftool :)  BTF changes so does the output.    
> > > > > As we see, the key/value's btf-output is inherently not backward compat.
> > > > > Hence, "-j" and "-p" will stay as is.  The whole existing json will
> > > > > be backward compat instead of only partly backward compat.  
> > > > 
> > > > No.  There is a difference between user of a facility changing their
> > > > input and kernel/libraries providing different output in response to
> > > > that, and the libraries suddenly changing the output on their own.
> > > > 
> > > > Your example is like saying if user started using IPv6 addresses
> > > > instead of IPv4 the netlink attributes in dumps will be different so
> > > > kernel didn't keep backwards compat.  While what you're doing is more
> > > > equivalent to dropping support for old ioctl interfaces because there
> > > > is a better mechanism now.  
> > > Sorry, I don't follow this.  I don't see netlink suffer json issue like
> > > the one on "key" and "value".
> > > 
> > > All I can grasp is, the json should normally be backward compat but now
> > > we are saying anything added by btf-output is an exception because
> > > the script parsing it will treat it differently than "key" and "value"
> > 
> > Backward compatibility means that if I run *the same* program against
> > different kernels/libraries it continues to work.  If someone decides
> > to upgrade their program to work with IPv6 (which was your example)
> > obviously there is no way system as a whole will look 1:1 the same.
> > 
> > > > BTF in JSON is very useful, and will help people who writes simple
> > > > orchestration/scripts based on bpftool *a* *lot*.  I really appreciate  
> > > Can you share what the script will do?  I want to understand why
> > > it cannot directly use the BTF format and the map data.
> > 
> > Think about a python script which wants to read a counter in a map.
> > Right now it would have to get the BTF, find out which bytes are the
> > counter, then convert the bytes into a larger int.  With JSON BTF if
> > just does entry["formatted"]["value"]["counter"].
> > 
> > Real life example from my test code (conversion of 3 element counter
> > array):
> > 
> > def str2int(strtab):
> >     inttab = []
> >     for i in strtab:
> >         inttab.append(int(i, 16))
> >     ba = bytearray(inttab)
> >     if len(strtab) == 4:
> >         fmt = "I"
> >     elif len(strtab) == 8:
> >         fmt = "Q"
> >     else:
> >         raise Exception("String array of len %d can't be unpacked to an int" %
> >                         (len(strtab)))
> >     return struct.unpack(fmt, ba)[0]
> > 
> > def convert(elems, idx):
> >     val = []
> >     for i in range(3):
> >         part = elems[idx]["value"][i * length:(i + 1) * length]
> >         val.append(str2int(part))
> >     return val
> > 
> > With BTF it would be:
> > 
> > 	elems[idx]["formatted"]["value"]
> > 
> > Which is fairly awesome.
> Thanks for the example.  Agree that with BTF, things are easier in general.
> 
> btw, what more awesome is,
> #> bpftool map find id 100 key 1
> {
> 	"counter_x": 1,
> 	"counter_y": 10
> }
> 
> > 
> > > > this addition to bpftool and will start using it myself as soon as it
> > > > lands.  I'm not sure why the reluctance to slightly change the output
> > > > format?  
> > > The initial change argument is because the json has to be backward compat.
> > > 
> > > Then we show that btf-output is inherently not backward compat, so
> > > printing it in json does not make sense at all.
> > > 
> > > However, now it is saying part of it does not have to be backward compat.
> > 
> > Compatibility of "formatted" member is defined as -> fields broken out
> > according to BTF.  So it is backward compatible.  The definition of
> > "value" member is -> an array of unfortunately formatted array of
> > ugly hex strings :(
> > 
> > > I am fine putting it under "formatted" for "-j" or "-p" if that is the
> > > case, other than the double output is still confusing.  Lets wait for
> > > Okash's input.
> > >
> > > At the same time, the same output will be used as the default plaintext
> > > output when BTF is available.  Then the plaintext BTF output
> > > will not be limited by the json restrictions when we want
> > > to improve human readability later.  Apparently, the
> > > improvements on plaintext will not be always applicable
> > > to json output.
> > 

hi,

so i guess following is what we want:

1. a "formatted" object nested inside -p and -j switches for bpf map
  dump. this will be JSON and backward compatible
2. an output for humans - which is like the current output. this will
not be JSON. this won't have to be backward compatible. this output will
be shown when neither of -j and -p are supplied and btf info is
available.

i can update the patches to v2 which covers 2 above + all other comments
on the patchset. later we can follow up with a patch for 1.

thanks for valuable feedback :)

okash