* Re: [PATCH] rtnetlink: Synchronze net in rtnl_unregister()
From: Dmitry Safonov @ 2019-02-25 23:21 UTC (permalink / raw)
To: Eric Dumazet, linux-kernel
Cc: 0x7f454c46, David S. Miller, Florian Westphal,
Hannes Frederic Sowa, netdev
In-Reply-To: <eb7bca34-2b6a-1140-b230-71f333b42807@gmail.com>
Hi Eric,
On 2/25/19 11:09 PM, Eric Dumazet wrote:
> On 02/25/2019 01:27 PM, Dmitry Safonov wrote:
>> While it's possible to document that rtnl_unregister() requires
>> synchronize_net() afterwards - unlike rtnl_unregister_all(), I believe
>> the module exit is very much slow-path.
>
> rtnl_unregister_all() needs the sychronize_rcu() at this moment
> because of the kfree(tab), not because of the kfree_rcu(link, rcu);
I may be wrong here, but shouldn't we wait for grace period to elapse by
the reason that rtnl_msg_handlers are protected by RCU, not only by rtnl?
Like, without synchronize_net() in rtnl_unregister() - what prevents
module exit race to say, rtnetlink_rcv_msg()=>rtnl_get_link()?
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -308,7 +308,9 @@ int rtnl_unregister(int protocol, int msgtype)
>> rcu_assign_pointer(tab[msgindex], NULL);
>> rtnl_unlock();
>>
>> - kfree_rcu(link, rcu);
>> + synchronize_net();
>> +
>> + kfree(link);
>
>
> I really do not see a difference here (other than this being much slower of course)
>
> If the caller needs rcu_barrier(), then add it in the caller ?
Well, sure - but it seems confusing that rtnl_unregister() will require
synchronize_rcu(), while rtnl_unregister_all() will not.
And I thought no one would care about another synchronize_rcu() in exit
path.
Thanks,
Dmitry
^ permalink raw reply
* Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING
From: Cong Wang @ 2019-02-25 23:18 UTC (permalink / raw)
To: David Miller, LKML
Cc: Yafang Shao, Daniel Borkmann, Eric Dumazet, Joe Perches,
Linux Kernel Network Developers, shaoyafang
In-Reply-To: <20190225.151141.792446098735157842.davem@davemloft.net>
(Cc'ing LKML)
On Mon, Feb 25, 2019 at 3:11 PM David Miller <davem@davemloft.net> wrote:
>
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Mon, 25 Feb 2019 14:58:36 -0800
>
> > Please be specific, and ideally make it a formal doc in netdev-FAQ.txt.
>
> And this isn't happening either Cong, I reserve the right to apply my
> judgment as networking maintainer on a case by case basis as I so see
> fit.
Thanks for rephrasing non-transparency in such a wonderful way.
^ permalink raw reply
* Re: [PATCH v2 bpf-next 2/9] bpf: Add bpf helper bpf_tcp_enter_cwr
From: Stanislav Fomichev @ 2019-02-25 23:14 UTC (permalink / raw)
To: brakmo
Cc: netdev, Martin Lau, Alexei Starovoitov, Daniel Borkmann,
Eric Dumazet, Kernel Team
In-Reply-To: <20190223010703.678070-3-brakmo@fb.com>
On 02/22, brakmo wrote:
> From: Martin KaFai Lau <kafai@fb.com>
>
> This patch adds a new bpf helper BPF_FUNC_tcp_enter_cwr
> "int bpf_tcp_enter_cwr(struct bpf_tcp_sock *tp)".
> It is added to BPF_PROG_TYPE_CGROUP_SKB which can be attached
> to the egress path where the bpf prog is called by
> ip_finish_output() or ip6_finish_output(). The verifier
> ensures that the parameter must be a tcp_sock.
>
> This helper makes a tcp_sock enter CWR state. It can be used
> by a bpf_prog to manage egress network bandwidth limit per
> cgroupv2. A later patch will have a sample program to
> show how it can be used to limit bandwidth usage per cgroupv2.
>
> To ensure it is only called from BPF_CGROUP_INET_EGRESS, the
> attr->expected_attach_type must be specified as BPF_CGROUP_INET_EGRESS
> during load time if the prog uses this new helper.
> The newly added prog->enforce_expected_attach_type bit will also be set
> if this new helper is used. This bit is for backward compatibility reason
> because currently prog->expected_attach_type has been ignored in
> BPF_PROG_TYPE_CGROUP_SKB. During attach time,
> prog->expected_attach_type is only enforced if the
> prog->enforce_expected_attach_type bit is set.
> i.e. prog->expected_attach_type is only enforced if this new helper
> is used by the prog.
>
> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
> include/linux/bpf.h | 1 +
> include/linux/filter.h | 3 ++-
> include/uapi/linux/bpf.h | 9 ++++++++-
> kernel/bpf/syscall.c | 12 ++++++++++++
> kernel/bpf/verifier.c | 4 ++++
> net/core/filter.c | 25 +++++++++++++++++++++++++
> 6 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d5ba2fc01af3..2d54ba7cf9dd 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -195,6 +195,7 @@ enum bpf_arg_type {
> ARG_PTR_TO_SOCKET, /* pointer to bpf_sock */
> ARG_PTR_TO_SPIN_LOCK, /* pointer to bpf_spin_lock */
> ARG_PTR_TO_SOCK_COMMON, /* pointer to sock_common */
> + ARG_PTR_TO_TCP_SOCK, /* pointer to tcp_sock */
> };
>
> /* type of values returned from helper functions */
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index f32b3eca5a04..c6e878bdc5a6 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -510,7 +510,8 @@ struct bpf_prog {
> blinded:1, /* Was blinded */
> is_func:1, /* program is a bpf function */
> kprobe_override:1, /* Do we override a kprobe? */
> - has_callchain_buf:1; /* callchain buffer allocated? */
> + has_callchain_buf:1, /* callchain buffer allocated? */
> + enforce_expected_attach_type:1; /* Enforce expected_attach_type checking at attach time */
> enum bpf_prog_type type; /* Type of BPF program */
> enum bpf_attach_type expected_attach_type; /* For some prog types */
> u32 len; /* Number of filter blocks */
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index bcdd2474eee7..95b5058fa945 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2359,6 +2359,12 @@ union bpf_attr {
> * Return
> * A **struct bpf_tcp_sock** pointer on success, or NULL in
> * case of failure.
> + *
> + * int bpf_tcp_enter_cwr(struct bpf_tcp_sock *tp)
> + * Description
> + * Make a tcp_sock enter CWR state.
> + * Return
> + * 0 on success, or a negative error in case of failure.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2457,7 +2463,8 @@ union bpf_attr {
> FN(spin_lock), \
> FN(spin_unlock), \
> FN(sk_fullsock), \
> - FN(tcp_sock),
> + FN(tcp_sock), \
> + FN(tcp_enter_cwr),
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> * function eBPF program intends to call
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index ec7c552af76b..9a478f2875cd 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1482,6 +1482,14 @@ bpf_prog_load_check_attach_type(enum bpf_prog_type prog_type,
> default:
> return -EINVAL;
> }
> + case BPF_PROG_TYPE_CGROUP_SKB:
> + switch (expected_attach_type) {
> + case BPF_CGROUP_INET_INGRESS:
> + case BPF_CGROUP_INET_EGRESS:
> + return 0;
> + default:
> + return -EINVAL;
> + }
> default:
> return 0;
> }
> @@ -1725,6 +1733,10 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
> case BPF_PROG_TYPE_CGROUP_SOCK:
> case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
> return attach_type == prog->expected_attach_type ? 0 : -EINVAL;
> + case BPF_PROG_TYPE_CGROUP_SKB:
> + return prog->enforce_expected_attach_type &&
> + prog->expected_attach_type != attach_type ?
> + -EINVAL : 0;
> default:
> return 0;
> }
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1b9496c41383..95fb385c6f3c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2424,6 +2424,10 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
> return -EFAULT;
> }
> meta->ptr_id = reg->id;
> + } else if (arg_type == ARG_PTR_TO_TCP_SOCK) {
> + expected_type = PTR_TO_TCP_SOCK;
> + if (type != expected_type)
> + goto err_type;
> } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
> if (meta->func_id == BPF_FUNC_spin_lock) {
> if (process_spin_lock(env, regno, true))
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 97916eedfe69..ca57ef25279c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5426,6 +5426,24 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
> .arg1_type = ARG_PTR_TO_SOCK_COMMON,
> };
>
> +BPF_CALL_1(bpf_tcp_enter_cwr, struct tcp_sock *, tp)
> +{
> + struct sock *sk = (struct sock *)tp;
> +
> + if (sk->sk_state == TCP_ESTABLISHED) {
> + tcp_enter_cwr(sk);
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static const struct bpf_func_proto bpf_tcp_enter_cwr_proto = {
> + .func = bpf_tcp_enter_cwr,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_TCP_SOCK,
> +};
> #endif /* CONFIG_INET */
>
> bool bpf_helper_changes_pkt_data(void *func)
> @@ -5585,6 +5603,13 @@ cg_skb_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
> #ifdef CONFIG_INET
> case BPF_FUNC_tcp_sock:
> return &bpf_tcp_sock_proto;
[...]
> + case BPF_FUNC_tcp_enter_cwr:
> + if (prog->expected_attach_type == BPF_CGROUP_INET_EGRESS) {
> + prog->enforce_expected_attach_type = 1;
> + return &bpf_tcp_enter_cwr_proto;
Instead of this back and forth with enforce_expected_attach_type, can we
just do here:
if (prog->expected_attach_type == BPF_CGROUP_INET_EGRESS)
return &bpf_tcp_enter_cwr_proto;
else
return null;
Wouldn't it have the same effect?
> + } else {
> + return NULL;
> + }
> #endif
> default:
> return sk_filter_func_proto(func_id, prog);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING
From: David Miller @ 2019-02-25 23:11 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: laoar.shao, daniel, edumazet, joe, netdev, shaoyafang
In-Reply-To: <CAM_iQpUknnab0bwJgp3-F864-02LsH+J07c8SzMZyYK84yoezw@mail.gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 25 Feb 2019 14:58:36 -0800
> Please be specific, and ideally make it a formal doc in netdev-FAQ.txt.
And this isn't happening either Cong, I reserve the right to apply my
judgment as networking maintainer on a case by case basis as I so see
fit.
^ permalink raw reply
* Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING
From: David Miller @ 2019-02-25 23:11 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: laoar.shao, daniel, edumazet, joe, netdev, shaoyafang
In-Reply-To: <CAM_iQpUknnab0bwJgp3-F864-02LsH+J07c8SzMZyYK84yoezw@mail.gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 25 Feb 2019 14:58:36 -0800
> So you agree that I can add debugging printk's only for my own use?
Let's put it this way.
If I ignore the fact that some of the data centers with the furtest
reach on the entire planet use something that is in the tree already,
then I am being unreasonable and insensitive.
This means no adding, but keeping things we already have.
Don't try to weasel word what I'm trying to say here Cong.
Have a nice day.
^ permalink raw reply
* Re: [PATCH] rtnetlink: Synchronze net in rtnl_unregister()
From: Eric Dumazet @ 2019-02-25 23:09 UTC (permalink / raw)
To: Dmitry Safonov, linux-kernel
Cc: 0x7f454c46, David S. Miller, Florian Westphal,
Hannes Frederic Sowa, netdev
In-Reply-To: <20190225212721.2908-1-dima@arista.com>
On 02/25/2019 01:27 PM, Dmitry Safonov wrote:
> rtnl_unregister() unsets handler from table, which is protected
> by rtnl_lock or RCU. At this moment only dump handlers access the table
> with rcu_lock(). Every other user accesses under rtnl.
>
> Callers may expect that rtnl_unregister() prevents any further handlers
> calls, alike rtnl_unregister_all(). And they do expect it.
>
> I've looked on in-tree caller uses:
> br_mdb: safe, but in err-path br_netlink_init()
> fib_rules: safe - err-path is very early in __init
> ip6mr: safe - following unregister_pernet_subsys() calls internally rcu_barrier()
> qrtr: safe - following sock_unregister() calls internally synchronize_rcu()
If rcu_barrier() was needed, then all callers should use it.
If synchronize_rcu() was needed, then all callers should use it.
But mixing is probably wrong.
>
> While it's possible to document that rtnl_unregister() requires
> synchronize_net() afterwards - unlike rtnl_unregister_all(), I believe
> the module exit is very much slow-path.
rtnl_unregister_all() needs the sychronize_rcu() at this moment
because of the kfree(tab), not because of the kfree_rcu(link, rcu);
>
> Issue seems to be very theoretical and unlikely, so I'm not Cc'ing
> stable tree.
>
> Fixes: 6853dd488119 ("rtnetlink: protect handler table with rcu")
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Florian Westphal <fw@strlen.de>
> Cc: "Hannes Frederic Sowa" <hannes@stressinduktion.org>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Dmitry Safonov <dima@arista.com>
> ---
> net/core/rtnetlink.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 5ea1bed08ede..3db70da4f951 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -308,7 +308,9 @@ int rtnl_unregister(int protocol, int msgtype)
> rcu_assign_pointer(tab[msgindex], NULL);
> rtnl_unlock();
>
> - kfree_rcu(link, rcu);
> + synchronize_net();
> +
> + kfree(link);
I really do not see a difference here (other than this being much slower of course)
If the caller needs rcu_barrier(), then add it in the caller ?
^ permalink raw reply
* Re: [PATCH v3 bpf-next 1/4] bpf: enable program stats
From: Stanislav Fomichev @ 2019-02-25 23:07 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: davem, daniel, edumazet, netdev, bpf, kernel-team
In-Reply-To: <20190225222842.2031962-2-ast@kernel.org>
On 02/25, Alexei Starovoitov wrote:
> JITed BPF programs are indistinguishable from kernel functions, but unlike
> kernel code BPF code can be changed often.
> Typical approach of "perf record" + "perf report" profiling and tuning of
> kernel code works just as well for BPF programs, but kernel code doesn't
> need to be monitored whereas BPF programs do.
> Users load and run large amount of BPF programs.
> These BPF stats allow tools monitor the usage of BPF on the server.
> The monitoring tools will turn sysctl kernel.bpf_stats_enabled
> on and off for few seconds to sample average cost of the programs.
> Aggregated data over hours and days will provide an insight into cost of BPF
> and alarms can trigger in case given program suddenly gets more expensive.
>
> The cost of two sched_clock() per program invocation adds ~20 nsec.
> Fast BPF progs (like selftests/bpf/progs/test_pkt_access.c) will slow down
> from ~10 nsec to ~30 nsec.
> static_key minimizes the cost of the stats collection.
> There is no measurable difference before/after this patch
> with kernel.bpf_stats_enabled=0
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
> include/linux/bpf.h | 9 +++++++++
> include/linux/filter.h | 20 +++++++++++++++++++-
> kernel/bpf/core.c | 31 +++++++++++++++++++++++++++++--
> kernel/bpf/syscall.c | 34 ++++++++++++++++++++++++++++++++--
> kernel/bpf/verifier.c | 7 ++++++-
> kernel/sysctl.c | 34 ++++++++++++++++++++++++++++++++++
> 6 files changed, 129 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index de18227b3d95..a2132e09dc1c 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -16,6 +16,7 @@
> #include <linux/rbtree_latch.h>
> #include <linux/numa.h>
> #include <linux/wait.h>
> +#include <linux/u64_stats_sync.h>
>
> struct bpf_verifier_env;
> struct perf_event;
> @@ -340,6 +341,12 @@ enum bpf_cgroup_storage_type {
>
> #define MAX_BPF_CGROUP_STORAGE_TYPE __BPF_CGROUP_STORAGE_MAX
>
> +struct bpf_prog_stats {
> + u64 cnt;
> + u64 nsecs;
> + struct u64_stats_sync syncp;
> +};
> +
> struct bpf_prog_aux {
> atomic_t refcnt;
> u32 used_map_cnt;
> @@ -389,6 +396,7 @@ struct bpf_prog_aux {
> * main prog always has linfo_idx == 0
> */
> u32 linfo_idx;
> + struct bpf_prog_stats __percpu *stats;
> union {
> struct work_struct work;
> struct rcu_head rcu;
> @@ -559,6 +567,7 @@ void bpf_map_area_free(void *base);
> void bpf_map_init_from_attr(struct bpf_map *map, union bpf_attr *attr);
>
> extern int sysctl_unprivileged_bpf_disabled;
> +extern int sysctl_bpf_stats_enabled;
>
> int bpf_map_new_fd(struct bpf_map *map, int flags);
> int bpf_prog_new_fd(struct bpf_prog *prog);
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index f32b3eca5a04..7e5e3db11106 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -533,7 +533,24 @@ struct sk_filter {
> struct bpf_prog *prog;
> };
>
> -#define BPF_PROG_RUN(filter, ctx) ({ cant_sleep(); (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
> +DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
> +
> +#define BPF_PROG_RUN(prog, ctx) ({ \
> + u32 ret; \
> + cant_sleep(); \
> + if (static_branch_unlikely(&bpf_stats_enabled_key)) { \
> + struct bpf_prog_stats *stats; \
> + u64 start = sched_clock(); \
QQ: why sched_clock() and not, for example, ktime_get_ns() which we do
in the bpf_test_run()? Or even why not local_clock?
I'm just wondering what king of trade off we are doing here
regarding precision vs run time cost.
> + ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
> + stats = this_cpu_ptr(prog->aux->stats); \
> + u64_stats_update_begin(&stats->syncp); \
> + stats->cnt++; \
> + stats->nsecs += sched_clock() - start; \
> + u64_stats_update_end(&stats->syncp); \
> + } else { \
> + ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
> + } \
> + ret; })
>
> #define BPF_SKB_CB_LEN QDISC_CB_PRIV_LEN
>
> @@ -764,6 +781,7 @@ void bpf_prog_free_jited_linfo(struct bpf_prog *prog);
> void bpf_prog_free_unused_jited_linfo(struct bpf_prog *prog);
>
> struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags);
> +struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flags);
> struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size,
> gfp_t gfp_extra_flags);
> void __bpf_prog_free(struct bpf_prog *fp);
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index ef88b167959d..1c14c347f3cf 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -78,7 +78,7 @@ void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, uns
> return NULL;
> }
>
> -struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags)
> +struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flags)
> {
> gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags;
> struct bpf_prog_aux *aux;
> @@ -104,6 +104,26 @@ struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags)
>
> return fp;
> }
> +
> +struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags)
> +{
> + gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags;
> + struct bpf_prog *prog;
> +
> + prog = bpf_prog_alloc_no_stats(size, gfp_extra_flags);
> + if (!prog)
> + return NULL;
> +
> + prog->aux->stats = alloc_percpu_gfp(struct bpf_prog_stats, gfp_flags);
> + if (!prog->aux->stats) {
> + kfree(prog->aux);
> + vfree(prog);
> + return NULL;
> + }
> +
> + u64_stats_init(&prog->aux->stats->syncp);
> + return prog;
> +}
> EXPORT_SYMBOL_GPL(bpf_prog_alloc);
>
> int bpf_prog_alloc_jited_linfo(struct bpf_prog *prog)
> @@ -231,7 +251,10 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size,
>
> void __bpf_prog_free(struct bpf_prog *fp)
> {
> - kfree(fp->aux);
> + if (fp->aux) {
> + free_percpu(fp->aux->stats);
> + kfree(fp->aux);
> + }
> vfree(fp);
> }
>
> @@ -2069,6 +2092,10 @@ int __weak skb_copy_bits(const struct sk_buff *skb, int offset, void *to,
> return -EFAULT;
> }
>
> +DEFINE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
> +EXPORT_SYMBOL(bpf_stats_enabled_key);
> +int sysctl_bpf_stats_enabled __read_mostly;
> +
> /* All definitions of tracepoints related to BPF. */
> #define CREATE_TRACE_POINTS
> #include <linux/bpf_trace.h>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index ec7c552af76b..31cf66fc3f5c 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1283,24 +1283,54 @@ static int bpf_prog_release(struct inode *inode, struct file *filp)
> return 0;
> }
>
> +static void bpf_prog_get_stats(const struct bpf_prog *prog,
> + struct bpf_prog_stats *stats)
> +{
> + u64 nsecs = 0, cnt = 0;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + const struct bpf_prog_stats *st;
> + unsigned int start;
> + u64 tnsecs, tcnt;
> +
> + st = per_cpu_ptr(prog->aux->stats, cpu);
> + do {
> + start = u64_stats_fetch_begin_irq(&st->syncp);
> + tnsecs = st->nsecs;
> + tcnt = st->cnt;
> + } while (u64_stats_fetch_retry_irq(&st->syncp, start));
> + nsecs += tnsecs;
> + cnt += tcnt;
> + }
> + stats->nsecs = nsecs;
> + stats->cnt = cnt;
> +}
> +
> #ifdef CONFIG_PROC_FS
> static void bpf_prog_show_fdinfo(struct seq_file *m, struct file *filp)
> {
> const struct bpf_prog *prog = filp->private_data;
> char prog_tag[sizeof(prog->tag) * 2 + 1] = { };
> + struct bpf_prog_stats stats;
>
> + bpf_prog_get_stats(prog, &stats);
> bin2hex(prog_tag, prog->tag, sizeof(prog->tag));
> seq_printf(m,
> "prog_type:\t%u\n"
> "prog_jited:\t%u\n"
> "prog_tag:\t%s\n"
> "memlock:\t%llu\n"
> - "prog_id:\t%u\n",
> + "prog_id:\t%u\n"
> + "run_time_ns:\t%llu\n"
> + "run_cnt:\t%llu\n",
> prog->type,
> prog->jited,
> prog_tag,
> prog->pages * 1ULL << PAGE_SHIFT,
> - prog->aux->id);
> + prog->aux->id,
> + stats.nsecs,
> + stats.cnt);
> }
> #endif
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1b9496c41383..0e4edd7e3c5f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7320,7 +7320,12 @@ static int jit_subprogs(struct bpf_verifier_env *env)
> subprog_end = env->subprog_info[i + 1].start;
>
> len = subprog_end - subprog_start;
> - func[i] = bpf_prog_alloc(bpf_prog_size(len), GFP_USER);
> + /* BPF_PROG_RUN doesn't call subprogs directly,
> + * hence main prog stats include the runtime of subprogs.
> + * subprogs don't have IDs and not reachable via prog_get_next_id
> + * func[i]->aux->stats will never be accessed and stays NULL
> + */
> + func[i] = bpf_prog_alloc_no_stats(bpf_prog_size(len), GFP_USER);
> if (!func[i])
> goto out_free;
> memcpy(func[i]->insnsi, &prog->insnsi[subprog_start],
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index ba4d9e85feb8..86e0771352f2 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -224,6 +224,9 @@ static int proc_dostring_coredump(struct ctl_table *table, int write,
> #endif
> static int proc_dopipe_max_size(struct ctl_table *table, int write,
> void __user *buffer, size_t *lenp, loff_t *ppos);
> +static int proc_dointvec_minmax_bpf_stats(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp,
> + loff_t *ppos);
>
> #ifdef CONFIG_MAGIC_SYSRQ
> /* Note: sysrq code uses its own private copy */
> @@ -1230,6 +1233,15 @@ static struct ctl_table kern_table[] = {
> .extra2 = &one,
> },
> #endif
> + {
> + .procname = "bpf_stats_enabled",
> + .data = &sysctl_bpf_stats_enabled,
> + .maxlen = sizeof(sysctl_bpf_stats_enabled),
> + .mode = 0644,
> + .proc_handler = proc_dointvec_minmax_bpf_stats,
> + .extra1 = &zero,
> + .extra2 = &one,
> + },
> #if defined(CONFIG_TREE_RCU) || defined(CONFIG_PREEMPT_RCU)
> {
> .procname = "panic_on_rcu_stall",
> @@ -3260,6 +3272,28 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
>
> #endif /* CONFIG_PROC_SYSCTL */
>
> +static int proc_dointvec_minmax_bpf_stats(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp,
> + loff_t *ppos)
> +{
> + int ret, bpf_stats = *(int *)table->data;
> + struct ctl_table tmp = *table;
> +
> + if (write && !capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + tmp.data = &bpf_stats;
> + ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> + if (write && !ret) {
> + *(int *)table->data = bpf_stats;
> + if (bpf_stats)
> + static_branch_enable(&bpf_stats_enabled_key);
> + else
> + static_branch_disable(&bpf_stats_enabled_key);
> + }
> + return ret;
> +}
> +
> /*
> * No sense putting this after each symbol definition, twice,
> * exception granted :-)
> --
> 2.20.0
>
^ permalink raw reply
* Re: [PATCH bpf-next v6 0/3] libbpf: adding AF_XDP support
From: Daniel Borkmann @ 2019-02-25 22:59 UTC (permalink / raw)
To: Magnus Karlsson, bjorn.topel, ast, netdev, jakub.kicinski,
bjorn.topel, qi.z.zhang
Cc: brouer, xiaolong.ye
In-Reply-To: <1550740888-26439-1-git-send-email-magnus.karlsson@intel.com>
On 02/21/2019 10:21 AM, Magnus Karlsson wrote:
> This patch proposes to add AF_XDP support to libbpf. The main reason
> for this is to facilitate writing applications that use AF_XDP by
> offering higher-level APIs that hide many of the details of the AF_XDP
> uapi. This is in the same vein as libbpf facilitates XDP adoption by
> offering easy-to-use higher level interfaces of XDP
> functionality. Hopefully this will facilitate adoption of AF_XDP, make
> applications using it simpler and smaller, and finally also make it
> possible for applications to benefit from optimizations in the AF_XDP
> user space access code. Previously, people just copied and pasted the
> code from the sample application into their application, which is not
> desirable.
>
> The proposed interface is composed of two parts:
>
> * Low-level access interface to the four rings and the packet
> * High-level control plane interface for creating and setting up umems
> and AF_XDP sockets. This interface also loads a simple XDP program
> that routes all traffic on a queue up to the AF_XDP socket.
>
> The sample program has been updated to use this new interface and in
> that process it lost roughly 300 lines of code. I cannot detect any
> performance degradations due to the use of this library instead of the
> previous functions that were inlined in the sample application. But I
> did measure this on a slower machine and not the Broadwell that we
> normally use.
>
> The rings are now called xsk_ring and when a producer operates on
> it. It is xsk_ring_prod and for a consumer it is xsk_ring_cons. This
> way we can get some compile time error checking that the rings are
> used correctly.
>
> Comments and contenplations:
>
> * The current behaviour is that the library loads an XDP program (if
> requested to do so) but the clean up of this program is left to the
> application. It would be possible to implement this cleanup in the
> library, but it would require state to be kept on netdev level,
> which there is none at the moment, and the synchronization of this
> between processes. All this adding complexity. But when we get an
> XDP program per queue id, then it becomes trivial to also remove the
> XDP program when the application exits. This proposal from Jesper,
> Björn and others will also improve the performance of libbpf, since
> most of the XDP program code can be removed when that feature is
> supported.
>
> * In a future release, I am planning on adding a higher level data
> plane interface too. This will be based around recvmsg and sendmsg
> with the use of struct iovec for batching, without the user having
> to know anything about the underlying four rings of an AF_XDP
> socket. There will be one semantic difference though from the
> standard recvmsg and that is that the kernel will fill in the iovecs
> instead of the application. But the rest should be the same as the
> libc versions so that application writers feel at home.
>
> Patch 1: adds AF_XDP support in libbpf
> Patch 2: updates the xdpsock sample application to use the libbpf functions
> Patch 3: Documentation update to help first time users
>
> Changes v5 to v6:
> * Fixed prog_fd bug found by Xiaolong Ye. Thanks!
> Changes v4 to v5:
> * Added a FAQ to the documentation
> * Removed xsk_umem__get_data and renamed xsk_umem__get_dat_raw to
> xsk_umem__get_data
> * Replaced the netlink code with bpf_get_link_xdp_id()
> * Dynamic allocation of the map sizes. They are now sized after
> the max number of queueus on the netdev in question.
Looks better, I've applied it to bpf-next, thanks!
^ permalink raw reply
* Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING
From: Cong Wang @ 2019-02-25 22:58 UTC (permalink / raw)
To: David Miller
Cc: Yafang Shao, Daniel Borkmann, Eric Dumazet, Joe Perches,
Linux Kernel Network Developers, shaoyafang
In-Reply-To: <20190225.142939.803768996918170563.davem@davemloft.net>
On Mon, Feb 25, 2019 at 2:29 PM David Miller <davem@davemloft.net> wrote:
>
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Mon, 25 Feb 2019 14:00:09 -0800
>
> > Just to clarify, I have been suggesting to completely remove
> > this unused macro, never suggest to just undefine it in-tree.
> >
> > There is no reason to keep it in-tree, whether defined or undefined,
> > just for downstream users.
>
> And this is where you and I fundamentally disagree.
So you agree that I can add debugging printk's only for my own use?
I can claim that I only use them downstream and you can't force me
to carry local changes?
If not, what is your criteria for accepting debugging printk's? Whose
can be accepted and whose can't?
Please be specific, and ideally make it a formal doc in netdev-FAQ.txt.
Thanks!
^ permalink raw reply
* Re: [PATCH net-next] net: sched: set dedicated tcf_walker flag when tp is empty
From: Cong Wang @ 2019-02-25 22:52 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <20190225153831.10037-1-vladbu@mellanox.com>
On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
> at least once is unreliable. Some classifiers set 'stop' flag on error
> before calling walker callback, other classifiers used to call it with NULL
> filter pointer when empty. In order to prevent further regressions, extend
> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
> tcf_walker->fn() implementation that is used to check if classifier has
> filters configured.
So, after this patch commits like 31a998487641 ("net: sched: fw: don't
set arg->stop in fw_walk() when empty") can be reverted??
>
> Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
> include/net/pkt_cls.h | 1 +
> net/sched/cls_api.c | 13 +++++++++----
> 2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 232f801f2a21..422dd8800478 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -17,6 +17,7 @@ struct tcf_walker {
> int stop;
> int skip;
> int count;
> + bool nonempty;
> unsigned long cookie;
> int (*fn)(struct tcf_proto *, void *node, struct tcf_walker *);
> };
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index e2c888961379..3543be31d400 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -238,18 +238,23 @@ static void tcf_proto_put(struct tcf_proto *tp, bool rtnl_held,
> tcf_proto_destroy(tp, rtnl_held, extack);
> }
>
> -static int walker_noop(struct tcf_proto *tp, void *d, struct tcf_walker *arg)
> +static int walker_check_empty(struct tcf_proto *tp, void *d,
> + struct tcf_walker *arg)
> {
> - return -1;
> + if (tp) {
> + arg->nonempty = true;
> + return -1;
> + }
> + return 0;
How does this even work? If we can simply check tp!=NULL as
non-empty, why do we even need a walker??
For me, it must be pushed down to each implementation to
determine how it is empty.
^ permalink raw reply
* Re: [PATCH net-next] net: wan: z85230: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: David Miller @ 2019-02-25 22:39 UTC (permalink / raw)
To: albin_yang; +Cc: netdev, yang.wei9
In-Reply-To: <1551107184-16755-1-git-send-email-albin_yang@163.com>
From: Yang Wei <albin_yang@163.com>
Date: Mon, 25 Feb 2019 23:06:24 +0800
> From: Yang Wei <yang.wei9@zte.com.cn>
>
> dev_consume_skb_irq() should be called in z8530_tx_done() when skb
> xmit done. It makes drop profiles(dropwatch, perf) more friendly.
>
> Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] net: wan: cosa: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: David Miller @ 2019-02-25 22:39 UTC (permalink / raw)
To: albin_yang; +Cc: netdev, kas, yang.wei9
In-Reply-To: <1551107141-16702-1-git-send-email-albin_yang@163.com>
From: Yang Wei <albin_yang@163.com>
Date: Mon, 25 Feb 2019 23:05:41 +0800
> From: Yang Wei <yang.wei9@zte.com.cn>
>
> dev_consume_skb_irq() should be called in cosa_net_tx_done() when skb
> xmit done. It makes drop profiles(dropwatch, perf) more friendly.
>
> Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] net: wan: sbni: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: David Miller @ 2019-02-25 22:39 UTC (permalink / raw)
To: albin_yang; +Cc: netdev, yang.wei9
In-Reply-To: <1551107020-16629-1-git-send-email-albin_yang@163.com>
From: Yang Wei <albin_yang@163.com>
Date: Mon, 25 Feb 2019 23:03:40 +0800
> From: Yang Wei <yang.wei9@zte.com.cn>
>
> dev_consume_skb_irq() should be called in send_complete() when skb
> xmit done. It makes drop profiles(dropwatch, perf) more friendly.
>
> Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 01/12] net: sched: flower: don't check for rtnl on head dereference
From: Cong Wang @ 2019-02-25 22:39 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <vbf36obu9y9.fsf@mellanox.com>
On Mon, Feb 25, 2019 at 8:11 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
>
> On Fri 22 Feb 2019 at 19:32, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > So if it is no longer RCU any more, why do you still use
> > rcu_dereference_protected()? That is, why not just deref it as a raw
> > pointer?
Any answer for this question?
> >
> > And, I don't think I can buy your argument here. The RCU infrastructure
> > should not be changed even after your patches, the fast path is still
> > protocted by RCU read lock, while the slow path now is protected by
> > some smaller-scope locks. What makes cls_flower so unique that
> > it doesn't even need RCU here? tp->root is not reassigned but it is still
> > freed via RCU infra, that is in fl_destroy_sleepable().
> >
> > Thanks.
>
> My cls API patch set introduced reference counting for tcf_proto
> structure. With that change tp->ops->destroy() (which calls fl_destroy()
> and fl_destroy_sleepable(), in case of flower classifier) is only called
> after last reference to tp is released. All slow path users of tp->ops
> must obtain reference to tp, so concurrent call to fl_destroy() is not
> possible. Before this change tcf_proto structure didn't have reference
> counting support and required users to obtain rtnl mutex before calling
> its ops callbacks. This was verified in flower by using rtnl_dereference
> to obtain tp->root.
Yes, but fast path doesn't hold a refnct of tp, does it? If not, you still
rely on RCU for sync with readers. If yes, then probably RCU can be
gone.
Now you are in a middle of the two, that is taking RCU read lock on
fast path without a refcnt, meanwhile still uses rcu_dereference on
slow paths without any lock.
For me, you at least don't use the RCU API correctly here.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: wan: ixp4xx_hss: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: David Miller @ 2019-02-25 22:39 UTC (permalink / raw)
To: albin_yang; +Cc: netdev, khalasa, yang.wei9
In-Reply-To: <1551106977-16579-1-git-send-email-albin_yang@163.com>
From: Yang Wei <albin_yang@163.com>
Date: Mon, 25 Feb 2019 23:02:57 +0800
> From: Yang Wei <yang.wei9@zte.com.cn>
>
> dev_consume_skb_irq() should be called in hss_hdlc_txdone_irq() when
> skb xmit done. It makes drop profiles(dropwatch, perf) more friendly.
>
> Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] net: wan: wanxl: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: David Miller @ 2019-02-25 22:39 UTC (permalink / raw)
To: albin_yang; +Cc: netdev, khc, yang.wei9
In-Reply-To: <1551106910-16515-1-git-send-email-albin_yang@163.com>
From: Yang Wei <albin_yang@163.com>
Date: Mon, 25 Feb 2019 23:01:50 +0800
> From: Yang Wei <yang.wei9@zte.com.cn>
>
> dev_consume_skb_irq() should be called in wanxl_tx_intr() when skb
> xmit done. It makes drop profiles(dropwatch, perf) more friendly.
>
> Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] net: lmc: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: David Miller @ 2019-02-25 22:39 UTC (permalink / raw)
To: albin_yang; +Cc: netdev, colin.king, yang.wei9
In-Reply-To: <1551106660-16428-1-git-send-email-albin_yang@163.com>
From: Yang Wei <albin_yang@163.com>
Date: Mon, 25 Feb 2019 22:57:40 +0800
> From: Yang Wei <yang.wei9@zte.com.cn>
>
> dev_consume_skb_irq() should be called in lmc_interrupt() when skb
> xmit done. It makes drop profiles(dropwatch, perf) more friendly.
>
> Delete a redundant comment line in lmc_interrupt().
>
> Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
Applied.
^ permalink raw reply
* Re: [PATCH v2 2/2] NETWORKING: avoid use IPCB in cipso_v4_error
From: David Miller @ 2019-02-25 22:34 UTC (permalink / raw)
To: s-nazarov; +Cc: paul, netdev, linux-security-module, kuznet, yoshfuji
In-Reply-To: <3666661551112035@myt2-dc4bba9bb23c.qloud-c.yandex.net>
From: Nazarov Sergey <s-nazarov@yandex.ru>
Date: Mon, 25 Feb 2019 19:27:15 +0300
> Extract IP options in cipso_v4_error and use __icmp_send.
>
> Signed-off-by: Sergey Nazarov <s-nazarov@yandex.ru>
Applied.
^ permalink raw reply
* Re: [PATCH v2 1/2] NETWORKING: avoid use IPCB in cipso_v4_error
From: David Miller @ 2019-02-25 22:33 UTC (permalink / raw)
To: s-nazarov; +Cc: paul, netdev, linux-security-module, kuznet, yoshfuji
In-Reply-To: <52295911551111855@myt5-f1576e7b5bad.qloud-c.yandex.net>
From: Nazarov Sergey <s-nazarov@yandex.ru>
Date: Mon, 25 Feb 2019 19:24:15 +0300
> Add __icmp_send function having ip_options struct parameter
>
> Signed-off-by: Sergey Nazarov <s-nazarov@yandex.ru>
Applied with Subject line fixes up. This commit doesn't even make
changes to cipse_v4_error().
Anyone who ACK'd this change or added their Reviewed-by did not read
this email and are just rubber stamping crap.
^ permalink raw reply
* Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING
From: David Miller @ 2019-02-25 22:29 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: laoar.shao, daniel, edumazet, joe, netdev, shaoyafang
In-Reply-To: <CAM_iQpUePM2L5qStoq7XmQe-4sDrM9n6BRwa=FShfzZda=Lccw@mail.gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 25 Feb 2019 14:00:09 -0800
> Just to clarify, I have been suggesting to completely remove
> this unused macro, never suggest to just undefine it in-tree.
>
> There is no reason to keep it in-tree, whether defined or undefined,
> just for downstream users.
And this is where you and I fundamentally disagree.
^ permalink raw reply
* [PATCH v3 bpf-next 1/4] bpf: enable program stats
From: Alexei Starovoitov @ 2019-02-25 22:28 UTC (permalink / raw)
To: davem; +Cc: daniel, edumazet, netdev, bpf, kernel-team
In-Reply-To: <20190225222842.2031962-1-ast@kernel.org>
JITed BPF programs are indistinguishable from kernel functions, but unlike
kernel code BPF code can be changed often.
Typical approach of "perf record" + "perf report" profiling and tuning of
kernel code works just as well for BPF programs, but kernel code doesn't
need to be monitored whereas BPF programs do.
Users load and run large amount of BPF programs.
These BPF stats allow tools monitor the usage of BPF on the server.
The monitoring tools will turn sysctl kernel.bpf_stats_enabled
on and off for few seconds to sample average cost of the programs.
Aggregated data over hours and days will provide an insight into cost of BPF
and alarms can trigger in case given program suddenly gets more expensive.
The cost of two sched_clock() per program invocation adds ~20 nsec.
Fast BPF progs (like selftests/bpf/progs/test_pkt_access.c) will slow down
from ~10 nsec to ~30 nsec.
static_key minimizes the cost of the stats collection.
There is no measurable difference before/after this patch
with kernel.bpf_stats_enabled=0
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
include/linux/bpf.h | 9 +++++++++
include/linux/filter.h | 20 +++++++++++++++++++-
kernel/bpf/core.c | 31 +++++++++++++++++++++++++++++--
kernel/bpf/syscall.c | 34 ++++++++++++++++++++++++++++++++--
kernel/bpf/verifier.c | 7 ++++++-
kernel/sysctl.c | 34 ++++++++++++++++++++++++++++++++++
6 files changed, 129 insertions(+), 6 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index de18227b3d95..a2132e09dc1c 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -16,6 +16,7 @@
#include <linux/rbtree_latch.h>
#include <linux/numa.h>
#include <linux/wait.h>
+#include <linux/u64_stats_sync.h>
struct bpf_verifier_env;
struct perf_event;
@@ -340,6 +341,12 @@ enum bpf_cgroup_storage_type {
#define MAX_BPF_CGROUP_STORAGE_TYPE __BPF_CGROUP_STORAGE_MAX
+struct bpf_prog_stats {
+ u64 cnt;
+ u64 nsecs;
+ struct u64_stats_sync syncp;
+};
+
struct bpf_prog_aux {
atomic_t refcnt;
u32 used_map_cnt;
@@ -389,6 +396,7 @@ struct bpf_prog_aux {
* main prog always has linfo_idx == 0
*/
u32 linfo_idx;
+ struct bpf_prog_stats __percpu *stats;
union {
struct work_struct work;
struct rcu_head rcu;
@@ -559,6 +567,7 @@ void bpf_map_area_free(void *base);
void bpf_map_init_from_attr(struct bpf_map *map, union bpf_attr *attr);
extern int sysctl_unprivileged_bpf_disabled;
+extern int sysctl_bpf_stats_enabled;
int bpf_map_new_fd(struct bpf_map *map, int flags);
int bpf_prog_new_fd(struct bpf_prog *prog);
diff --git a/include/linux/filter.h b/include/linux/filter.h
index f32b3eca5a04..7e5e3db11106 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -533,7 +533,24 @@ struct sk_filter {
struct bpf_prog *prog;
};
-#define BPF_PROG_RUN(filter, ctx) ({ cant_sleep(); (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
+DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
+
+#define BPF_PROG_RUN(prog, ctx) ({ \
+ u32 ret; \
+ cant_sleep(); \
+ if (static_branch_unlikely(&bpf_stats_enabled_key)) { \
+ struct bpf_prog_stats *stats; \
+ u64 start = sched_clock(); \
+ ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
+ stats = this_cpu_ptr(prog->aux->stats); \
+ u64_stats_update_begin(&stats->syncp); \
+ stats->cnt++; \
+ stats->nsecs += sched_clock() - start; \
+ u64_stats_update_end(&stats->syncp); \
+ } else { \
+ ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
+ } \
+ ret; })
#define BPF_SKB_CB_LEN QDISC_CB_PRIV_LEN
@@ -764,6 +781,7 @@ void bpf_prog_free_jited_linfo(struct bpf_prog *prog);
void bpf_prog_free_unused_jited_linfo(struct bpf_prog *prog);
struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags);
+struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flags);
struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size,
gfp_t gfp_extra_flags);
void __bpf_prog_free(struct bpf_prog *fp);
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ef88b167959d..1c14c347f3cf 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -78,7 +78,7 @@ void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, uns
return NULL;
}
-struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags)
+struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flags)
{
gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags;
struct bpf_prog_aux *aux;
@@ -104,6 +104,26 @@ struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags)
return fp;
}
+
+struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags)
+{
+ gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags;
+ struct bpf_prog *prog;
+
+ prog = bpf_prog_alloc_no_stats(size, gfp_extra_flags);
+ if (!prog)
+ return NULL;
+
+ prog->aux->stats = alloc_percpu_gfp(struct bpf_prog_stats, gfp_flags);
+ if (!prog->aux->stats) {
+ kfree(prog->aux);
+ vfree(prog);
+ return NULL;
+ }
+
+ u64_stats_init(&prog->aux->stats->syncp);
+ return prog;
+}
EXPORT_SYMBOL_GPL(bpf_prog_alloc);
int bpf_prog_alloc_jited_linfo(struct bpf_prog *prog)
@@ -231,7 +251,10 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size,
void __bpf_prog_free(struct bpf_prog *fp)
{
- kfree(fp->aux);
+ if (fp->aux) {
+ free_percpu(fp->aux->stats);
+ kfree(fp->aux);
+ }
vfree(fp);
}
@@ -2069,6 +2092,10 @@ int __weak skb_copy_bits(const struct sk_buff *skb, int offset, void *to,
return -EFAULT;
}
+DEFINE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
+EXPORT_SYMBOL(bpf_stats_enabled_key);
+int sysctl_bpf_stats_enabled __read_mostly;
+
/* All definitions of tracepoints related to BPF. */
#define CREATE_TRACE_POINTS
#include <linux/bpf_trace.h>
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ec7c552af76b..31cf66fc3f5c 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1283,24 +1283,54 @@ static int bpf_prog_release(struct inode *inode, struct file *filp)
return 0;
}
+static void bpf_prog_get_stats(const struct bpf_prog *prog,
+ struct bpf_prog_stats *stats)
+{
+ u64 nsecs = 0, cnt = 0;
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ const struct bpf_prog_stats *st;
+ unsigned int start;
+ u64 tnsecs, tcnt;
+
+ st = per_cpu_ptr(prog->aux->stats, cpu);
+ do {
+ start = u64_stats_fetch_begin_irq(&st->syncp);
+ tnsecs = st->nsecs;
+ tcnt = st->cnt;
+ } while (u64_stats_fetch_retry_irq(&st->syncp, start));
+ nsecs += tnsecs;
+ cnt += tcnt;
+ }
+ stats->nsecs = nsecs;
+ stats->cnt = cnt;
+}
+
#ifdef CONFIG_PROC_FS
static void bpf_prog_show_fdinfo(struct seq_file *m, struct file *filp)
{
const struct bpf_prog *prog = filp->private_data;
char prog_tag[sizeof(prog->tag) * 2 + 1] = { };
+ struct bpf_prog_stats stats;
+ bpf_prog_get_stats(prog, &stats);
bin2hex(prog_tag, prog->tag, sizeof(prog->tag));
seq_printf(m,
"prog_type:\t%u\n"
"prog_jited:\t%u\n"
"prog_tag:\t%s\n"
"memlock:\t%llu\n"
- "prog_id:\t%u\n",
+ "prog_id:\t%u\n"
+ "run_time_ns:\t%llu\n"
+ "run_cnt:\t%llu\n",
prog->type,
prog->jited,
prog_tag,
prog->pages * 1ULL << PAGE_SHIFT,
- prog->aux->id);
+ prog->aux->id,
+ stats.nsecs,
+ stats.cnt);
}
#endif
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1b9496c41383..0e4edd7e3c5f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7320,7 +7320,12 @@ static int jit_subprogs(struct bpf_verifier_env *env)
subprog_end = env->subprog_info[i + 1].start;
len = subprog_end - subprog_start;
- func[i] = bpf_prog_alloc(bpf_prog_size(len), GFP_USER);
+ /* BPF_PROG_RUN doesn't call subprogs directly,
+ * hence main prog stats include the runtime of subprogs.
+ * subprogs don't have IDs and not reachable via prog_get_next_id
+ * func[i]->aux->stats will never be accessed and stays NULL
+ */
+ func[i] = bpf_prog_alloc_no_stats(bpf_prog_size(len), GFP_USER);
if (!func[i])
goto out_free;
memcpy(func[i]->insnsi, &prog->insnsi[subprog_start],
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ba4d9e85feb8..86e0771352f2 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -224,6 +224,9 @@ static int proc_dostring_coredump(struct ctl_table *table, int write,
#endif
static int proc_dopipe_max_size(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos);
+static int proc_dointvec_minmax_bpf_stats(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp,
+ loff_t *ppos);
#ifdef CONFIG_MAGIC_SYSRQ
/* Note: sysrq code uses its own private copy */
@@ -1230,6 +1233,15 @@ static struct ctl_table kern_table[] = {
.extra2 = &one,
},
#endif
+ {
+ .procname = "bpf_stats_enabled",
+ .data = &sysctl_bpf_stats_enabled,
+ .maxlen = sizeof(sysctl_bpf_stats_enabled),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax_bpf_stats,
+ .extra1 = &zero,
+ .extra2 = &one,
+ },
#if defined(CONFIG_TREE_RCU) || defined(CONFIG_PREEMPT_RCU)
{
.procname = "panic_on_rcu_stall",
@@ -3260,6 +3272,28 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
#endif /* CONFIG_PROC_SYSCTL */
+static int proc_dointvec_minmax_bpf_stats(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp,
+ loff_t *ppos)
+{
+ int ret, bpf_stats = *(int *)table->data;
+ struct ctl_table tmp = *table;
+
+ if (write && !capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ tmp.data = &bpf_stats;
+ ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
+ if (write && !ret) {
+ *(int *)table->data = bpf_stats;
+ if (bpf_stats)
+ static_branch_enable(&bpf_stats_enabled_key);
+ else
+ static_branch_disable(&bpf_stats_enabled_key);
+ }
+ return ret;
+}
+
/*
* No sense putting this after each symbol definition, twice,
* exception granted :-)
--
2.20.0
^ permalink raw reply related
* [PATCH v3 bpf-next 0/4] bpf: per program stats
From: Alexei Starovoitov @ 2019-02-25 22:28 UTC (permalink / raw)
To: davem; +Cc: daniel, edumazet, netdev, bpf, kernel-team
Introduce per program stats to monitor the usage BPF
v2->v3:
- rename to run_time_ns/run_cnt everywhere
v1->v2:
- fixed u64 stats on 32-bit archs. Thanks Eric
- use more verbose run_time_ns in json output as suggested by Andrii
- refactored prog_alloc and clarified behavior of stats in subprogs
Alexei Starovoitov (4):
bpf: enable program stats
bpf: expose program stats via bpf_prog_info
tools/bpf: sync bpf.h into tools
tools/bpftool: recognize bpf_prog_info run_time_ns and run_cnt
include/linux/bpf.h | 9 +++++
include/linux/filter.h | 20 +++++++++-
include/uapi/linux/bpf.h | 2 +
kernel/bpf/core.c | 31 ++++++++++++++-
kernel/bpf/syscall.c | 39 ++++++++++++++++++-
kernel/bpf/verifier.c | 7 +++-
kernel/sysctl.c | 34 ++++++++++++++++
.../bpftool/Documentation/bpftool-prog.rst | 4 +-
tools/bpf/bpftool/prog.c | 7 ++++
tools/include/uapi/linux/bpf.h | 2 +
10 files changed, 148 insertions(+), 7 deletions(-)
--
2.20.0
^ permalink raw reply
* [PATCH v3 bpf-next 4/4] tools/bpftool: recognize bpf_prog_info run_time_ns and run_cnt
From: Alexei Starovoitov @ 2019-02-25 22:28 UTC (permalink / raw)
To: davem; +Cc: daniel, edumazet, netdev, bpf, kernel-team
In-Reply-To: <20190225222842.2031962-1-ast@kernel.org>
$ bpftool p s
1: kprobe tag a56587d488d216c9 gpl run_time_ns 79786 run_cnt 8
loaded_at 2019-02-22T12:22:51-0800 uid 0
xlated 352B not jited memlock 4096B
$ bpftool --json --pretty p s
[{
"id": 1,
"type": "kprobe",
"tag": "a56587d488d216c9",
"gpl_compatible": true,
"run_time_ns": 79786,
"run_cnt": 8,
"loaded_at": 1550866971,
"uid": 0,
"bytes_xlated": 352,
"jited": false,
"bytes_memlock": 4096
}
]
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
tools/bpf/bpftool/Documentation/bpftool-prog.rst | 4 +++-
tools/bpf/bpftool/prog.c | 7 +++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 12bc1e2d4b46..9386bd6e0396 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -171,7 +171,7 @@ EXAMPLES
::
- 10: xdp name some_prog tag 005a3d2123620c8b gpl
+ 10: xdp name some_prog tag 005a3d2123620c8b gpl run_time_ns 81632 run_cnt 10
loaded_at 2017-09-29T20:11:00+0000 uid 0
xlated 528B jited 370B memlock 4096B map_ids 10
@@ -184,6 +184,8 @@ EXAMPLES
"type": "xdp",
"tag": "005a3d2123620c8b",
"gpl_compatible": true,
+ "run_time_ns": 81632,
+ "run_cnt": 10,
"loaded_at": 1506715860,
"uid": 0,
"bytes_xlated": 528,
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index db978c8d76a8..0c35dd543d49 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -214,6 +214,10 @@ static void print_prog_json(struct bpf_prog_info *info, int fd)
info->tag[4], info->tag[5], info->tag[6], info->tag[7]);
jsonw_bool_field(json_wtr, "gpl_compatible", info->gpl_compatible);
+ if (info->run_time_ns) {
+ jsonw_uint_field(json_wtr, "run_time_ns", info->run_time_ns);
+ jsonw_uint_field(json_wtr, "run_cnt", info->run_cnt);
+ }
print_dev_json(info->ifindex, info->netns_dev, info->netns_ino);
@@ -277,6 +281,9 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd)
fprint_hex(stdout, info->tag, BPF_TAG_SIZE, "");
print_dev_plain(info->ifindex, info->netns_dev, info->netns_ino);
printf("%s", info->gpl_compatible ? " gpl" : "");
+ if (info->run_time_ns)
+ printf(" run_time_ns %lld run_cnt %lld",
+ info->run_time_ns, info->run_cnt);
printf("\n");
if (info->load_time) {
--
2.20.0
^ permalink raw reply related
* [PATCH v3 bpf-next 3/4] tools/bpf: sync bpf.h into tools
From: Alexei Starovoitov @ 2019-02-25 22:28 UTC (permalink / raw)
To: davem; +Cc: daniel, edumazet, netdev, bpf, kernel-team
In-Reply-To: <20190225222842.2031962-1-ast@kernel.org>
sync bpf.h into tools directory
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
tools/include/uapi/linux/bpf.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index bcdd2474eee7..2e308e90ffea 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2813,6 +2813,8 @@ struct bpf_prog_info {
__u32 jited_line_info_rec_size;
__u32 nr_prog_tags;
__aligned_u64 prog_tags;
+ __u64 run_time_ns;
+ __u64 run_cnt;
} __attribute__((aligned(8)));
struct bpf_map_info {
--
2.20.0
^ permalink raw reply related
* [PATCH v3 bpf-next 2/4] bpf: expose program stats via bpf_prog_info
From: Alexei Starovoitov @ 2019-02-25 22:28 UTC (permalink / raw)
To: davem; +Cc: daniel, edumazet, netdev, bpf, kernel-team
In-Reply-To: <20190225222842.2031962-1-ast@kernel.org>
Return bpf program run_time_ns and run_cnt via bpf_prog_info
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Andrii Nakryiko <andriin@fb.com>
---
include/uapi/linux/bpf.h | 2 ++
kernel/bpf/syscall.c | 5 +++++
2 files changed, 7 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bcdd2474eee7..2e308e90ffea 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2813,6 +2813,8 @@ struct bpf_prog_info {
__u32 jited_line_info_rec_size;
__u32 nr_prog_tags;
__aligned_u64 prog_tags;
+ __u64 run_time_ns;
+ __u64 run_cnt;
} __attribute__((aligned(8)));
struct bpf_map_info {
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 31cf66fc3f5c..174581dfe225 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2152,6 +2152,7 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
struct bpf_prog_info __user *uinfo = u64_to_user_ptr(attr->info.info);
struct bpf_prog_info info = {};
u32 info_len = attr->info.info_len;
+ struct bpf_prog_stats stats;
char __user *uinsns;
u32 ulen;
int err;
@@ -2191,6 +2192,10 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
if (err)
return err;
+ bpf_prog_get_stats(prog, &stats);
+ info.run_time_ns = stats.nsecs;
+ info.run_cnt = stats.cnt;
+
if (!capable(CAP_SYS_ADMIN)) {
info.jited_prog_len = 0;
info.xlated_prog_len = 0;
--
2.20.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox