From: "Michael S. Tsirkin" <mst@redhat•com>
To: Jesper Dangaard Brouer <brouer@redhat•com>
Cc: "Jakub Kicinski" <jakub.kicinski@netronome•com>,
"Toke Høiland-Jørgensen" <toke@toke•dk>,
"David Miller" <davem@davemloft•net>,
dsahern@gmail•com, saeedm@mellanox•com, netdev@vger•kernel.org,
pstaszewski@itcare•pl, jasowang@redhat•com
Subject: Re: consistency for statistics with XDP mode
Date: Tue, 4 Dec 2018 13:06:17 -0500 [thread overview]
Message-ID: <20181204130545-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20181204102904.1cc2c33c@redhat.com>
On Tue, Dec 04, 2018 at 10:29:04AM +0100, Jesper Dangaard Brouer wrote:
> On Mon, 3 Dec 2018 23:24:18 -0800
> Jakub Kicinski <jakub.kicinski@netronome•com> wrote:
>
> > On Tue, 04 Dec 2018 09:03:52 +0200, Toke Høiland-Jørgensen wrote:
> > > David Miller <davem@davemloft•net> writes:
> > >
> > > > From: David Ahern <dsahern@gmail•com>
> > > > Date: Mon, 3 Dec 2018 17:15:03 -0700
> > > >
> > > >> So, instead of a program tag which the program writer controls, how
> > > >> about some config knob that an admin controls that says at attach time
> > > >> use standard stats?
> > > >
> > > > How about, instead of replacing it is in addition to, and admin can
> > > > override?
> > >
> > > Yeah, I was also thinking about something the program writer can set,
> > > but the admin can override. There could even be a system-wide setting
> > > that would make the verifier inject it into all programs at load time?
> >
> > That'd be far preferable, having all drivers include the code when we
> > have JITs seems like a step backward.
>
> There is one problem with it being part of the eBPF prog. Once eBPf
> prog is loaded you cannot change it (store in read-only page for
> security reasons). So, that will not make it possible for an admin to
> enable stats when troubleshooting a system. The use-case I think we
> want to support, is to allow to opt-out due to performance concerns,
> but when an admin need to troubleshoot the system, allow the admin to
> enable this system wide.
>
> Besides placing this in every eBPF program in the system will replicate
> the stats update code (and put more I-cache pressure). The coded
> needed is actually very simple:
>
> [PATCH] xdp: add stats for XDP action codes in xdp_rxq_info
>
> Code muckup of adding XDP stats
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index cc17f5f32fbb..600a95e0cbcc 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -628,7 +628,10 @@ static __always_inline u32 bpf_prog_run_xdp(const struct bpf_prog *prog,
> * already takes rcu_read_lock() when fetching the program, so
> * it's not necessary here anymore.
> */
> - return BPF_PROG_RUN(prog, xdp);
> + u32 action = BPF_PROG_RUN(prog, xdp);
> + // Q: will adding a branch here cost more than always accounting?
> + xdp->rxq->stats[action <= XDP_REDIRECT ? action : 0]++;
We need array_index_nospec I guess?
> + return action;
> }
>
> static inline u32 bpf_prog_insn_size(const struct bpf_prog *prog)
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 4a0ca7a3d5e5..3409dd9e0fbc 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -6,6 +6,7 @@
> #ifndef __LINUX_NET_XDP_H__
> #define __LINUX_NET_XDP_H__
>
> +#include <uapi/linux/bpf.h>
> /**
> * DOC: XDP RX-queue information
> *
> @@ -61,6 +62,8 @@ struct xdp_rxq_info {
> u32 queue_index;
> u32 reg_state;
> struct xdp_mem_info mem;
> + // TODO: benchmark if stats should be placed on different cache-line
> + u64 stats[XDP_REDIRECT + 1];
> } ____cacheline_aligned; /* perf critical, avoid false-sharing */
>
> struct xdp_buff {
>
>
>
> > We could probably fit the stats into the enormous padding of struct
> > xdp_rxq_info, the question is - how do we get to it in a clean way..
>
> Struct xdp_rxq_info is explicitly a read-only cache-line, which contain
> static information for each RX-queue. We could place the stats record
> in the next cache-line (most HW systems fetch 2 cache-lines). But we
> can also benchmark if it matters changing xdp_rxq_info to be a
> write-cache-line.
>
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
next prev parent reply other threads:[~2018-12-04 18:06 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-21 21:06 consistency for statistics with XDP mode David Ahern
2018-11-21 21:14 ` Toke Høiland-Jørgensen
2018-11-21 21:29 ` Paweł Staszewski
2018-11-22 0:21 ` Saeed Mahameed
2018-11-22 8:26 ` Toke Høiland-Jørgensen
2018-11-22 16:51 ` David Ahern
2018-11-22 17:00 ` Toke Høiland-Jørgensen
2018-11-30 20:10 ` Saeed Mahameed
2018-11-30 20:30 ` Michael S. Tsirkin
2018-11-30 20:35 ` David Ahern
2018-12-01 4:41 ` Jakub Kicinski
2018-12-01 11:14 ` Jesper Dangaard Brouer
2018-12-03 15:56 ` David Ahern
2018-12-03 19:32 ` David Miller
2018-11-30 23:54 ` Saeed Mahameed
2018-12-01 11:22 ` Jesper Dangaard Brouer
2018-12-03 15:45 ` David Ahern
2018-12-03 19:30 ` David Miller
2018-12-03 19:41 ` Arnaldo Carvalho de Melo
2018-12-03 20:00 ` Toke Høiland-Jørgensen
2018-12-04 0:00 ` David Miller
2018-12-04 0:15 ` David Ahern
2018-12-04 0:36 ` David Miller
2018-12-04 7:03 ` Toke Høiland-Jørgensen
2018-12-04 7:24 ` Jakub Kicinski
2018-12-04 9:29 ` Jesper Dangaard Brouer
2018-12-04 17:56 ` Jakub Kicinski
2018-12-04 18:06 ` Michael S. Tsirkin [this message]
2018-11-24 7:07 ` David Miller
2018-11-22 0:53 ` Toshiaki Makita
2018-11-22 16:43 ` David Ahern
2018-11-26 1:37 ` Toshiaki Makita
2018-11-27 7:04 ` Toshiaki Makita
2018-11-28 4:03 ` Jason Wang
2018-11-28 5:09 ` Toshiaki Makita
2018-11-26 22:15 ` 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=20181204130545-mutt-send-email-mst@kernel.org \
--to=mst@redhat$(echo .)com \
--cc=brouer@redhat$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=dsahern@gmail$(echo .)com \
--cc=jakub.kicinski@netronome$(echo .)com \
--cc=jasowang@redhat$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pstaszewski@itcare$(echo .)pl \
--cc=saeedm@mellanox$(echo .)com \
--cc=toke@toke$(echo .)dk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox