All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stanislav Fomichev <sdf@fomichev.me>
To: Jakub Kicinski <jakub.kicinski@netronome.com>
Cc: Quentin Monnet <quentin.monnet@netronome.com>,
	Stanislav Fomichev <sdf@google.com>,
	netdev@vger.kernel.org, linux-kselftest@vger.kernel.org,
	ast@kernel.org, daniel@iogearbox.net, shuah@kernel.org,
	guro@fb.com, jiong.wang@netronome.com,
	bhole_prashant_q7@lab.ntt.co.jp, john.fastabend@gmail.com,
	jbenc@redhat.com, treeze.taeung@gmail.com, yhs@fb.com,
	osk@fb.com, sandipan@linux.vnet.ibm.com
Subject: Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
Date: Thu, 8 Nov 2018 13:25:39 -0800	[thread overview]
Message-ID: <20181108212539.esqjw4k5hz2pxowu@mini-arch> (raw)
In-Reply-To: <20181108114544.1ac0cd44@cakuba.hsd1.ca.comcast.net>

On 11/08, Jakub Kicinski wrote:
> On Thu, 8 Nov 2018 11:16:43 +0000, Quentin Monnet wrote:
> > > -	bpf_program__set_ifindex(prog, ifindex);
> > >   	if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
> > > +		if (!prog) {
> > > +			p_err("can not guess program type when loading all programs\n");
> 
> No new lines in p_err(), beaks JSON.
Thanks, I'll probably remove that altogether and do auto inference of
prog_type from the section name here.

> > > +			goto err_close_obj;
> > > +		}
> > > +
> > >   		const char *sec_name = bpf_program__title(prog, false);
> > >   
> > >   		err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,
> > > @@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
> > >   			goto err_close_obj;
> > >   		}
> > >   	}
> > > -	bpf_program__set_type(prog, attr.prog_type);
> > > -	bpf_program__set_expected_attach_type(prog, expected_attach_type);
> > > +
> > > +	bpf_object__for_each_program(pos, obj) {
> > > +		bpf_program__set_ifindex(pos, ifindex);
> > > +		bpf_program__set_type(pos, attr.prog_type);
> > > +		bpf_program__set_expected_attach_type(pos,
> > > +						      expected_attach_type);
> > > +	}  
> > 
> > I still believe you can have programs of different types here, and be 
> > able to load them. I tried it and managed to have it working fine. If no 
> > type is provided from command line we can retrieve types for each 
> > program from its section name. If a type is provided on the command 
> > line, we can do the same, but I am not sure we should do it, or impose 
> > that type for all programs instead.
> 
> attr->prog_type is one per object, though.  How do we set that one?
Isn't it used only in __bpf_object__open_xattr to require/not-require kernel
version in the bpf prog?

It will probably work quite nicely for both of our options:

* type not specified: prog_type = BPF_PROG_TYPE_UNSPEC, need kernel
  version (over cautious?)
* type specified (and applied to all progs): using bpf_prog_type__needs_kver
  to require/not requqire kernel version

WARNING: multiple messages have this Message-ID (diff)
From: sdf at fomichev.me (Stanislav Fomichev)
Subject: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
Date: Thu, 8 Nov 2018 13:25:39 -0800	[thread overview]
Message-ID: <20181108212539.esqjw4k5hz2pxowu@mini-arch> (raw)
In-Reply-To: <20181108114544.1ac0cd44@cakuba.hsd1.ca.comcast.net>

On 11/08, Jakub Kicinski wrote:
> On Thu, 8 Nov 2018 11:16:43 +0000, Quentin Monnet wrote:
> > > -	bpf_program__set_ifindex(prog, ifindex);
> > >   	if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
> > > +		if (!prog) {
> > > +			p_err("can not guess program type when loading all programs\n");
> 
> No new lines in p_err(), beaks JSON.
Thanks, I'll probably remove that altogether and do auto inference of
prog_type from the section name here.

> > > +			goto err_close_obj;
> > > +		}
> > > +
> > >   		const char *sec_name = bpf_program__title(prog, false);
> > >   
> > >   		err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,
> > > @@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
> > >   			goto err_close_obj;
> > >   		}
> > >   	}
> > > -	bpf_program__set_type(prog, attr.prog_type);
> > > -	bpf_program__set_expected_attach_type(prog, expected_attach_type);
> > > +
> > > +	bpf_object__for_each_program(pos, obj) {
> > > +		bpf_program__set_ifindex(pos, ifindex);
> > > +		bpf_program__set_type(pos, attr.prog_type);
> > > +		bpf_program__set_expected_attach_type(pos,
> > > +						      expected_attach_type);
> > > +	}  
> > 
> > I still believe you can have programs of different types here, and be 
> > able to load them. I tried it and managed to have it working fine. If no 
> > type is provided from command line we can retrieve types for each 
> > program from its section name. If a type is provided on the command 
> > line, we can do the same, but I am not sure we should do it, or impose 
> > that type for all programs instead.
> 
> attr->prog_type is one per object, though.  How do we set that one?
Isn't it used only in __bpf_object__open_xattr to require/not-require kernel
version in the bpf prog?

It will probably work quite nicely for both of our options:

* type not specified: prog_type = BPF_PROG_TYPE_UNSPEC, need kernel
  version (over cautious?)
* type specified (and applied to all progs): using bpf_prog_type__needs_kver
  to require/not requqire kernel version

WARNING: multiple messages have this Message-ID (diff)
From: sdf@fomichev.me (Stanislav Fomichev)
Subject: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
Date: Thu, 8 Nov 2018 13:25:39 -0800	[thread overview]
Message-ID: <20181108212539.esqjw4k5hz2pxowu@mini-arch> (raw)
Message-ID: <20181108212539.8R_K1OVZV2cSArzbMpP4FN9ylKEO7KVu-8P8EsPIppU@z> (raw)
In-Reply-To: <20181108114544.1ac0cd44@cakuba.hsd1.ca.comcast.net>

On 11/08, Jakub Kicinski wrote:
> On Thu, 8 Nov 2018 11:16:43 +0000, Quentin Monnet wrote:
> > > -	bpf_program__set_ifindex(prog, ifindex);
> > >   	if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
> > > +		if (!prog) {
> > > +			p_err("can not guess program type when loading all programs\n");
> 
> No new lines in p_err(), beaks JSON.
Thanks, I'll probably remove that altogether and do auto inference of
prog_type from the section name here.

> > > +			goto err_close_obj;
> > > +		}
> > > +
> > >   		const char *sec_name = bpf_program__title(prog, false);
> > >   
> > >   		err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,
> > > @@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
> > >   			goto err_close_obj;
> > >   		}
> > >   	}
> > > -	bpf_program__set_type(prog, attr.prog_type);
> > > -	bpf_program__set_expected_attach_type(prog, expected_attach_type);
> > > +
> > > +	bpf_object__for_each_program(pos, obj) {
> > > +		bpf_program__set_ifindex(pos, ifindex);
> > > +		bpf_program__set_type(pos, attr.prog_type);
> > > +		bpf_program__set_expected_attach_type(pos,
> > > +						      expected_attach_type);
> > > +	}  
> > 
> > I still believe you can have programs of different types here, and be 
> > able to load them. I tried it and managed to have it working fine. If no 
> > type is provided from command line we can retrieve types for each 
> > program from its section name. If a type is provided on the command 
> > line, we can do the same, but I am not sure we should do it, or impose 
> > that type for all programs instead.
> 
> attr->prog_type is one per object, though.  How do we set that one?
Isn't it used only in __bpf_object__open_xattr to require/not-require kernel
version in the bpf prog?

It will probably work quite nicely for both of our options:

* type not specified: prog_type = BPF_PROG_TYPE_UNSPEC, need kernel
  version (over cautious?)
