[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 5 May 2020 13:28:18 -0700
From: Yonghong Song <yhs@...com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
CC: Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
Martin KaFai Lau <kafai@...com>,
Networking <netdev@...r.kernel.org>,
Alexei Starovoitov <ast@...com>,
Daniel Borkmann <daniel@...earbox.net>,
Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next v2 06/20] bpf: create anonymous bpf iterator
On 5/5/20 1:11 PM, Andrii Nakryiko wrote:
> On Sun, May 3, 2020 at 11:29 PM Yonghong Song <yhs@...com> wrote:
>>
>> A new bpf command BPF_ITER_CREATE is added.
>>
>> The anonymous bpf iterator is seq_file based.
>> The seq_file private data are referenced by targets.
>> The bpf_iter infrastructure allocated additional space
>> at seq_file->private before the space used by targets
>> to store some meta data, e.g.,
>> prog: prog to run
>> session_id: an unique id for each opened seq_file
>> seq_num: how many times bpf programs are queried in this session
>> do_stop: an internal state to decide whether bpf program
>> should be called in seq_ops->stop() or not
>>
>> Signed-off-by: Yonghong Song <yhs@...com>
>> ---
>> include/linux/bpf.h | 1 +
>> include/uapi/linux/bpf.h | 6 ++
>> kernel/bpf/bpf_iter.c | 128 +++++++++++++++++++++++++++++++++
>> kernel/bpf/syscall.c | 26 +++++++
>> tools/include/uapi/linux/bpf.h | 6 ++
>> 5 files changed, 167 insertions(+)
>>
>
> [...]
>
>> /* The description below is an attempt at providing documentation to eBPF
>> diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
>> index 2674c9cbc3dc..2a9f939be6e6 100644
>> --- a/kernel/bpf/bpf_iter.c
>> +++ b/kernel/bpf/bpf_iter.c
>> @@ -2,6 +2,7 @@
>> /* Copyright (c) 2020 Facebook */
>>
>> #include <linux/fs.h>
>> +#include <linux/anon_inodes.h>
>> #include <linux/filter.h>
>> #include <linux/bpf.h>
>>
>> @@ -20,12 +21,26 @@ struct bpf_iter_link {
>> struct bpf_iter_target_info *tinfo;
>> };
>>
>> +struct bpf_iter_priv_data {
>> + struct {
>
> nit: anon struct seems unnecessary here? is it just for visual grouping?
Yes, this is just for virtual grouping. Not 100% sure whether this
is needed or not.
>
>> + struct bpf_iter_target_info *tinfo;
>> + struct bpf_prog *prog;
>> + u64 session_id;
>> + u64 seq_num;
>> + u64 do_stop;
>> + };
>> + u8 target_private[] __aligned(8);
>> +};
>> +
>> static struct list_head targets = LIST_HEAD_INIT(targets);
>> static DEFINE_MUTEX(targets_mutex);
>>
>> /* protect bpf_iter_link changes */
>> static DEFINE_MUTEX(link_mutex);
>>
>> +/* incremented on every opened seq_file */
>> +static atomic64_t session_id;
>> +
>> /* bpf_seq_read, a customized and simpler version for bpf iterator.
>> * no_llseek is assumed for this file.
>> * The following are differences from seq_read():
>> @@ -154,6 +169,31 @@ static ssize_t bpf_seq_read(struct file *file, char __user *buf, size_t size,
>> goto Done;
>> }
>>
>> +static int iter_release(struct inode *inode, struct file *file)
>> +{
>> + struct bpf_iter_priv_data *iter_priv;
>> + void *file_priv = file->private_data;
>> + struct seq_file *seq;
>> +
>> + seq = file_priv;
>
>
> seq might be NULL, if anon_inode_getfile succeeded, but then
> prepare_seq_file failed, so you need to handle that.
Thanks for catching this. Missed this case.
>
> Also, file_priv is redundant, assign to seq directly from file->private_data?
Ack.
>
>> + iter_priv = container_of(seq->private, struct bpf_iter_priv_data,
>> + target_private);
>> +
>> + if (iter_priv->tinfo->fini_seq_private)
>> + iter_priv->tinfo->fini_seq_private(seq->private);
>> +
>> + bpf_prog_put(iter_priv->prog);
>> + seq->private = iter_priv;
>> +
>> + return seq_release_private(inode, file);
>> +}
>> +
>> +static const struct file_operations bpf_iter_fops = {
>> + .llseek = no_llseek,
>> + .read = bpf_seq_read,
>> + .release = iter_release,
>> +};
>> +
>> int bpf_iter_reg_target(struct bpf_iter_reg *reg_info)
>> {
>> struct bpf_iter_target_info *tinfo;
>> @@ -289,3 +329,91 @@ int bpf_iter_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>>
>> return bpf_link_settle(&link_primer);
>> }
>> +
>> +static void init_seq_meta(struct bpf_iter_priv_data *priv_data,
>> + struct bpf_iter_target_info *tinfo,
>> + struct bpf_prog *prog)
>> +{
>> + priv_data->tinfo = tinfo;
>> + priv_data->prog = prog;
>> + priv_data->session_id = atomic64_add_return(1, &session_id);
>
> nit: atomic64_inc_return?
Ack.
>
>> + priv_data->seq_num = 0;
>> + priv_data->do_stop = 0;
>> +}
>> +
>
> [...]
>
Powered by blists - more mailing lists