From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [PATCH v10 12/12] bpf: add sample for xdp forwarding and rewrite Date: Thu, 04 Aug 2016 01:18:36 +0200 Message-ID: <57A27BCC.4000608@iogearbox.net> References: <1468955817-10604-1-git-send-email-bblanco@plumgrid.com> <1468955817-10604-13-git-send-email-bblanco@plumgrid.com> <20160803171118.GA37742@ast-mbp.thefacebook.com> <20160803182950.GA10130@gmail.com> <20160803223632.GA42605@ast-mbp.thefacebook.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: Brenden Blanco , "David S. Miller" , Linux Kernel Network Developers , Jamal Hadi Salim , Saeed Mahameed , Martin KaFai Lau , Jesper Dangaard Brouer , Ari Saha , Or Gerlitz , john fastabend , Hannes Frederic Sowa , Thomas Graf , Tariq Toukan , Aaron Yue To: Alexei Starovoitov , Tom Herbert Return-path: Received: from www62.your-server.de ([213.133.104.62]:40311 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758389AbcHCXTk (ORCPT ); Wed, 3 Aug 2016 19:19:40 -0400 In-Reply-To: <20160803223632.GA42605@ast-mbp.thefacebook.com> Sender: netdev-owner@vger.kernel.org List-ID: On 08/04/2016 12:36 AM, Alexei Starovoitov wrote: > On Wed, Aug 03, 2016 at 12:06:55PM -0700, Tom Herbert wrote: [...] >> This is not at all obvious to XDP programmer. The type of ctx >> structure is xdp_md and the definition of that structure in >> uapi/linux/bpf.h says that the fields in the that structure are __u32. >> So when I, as a user naive the inner workings of the verifier, read >> this code it sure looks like we are upcasting a 32 bit value to a 64 >> bit value-- that does not seem right at all and the compiler >> apparently concurs my point of view. If the code ends up being correct >> anyway, then the obvious answer to have an explicit cast that points >> out the special nature of this cast. Blindly casting to u32 to long >> for the purposes of assigning to a pointer is only going to confuse >> more people as it has me. > > Agree. Would be nice to have few helpers. The question is whether > they belong in bpf.h. Probably not, since they're not kernel abi. Yeah, fully agree, they don't belong into uapi. > For the same reasons we didn't include instruction building macros > like BPF_ALU64_REG and instead kept them in samples/bpf/libbpf.h +1 > Here probably four static inline functions are needed. Two for __sk_buff > and two for xpd_md. That should make xdp*_kern.c examples a bit easier > to read. Yeah, all these bits should go into some library headers that the test cases make use of (and make them easier to parse for developers).