* type specified (and applied to all progs): using bpf_prog_type__needs_kver
  to require/not requqire kernel version

  reply	other threads:[~2018-11-09  7:03 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-08  5:39 [PATCH v3 bpf-next 0/4] bpftool: support loading flow dissector Stanislav Fomichev
2018-11-08  5:39 ` Stanislav Fomichev
2018-11-08  5:39 ` sdf
2018-11-08  5:39 ` [PATCH v3 bpf-next 1/4] selftests/bpf: rename flow dissector section to flow_dissector Stanislav Fomichev
2018-11-08  5:39   ` Stanislav Fomichev
2018-11-08  5:39   ` sdf
2018-11-08  5:39 ` [PATCH v3 bpf-next 2/4] libbpf: cleanup after partial failure in bpf_object__pin Stanislav Fomichev
2018-11-08  5:39   ` Stanislav Fomichev
2018-11-08  5:39   ` sdf
2018-11-08  5:39 ` [PATCH v3 bpf-next 3/4] libbpf: bpf_program__pin: add special case for instances.nr == 1 Stanislav Fomichev
2018-11-08  5:39   ` Stanislav Fomichev
2018-11-08  5:39   ` sdf
2018-11-08  5:39 ` [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector Stanislav Fomichev
2018-11-08  5:39   ` Stanislav Fomichev
2018-11-08  5:39   ` sdf
2018-11-08 11:16   ` Quentin Monnet
2018-11-08 11:16     ` Quentin Monnet
2018-11-08 11:16     ` quentin.monnet
2018-11-08 18:01     ` Stanislav Fomichev
2018-11-08 18:01       ` Stanislav Fomichev
2018-11-08 18:01       ` sdf
2018-11-08 18:21       ` Quentin Monnet
2018-11-08 18:21         ` Quentin Monnet
2018-11-08 18:21         ` quentin.monnet
2018-11-08 19:01         ` Stanislav Fomichev
2018-11-08 19:01           ` Stanislav Fomichev
2018-11-08 19:01           ` sdf
2018-11-08 19:35         ` Jakub Kicinski
2018-11-08 19:35           ` Jakub Kicinski
2018-11-08 19:35           ` jakub.kicinski
2018-11-08 21:20           ` Stanislav Fomichev
2018-11-08 21:20             ` Stanislav Fomichev
2018-11-08 21:20             ` sdf
2018-11-08 21:51             ` Jakub Kicinski
2018-11-08 21:51               ` Jakub Kicinski
2018-11-08 21:51               ` jakub.kicinski
2018-11-08 19:45     ` Jakub Kicinski
2018-11-08 19:45       ` Jakub Kicinski
2018-11-08 19:45       ` jakub.kicinski
2018-11-08 21:25       ` Stanislav Fomichev [this message]
2018-11-08 21:25         ` Stanislav Fomichev
2018-11-08 21:25         ` sdf
2018-11-08 21:52         ` Jakub Kicinski
2018-11-08 21:52           ` Jakub Kicinski
2018-11-08 21:52           ` jakub.kicinski
2018-11-08 19:46   ` Jakub Kicinski
2018-11-08 19:46     ` Jakub Kicinski
2018-11-08 19:46     ` jakub.kicinski
2018-11-08 21:29     ` Stanislav Fomichev
2018-11-08 21:29       ` Stanislav Fomichev
2018-11-08 21:29       ` sdf
2018-11-08 21:54       ` Jakub Kicinski
2018-11-08 21:54         ` Jakub Kicinski
2018-11-08 21:54         ` jakub.kicinski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181108212539.esqjw4k5hz2pxowu@mini-arch \
    --to=sdf@fomichev.me \
    --cc=ast@kernel.org \
    --cc=bhole_prashant_q7@lab.ntt.co.jp \
    --cc=daniel@iogearbox.net \
    --cc=guro@fb.com \
    --cc=jakub.kicinski@netronome.com \
    --cc=jbenc@redhat.com \
    --cc=jiong.wang@netronome.com \
    --cc=john.fastabend@gmail.com \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=osk@fb.com \
    --cc=quentin.monnet@netronome.com \
    --cc=sandipan@linux.vnet.ibm.com \
    --cc=sdf@google.com \
    --cc=shuah@kernel.org \
    --cc=treeze.taeung@gmail.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.