* [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames @ 2019-01-31 11:40 Toshiaki Makita 2019-01-31 15:25 ` Michael S. Tsirkin 2019-02-04 4:18 ` [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames David Miller 0 siblings, 2 replies; 27+ messages in thread From: Toshiaki Makita @ 2019-01-31 11:40 UTC (permalink / raw) To: David S. Miller, Michael S. Tsirkin, Jason Wang Cc: Toshiaki Makita, netdev, virtualization, David Ahern Previously virtnet_xdp_xmit() did not account for device tx counters, which caused confusions. To be consistent with SKBs, account them on freeing xdp_frames. Reported-by: David Ahern <dsahern@gmail.com> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> --- drivers/net/virtio_net.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 2594481..4cfceb7 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -503,6 +503,8 @@ static int virtnet_xdp_xmit(struct net_device *dev, struct bpf_prog *xdp_prog; struct send_queue *sq; unsigned int len; + int packets = 0; + int bytes = 0; int drops = 0; int kicks = 0; int ret, err; @@ -526,10 +528,18 @@ static int virtnet_xdp_xmit(struct net_device *dev, /* Free up any pending old buffers before queueing new ones. */ while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) { - if (likely(is_xdp_frame(ptr))) - xdp_return_frame(ptr_to_xdp(ptr)); - else - napi_consume_skb(ptr, false); + if (likely(is_xdp_frame(ptr))) { + struct xdp_frame *frame = ptr_to_xdp(ptr); + + bytes += frame->len; + xdp_return_frame(frame); + } else { + struct sk_buff *skb = ptr; + + bytes += skb->len; + napi_consume_skb(skb, false); + } + packets++; } for (i = 0; i < n; i++) { @@ -549,6 +559,8 @@ static int virtnet_xdp_xmit(struct net_device *dev, } out: u64_stats_update_begin(&sq->stats.syncp); + sq->stats.bytes += bytes; + sq->stats.packets += packets; sq->stats.xdp_tx += n; sq->stats.xdp_tx_drops += drops; sq->stats.kicks += kicks; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames 2019-01-31 11:40 [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames Toshiaki Makita @ 2019-01-31 15:25 ` Michael S. Tsirkin 2019-01-31 17:45 ` David Miller 2019-02-04 4:18 ` [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames David Miller 1 sibling, 1 reply; 27+ messages in thread From: Michael S. Tsirkin @ 2019-01-31 15:25 UTC (permalink / raw) To: Toshiaki Makita Cc: David S. Miller, Jason Wang, netdev, virtualization, David Ahern On Thu, Jan 31, 2019 at 08:40:30PM +0900, Toshiaki Makita wrote: > Previously virtnet_xdp_xmit() did not account for device tx counters, > which caused confusions. > To be consistent with SKBs, account them on freeing xdp_frames. > > Reported-by: David Ahern <dsahern@gmail.com> > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> Well we count them on receive so I guess it makes sense for consistency Acked-by: Michael S. Tsirkin <mst@redhat.com> however, I really wonder whether adding more and more standard net stack things like this will end up costing most of XDP its speed. Should we instead make sure *not* to account XDP packets in any counters at all? XDP programs can use maps to do their own counting... > --- > drivers/net/virtio_net.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 2594481..4cfceb7 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -503,6 +503,8 @@ static int virtnet_xdp_xmit(struct net_device *dev, > struct bpf_prog *xdp_prog; > struct send_queue *sq; > unsigned int len; > + int packets = 0; > + int bytes = 0; > int drops = 0; > int kicks = 0; > int ret, err; > @@ -526,10 +528,18 @@ static int virtnet_xdp_xmit(struct net_device *dev, > > /* Free up any pending old buffers before queueing new ones. */ > while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) { > - if (likely(is_xdp_frame(ptr))) > - xdp_return_frame(ptr_to_xdp(ptr)); > - else > - napi_consume_skb(ptr, false); > + if (likely(is_xdp_frame(ptr))) { > + struct xdp_frame *frame = ptr_to_xdp(ptr); > + > + bytes += frame->len; > + xdp_return_frame(frame); > + } else { > + struct sk_buff *skb = ptr; > + > + bytes += skb->len; > + napi_consume_skb(skb, false); > + } > + packets++; > } > > for (i = 0; i < n; i++) { > @@ -549,6 +559,8 @@ static int virtnet_xdp_xmit(struct net_device *dev, > } > out: > u64_stats_update_begin(&sq->stats.syncp); > + sq->stats.bytes += bytes; > + sq->stats.packets += packets; > sq->stats.xdp_tx += n; > sq->stats.xdp_tx_drops += drops; > sq->stats.kicks += kicks; > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames 2019-01-31 15:25 ` Michael S. Tsirkin @ 2019-01-31 17:45 ` David Miller 2019-01-31 20:15 ` Jesper Dangaard Brouer 0 siblings, 1 reply; 27+ messages in thread From: David Miller @ 2019-01-31 17:45 UTC (permalink / raw) To: mst; +Cc: makita.toshiaki, jasowang, netdev, virtualization, dsahern, hawk From: "Michael S. Tsirkin" <mst@redhat.com> Date: Thu, 31 Jan 2019 10:25:17 -0500 > On Thu, Jan 31, 2019 at 08:40:30PM +0900, Toshiaki Makita wrote: >> Previously virtnet_xdp_xmit() did not account for device tx counters, >> which caused confusions. >> To be consistent with SKBs, account them on freeing xdp_frames. >> >> Reported-by: David Ahern <dsahern@gmail.com> >> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > > Well we count them on receive so I guess it makes sense for consistency > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > however, I really wonder whether adding more and more standard net stack > things like this will end up costing most of XDP its speed. > > Should we instead make sure *not* to account XDP packets > in any counters at all? XDP programs can use maps > to do their own counting... This has been definitely a discussion point, and something we should develop a clear, strong, policy on. David, Jesper, care to chime in where we ended up in that last thread discussion this? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames 2019-01-31 17:45 ` David Miller @ 2019-01-31 20:15 ` Jesper Dangaard Brouer 2019-02-01 1:53 ` Toshiaki Makita 2019-02-02 21:27 ` David Ahern 0 siblings, 2 replies; 27+ messages in thread From: Jesper Dangaard Brouer @ 2019-01-31 20:15 UTC (permalink / raw) To: David Miller Cc: mst, makita.toshiaki, jasowang, netdev, virtualization, dsahern, hawk, Toke Høiland-Jørgensen On Thu, 31 Jan 2019 09:45:23 -0800 (PST) David Miller <davem@davemloft.net> wrote: > From: "Michael S. Tsirkin" <mst@redhat.com> > Date: Thu, 31 Jan 2019 10:25:17 -0500 > > > On Thu, Jan 31, 2019 at 08:40:30PM +0900, Toshiaki Makita wrote: > >> Previously virtnet_xdp_xmit() did not account for device tx counters, > >> which caused confusions. > >> To be consistent with SKBs, account them on freeing xdp_frames. > >> > >> Reported-by: David Ahern <dsahern@gmail.com> > >> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > > > > Well we count them on receive so I guess it makes sense for consistency > > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > > however, I really wonder whether adding more and more standard net stack > > things like this will end up costing most of XDP its speed. > > > > Should we instead make sure *not* to account XDP packets > > in any counters at all? XDP programs can use maps > > to do their own counting... > > This has been definitely a discussion point, and something we should > develop a clear, strong, policy on. > > David, Jesper, care to chime in where we ended up in that last thread > discussion this? IHMO packets RX and TX on a device need to be accounted, in standard counters, regardless of XDP. For XDP RX the packet is counted as RX, regardless if XDP choose to XDP_DROP. On XDP TX which is via XDP_REDIRECT or XDP_TX, the driver that transmit the packet need to account the packet in a TX counter (this if often delayed to DMA TX completion handling). We cannot break the expectation that RX and TX counter are visible to userspace stats tools. XDP should not make these packets invisible. Performance wise, I don't see an issue. As updating these counters (packets and bytes) can be done as a bulk, either when driver NAPI RX func ends, or in TX DMA completion, like most drivers already do today. Further more, most drivers save this in per RX ring data-area, which are only summed when userspace read these. A separate question (and project) raised by David Ahern, was if we should have more detailed stats on the different XDP action return codes, as an easy means for sysadms to diagnose running XDP programs. That is something that require more discussions, as it can impact performance, and likely need to be opt-in. My opinion is yes we should do this for the sake of better User eXperience, BUT *only* if we can find a technical solution that does not hurt performance. --Jesper ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames 2019-01-31 20:15 ` Jesper Dangaard Brouer @ 2019-02-01 1:53 ` Toshiaki Makita 2019-02-02 21:27 ` David Ahern 1 sibling, 0 replies; 27+ messages in thread From: Toshiaki Makita @ 2019-02-01 1:53 UTC (permalink / raw) To: Jesper Dangaard Brouer, David Miller Cc: mst, jasowang, netdev, virtualization, dsahern, hawk, Toke Høiland-Jørgensen On 2019/02/01 5:15, Jesper Dangaard Brouer wrote: > On Thu, 31 Jan 2019 09:45:23 -0800 (PST) > David Miller <davem@davemloft.net> wrote: > >> From: "Michael S. Tsirkin" <mst@redhat.com> >> Date: Thu, 31 Jan 2019 10:25:17 -0500 >> >>> On Thu, Jan 31, 2019 at 08:40:30PM +0900, Toshiaki Makita wrote: >>>> Previously virtnet_xdp_xmit() did not account for device tx counters, >>>> which caused confusions. >>>> To be consistent with SKBs, account them on freeing xdp_frames. >>>> >>>> Reported-by: David Ahern <dsahern@gmail.com> >>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> >>> >>> Well we count them on receive so I guess it makes sense for consistency >>> >>> Acked-by: Michael S. Tsirkin <mst@redhat.com> >>> >>> however, I really wonder whether adding more and more standard net stack >>> things like this will end up costing most of XDP its speed. >>> >>> Should we instead make sure *not* to account XDP packets >>> in any counters at all? XDP programs can use maps >>> to do their own counting... >> >> This has been definitely a discussion point, and something we should >> develop a clear, strong, policy on. >> >> David, Jesper, care to chime in where we ended up in that last thread >> discussion this? > > IHMO packets RX and TX on a device need to be accounted, in standard > counters, regardless of XDP. For XDP RX the packet is counted as RX, > regardless if XDP choose to XDP_DROP. On XDP TX which is via > XDP_REDIRECT or XDP_TX, the driver that transmit the packet need to > account the packet in a TX counter (this if often delayed to DMA TX > completion handling). We cannot break the expectation that RX and TX > counter are visible to userspace stats tools. XDP should not make these > packets invisible. > > Performance wise, I don't see an issue. As updating these counters > (packets and bytes) can be done as a bulk, either when driver NAPI RX > func ends, or in TX DMA completion, like most drivers already do today. > Further more, most drivers save this in per RX ring data-area, which > are only summed when userspace read these. Agreed. > A separate question (and project) raised by David Ahern, was if we > should have more detailed stats on the different XDP action return > codes, as an easy means for sysadms to diagnose running XDP programs. > That is something that require more discussions, as it can impact > performance, and likely need to be opt-in. My opinion is yes we should > do this for the sake of better User eXperience, BUT *only* if we can find > a technical solution that does not hurt performance. Basically the situation for the detailed stats is the same as standard stats, at least in virtio_net. Stats are updated as a bulk, and the counters reside in RX/TX ring structures. Probably this way of implementation would be ok performance-wise? But as other drivers may have different situations, if it is generally difficult to avoid performance penalty I'm OK with making them opt-in as a standard way. -- Toshiaki Makita ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames 2019-01-31 20:15 ` Jesper Dangaard Brouer 2019-02-01 1:53 ` Toshiaki Makita @ 2019-02-02 21:27 ` David Ahern 2019-02-04 11:53 ` Jesper Dangaard Brouer 1 sibling, 1 reply; 27+ messages in thread From: David Ahern @ 2019-02-02 21:27 UTC (permalink / raw) To: Jesper Dangaard Brouer, David Miller Cc: mst, makita.toshiaki, jasowang, netdev, virtualization, hawk, Toke Høiland-Jørgensen On 1/31/19 1:15 PM, Jesper Dangaard Brouer wrote: >> >> David, Jesper, care to chime in where we ended up in that last thread >> discussion this? > > IHMO packets RX and TX on a device need to be accounted, in standard > counters, regardless of XDP. For XDP RX the packet is counted as RX, > regardless if XDP choose to XDP_DROP. On XDP TX which is via > XDP_REDIRECT or XDP_TX, the driver that transmit the packet need to > account the packet in a TX counter (this if often delayed to DMA TX > completion handling). We cannot break the expectation that RX and TX > counter are visible to userspace stats tools. XDP should not make these > packets invisible. Agreed. What I was pushing on that last thread was Rx, Tx and dropped are all accounted by the driver in standard stats. Basically if the driver touched it, the driver's counters should indicate that. The push back was on dropped packets and whether that counter should be bumped on XDP_DROP. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames 2019-02-02 21:27 ` David Ahern @ 2019-02-04 11:53 ` Jesper Dangaard Brouer 2019-02-05 3:13 ` David Ahern 0 siblings, 1 reply; 27+ messages in thread From: Jesper Dangaard Brouer @ 2019-02-04 11:53 UTC (permalink / raw) To: David Ahern Cc: David Miller, mst, makita.toshiaki, jasowang, netdev, virtualization, hawk, Toke Høiland-Jørgensen, brouer, Jakub Kicinski, John Fastabend, Daniel Borkmann, Saeed Mahameed, Tariq Toukan On Sat, 2 Feb 2019 14:27:26 -0700 David Ahern <dsahern@gmail.com> wrote: > On 1/31/19 1:15 PM, Jesper Dangaard Brouer wrote: > >> > >> David, Jesper, care to chime in where we ended up in that last thread > >> discussion this? > > > > IHMO packets RX and TX on a device need to be accounted, in standard > > counters, regardless of XDP. For XDP RX the packet is counted as RX, > > regardless if XDP choose to XDP_DROP. On XDP TX which is via > > XDP_REDIRECT or XDP_TX, the driver that transmit the packet need to > > account the packet in a TX counter (this if often delayed to DMA TX > > completion handling). We cannot break the expectation that RX and TX > > counter are visible to userspace stats tools. XDP should not make these > > packets invisible. > > Agreed. What I was pushing on that last thread was Rx, Tx and dropped > are all accounted by the driver in standard stats. Basically if the > driver touched it, the driver's counters should indicate that. Sound like we all agree (except with the dropped counter, see below). Do notice that mlx5 driver doesn't do this. It is actually rather confusing to use XDP on mlx5, as when XDP "consume" which include XDP_DROP, XDP_REDIRECT or XDP_TX, then the driver standard stats are not incremented... the packet is invisible to "ifconfig" stat based tools. > The push back was on dropped packets and whether that counter should be > bumped on XDP_DROP. My opinion is the XDP_DROP action should NOT increment the drivers drop counter. First of all the "dropped" counter is also use for other stuff, which will confuse that this counter express. Second, choosing XDP_DROP is a policy choice, it still means it was RX-ed at the driver level. -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames 2019-02-04 11:53 ` Jesper Dangaard Brouer @ 2019-02-05 3:13 ` David Ahern 2019-02-06 0:06 ` Saeed Mahameed 2019-04-18 14:24 ` Stats for XDP actions (was: Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) Toke Høiland-Jørgensen 0 siblings, 2 replies; 27+ messages in thread From: David Ahern @ 2019-02-05 3:13 UTC (permalink / raw) To: Jesper Dangaard Brouer Cc: David Miller, mst, makita.toshiaki, jasowang, netdev, virtualization, hawk, Toke Høiland-Jørgensen, Jakub Kicinski, John Fastabend, Daniel Borkmann, Saeed Mahameed, Tariq Toukan On 2/4/19 3:53 AM, Jesper Dangaard Brouer wrote: > On Sat, 2 Feb 2019 14:27:26 -0700 > David Ahern <dsahern@gmail.com> wrote: > >> On 1/31/19 1:15 PM, Jesper Dangaard Brouer wrote: >>>> >>>> David, Jesper, care to chime in where we ended up in that last thread >>>> discussion this? >>> >>> IHMO packets RX and TX on a device need to be accounted, in standard >>> counters, regardless of XDP. For XDP RX the packet is counted as RX, >>> regardless if XDP choose to XDP_DROP. On XDP TX which is via >>> XDP_REDIRECT or XDP_TX, the driver that transmit the packet need to >>> account the packet in a TX counter (this if often delayed to DMA TX >>> completion handling). We cannot break the expectation that RX and TX >>> counter are visible to userspace stats tools. XDP should not make these >>> packets invisible. >> >> Agreed. What I was pushing on that last thread was Rx, Tx and dropped >> are all accounted by the driver in standard stats. Basically if the >> driver touched it, the driver's counters should indicate that. > > Sound like we all agree (except with the dropped counter, see below). > > Do notice that mlx5 driver doesn't do this. It is actually rather > confusing to use XDP on mlx5, as when XDP "consume" which include > XDP_DROP, XDP_REDIRECT or XDP_TX, then the driver standard stats are > not incremented... the packet is invisible to "ifconfig" stat based > tools. mlx5 needs some work. As I recall it still has the bug/panic removing xdp programs - at least I don't recall seeing a patch for it. > > >> The push back was on dropped packets and whether that counter should be >> bumped on XDP_DROP. > > My opinion is the XDP_DROP action should NOT increment the drivers drop > counter. First of all the "dropped" counter is also use for other > stuff, which will confuse that this counter express. Second, choosing > XDP_DROP is a policy choice, it still means it was RX-ed at the driver > level. > Understood. Hopefully in March I will get some time to come back to this and propose an idea on what I would like to see - namely, the admin has a config option at load time to enable driver counters versus custom map counters. (meaning the operator of the node chooses standard stats over strict performance.) But of course that means the drivers have the code to collect those stats. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames 2019-02-05 3:13 ` David Ahern @ 2019-02-06 0:06 ` Saeed Mahameed 2019-02-06 13:48 ` Jesper Dangaard Brouer ` (2 more replies) 2019-04-18 14:24 ` Stats for XDP actions (was: Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) Toke Høiland-Jørgensen 1 sibling, 3 replies; 27+ messages in thread From: Saeed Mahameed @ 2019-02-06 0:06 UTC (permalink / raw) To: dsahern, brouer Cc: thoiland, hawk, virtualization, borkmann, Tariq Toukan, john.fastabend, mst, jakub.kicinski, netdev, jasowang, davem, makita.toshiaki On Mon, 2019-02-04 at 19:13 -0800, David Ahern wrote: > On 2/4/19 3:53 AM, Jesper Dangaard Brouer wrote: > > On Sat, 2 Feb 2019 14:27:26 -0700 > > David Ahern <dsahern@gmail.com> wrote: > > > > > On 1/31/19 1:15 PM, Jesper Dangaard Brouer wrote: > > > > > David, Jesper, care to chime in where we ended up in that > > > > > last thread > > > > > discussion this? > > > > > > > > IHMO packets RX and TX on a device need to be accounted, in > > > > standard > > > > counters, regardless of XDP. For XDP RX the packet is counted > > > > as RX, > > > > regardless if XDP choose to XDP_DROP. On XDP TX which is via > > > > XDP_REDIRECT or XDP_TX, the driver that transmit the packet > > > > need to > > > > account the packet in a TX counter (this if often delayed to > > > > DMA TX > > > > completion handling). We cannot break the expectation that RX > > > > and TX > > > > counter are visible to userspace stats tools. XDP should not > > > > make these > > > > packets invisible. > > > > > > Agreed. What I was pushing on that last thread was Rx, Tx and > > > dropped > > > are all accounted by the driver in standard stats. Basically if > > > the > > > driver touched it, the driver's counters should indicate that. > > > > Sound like we all agree (except with the dropped counter, see > > below). > > > > Do notice that mlx5 driver doesn't do this. It is actually rather > > confusing to use XDP on mlx5, as when XDP "consume" which include > > XDP_DROP, XDP_REDIRECT or XDP_TX, then the driver standard stats > > are > > not incremented... the packet is invisible to "ifconfig" stat based > > tools. > > mlx5 needs some work. As I recall it still has the bug/panic removing > xdp programs - at least I don't recall seeing a patch for it. Only when xdp_redirect to mlx5, and removing the program while redirect is happening, this is actually due to a lack of synchronization means between different drivers, we have some ideas to overcome this using a standard XDP API, or just use a hack in mlx5 driver which i don't like: https://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git/commit/?h=topic/xdp-redirect-fix&id=a3652d03cc35fd3ad62744986c8ccaca74c9f20c I will be working on this towards the end of this week. > > > > > > The push back was on dropped packets and whether that counter > > > should be > > > bumped on XDP_DROP. > > > > My opinion is the XDP_DROP action should NOT increment the drivers > > drop > > counter. First of all the "dropped" counter is also use for other > > stuff, which will confuse that this counter express. Second, > > choosing > > XDP_DROP is a policy choice, it still means it was RX-ed at the > > driver > > level. > > > > Understood. Hopefully in March I will get some time to come back to > this > and propose an idea on what I would like to see - namely, the admin > has > a config option at load time to enable driver counters versus custom > map > counters. (meaning the operator of the node chooses standard stats > over > strict performance.) But of course that means the drivers have the > code > to collect those stats. So bottom line: 1) Driver will count rx packets as rx-ed packets regardless of XDP decision. 2) Driver should keep track of XDP decisions statistics, report them in ethtool and in the new API suggested by David. track even (XDP_PASS) ? Maybe instead of having all drivers track the statistics on their own, we should move the responsibility to upper layer. Idea: since we already have rxq_info structure per XDP ring (no false sharing) and available per xdp_buff we can do: +++ b/include/linux/filter.h @@ -651,7 +651,9 @@ 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 ret = BPF_PROG_RUN(prog, xdp); + xdp->xdp_rxq_info.stats[ret]++ + return ret; } still we need a way (API) to report the rxq_info to whoever needs to read current XDP stats 3) Unrelated, In non XDP case, if skb allocation fails or driver fails to pass the skb up to the stack for somereason, should the driver increase rx packets ? IMHO the answer should be yes if we want to have similar behavior between XDP and non XDP cases. But this could result in netdev->stats.rx_packets + netdev- >stats.rx_dropped to be more than the actual rx-ed packets, is this acceptable ? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames 2019-02-06 0:06 ` Saeed Mahameed @ 2019-02-06 13:48 ` Jesper Dangaard Brouer 2019-02-06 15:52 ` Jakub Kicinski 2019-02-07 7:48 ` Resource management for ndo_xdp_xmit (Was: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) Jesper Dangaard Brouer 2019-02-08 16:02 ` [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames Jesper Dangaard Brouer 2 siblings, 1 reply; 27+ messages in thread From: Jesper Dangaard Brouer @ 2019-02-06 13:48 UTC (permalink / raw) To: Saeed Mahameed Cc: dsahern, thoiland, virtualization, borkmann, Tariq Toukan, john.fastabend, mst, jakub.kicinski, netdev, jasowang, davem, makita.toshiaki, brouer On Wed, 6 Feb 2019 00:06:33 +0000 Saeed Mahameed <saeedm@mellanox.com> wrote: > 3) Unrelated, In non XDP case, if skb allocation fails or driver fails > to pass the skb up to the stack for somereason, should the driver > increase rx packets ? IMHO the answer should be yes if we want to have > similar behavior between XDP and non XDP cases. I don't think "skb allocation fails" should increase rx packets counter. The difference is that these events are outside sysadm/users control, and is an error detected inside the driver. The XDP program takes a policy choice to XDP_DROP a packet, which can be accounted inside the XDP prog (as the samples show) or as we also discuss via a more generic XDP-action counters. That said, I took at quick look at driver code, and it seems this behavior differs per driver... ixgbe and mlx5 does not count "skb allocation fails" as RX-ed packets, while mlx4 seems to count them. > But this could result in netdev->stats.rx_packets + > netdev->stats.rx_dropped to be more than the actual rx-ed packets, is > this acceptable ? This is one reasons I think this is wrong. --Jesper ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames 2019-02-06 13:48 ` Jesper Dangaard Brouer @ 2019-02-06 15:52 ` Jakub Kicinski 0 siblings, 0 replies; 27+ messages in thread From: Jakub Kicinski @ 2019-02-06 15:52 UTC (permalink / raw) To: Jesper Dangaard Brouer Cc: Saeed Mahameed, dsahern, thoiland, virtualization, borkmann, Tariq Toukan, john.fastabend, mst, netdev, jasowang, davem, makita.toshiaki On Wed, 6 Feb 2019 14:48:14 +0100, Jesper Dangaard Brouer wrote: > On Wed, 6 Feb 2019 00:06:33 +0000 > Saeed Mahameed <saeedm@mellanox.com> wrote: > > > 3) Unrelated, In non XDP case, if skb allocation fails or driver fails > > to pass the skb up to the stack for somereason, should the driver > > increase rx packets ? IMHO the answer should be yes if we want to have > > similar behavior between XDP and non XDP cases. > > I don't think "skb allocation fails" should increase rx packets > counter. The difference is that these events are outside sysadm/users > control, and is an error detected inside the driver. The XDP program > takes a policy choice to XDP_DROP a packet, which can be accounted > inside the XDP prog (as the samples show) or as we also discuss via a > more generic XDP-action counters. FWIW that's my understanding as well. My understanding of Linux stats is that they are incrementing one counter per packet. I.e. in RX direction success packets are those given to the stack, and for TX those given to the hardware. Standards (IETF/IEEE) usually count stats on the same layer boundary, but I think software generally counts when it's done with the packet. I haven't seen it documented anywhere, yet. I have tried to document it in the docs of the recent RFC: https://patchwork.ozlabs.org/patch/1032332/ Incidentally XDP_DROP may have been better named XDP_DISCARD from stats perspective ;) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Resource management for ndo_xdp_xmit (Was: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) 2019-02-06 0:06 ` Saeed Mahameed 2019-02-06 13:48 ` Jesper Dangaard Brouer @ 2019-02-07 7:48 ` Jesper Dangaard Brouer 2019-02-07 19:08 ` Saeed Mahameed 2019-02-08 16:02 ` [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames Jesper Dangaard Brouer 2 siblings, 1 reply; 27+ messages in thread From: Jesper Dangaard Brouer @ 2019-02-07 7:48 UTC (permalink / raw) To: Saeed Mahameed Cc: dsahern, Toke Høiland-Jørgensen, hawk, virtualization, borkmann, Tariq Toukan, john.fastabend, mst, jakub.kicinski, netdev, jasowang, davem, makita.toshiaki, brouer, Toke Høiland-Jørgensen On Wed, 6 Feb 2019 00:06:33 +0000 Saeed Mahameed <saeedm@mellanox.com> wrote: > On Mon, 2019-02-04 at 19:13 -0800, David Ahern wrote: [...] > > > > mlx5 needs some work. As I recall it still has the bug/panic > > removing xdp programs - at least I don't recall seeing a patch for > > it. > > Only when xdp_redirect to mlx5, and removing the program while > redirect is happening, this is actually due to a lack of > synchronization means between different drivers, we have some ideas > to overcome this using a standard XDP API, or just use a hack in mlx5 > driver which i don't like: > > https://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git/commit/?h=topic/xdp-redirect-fix&id=a3652d03cc35fd3ad62744986c8ccaca74c9f20c > > I will be working on this towards the end of this week. Toke and I have been discussing how to solve this. The main idea for fixing this is to tie resource allocation to interface insertion into interface maps (kernel/bpf/devmap.c). As the =devmap= already have the needed synchronisation mechanisms and steps for safely adding and removing =net_devices= (e.g. stopping RX side, flushing remaining frames, waiting RCU period before freeing objects, etc.) As described here: https://github.com/xdp-project/xdp-project/blob/master/xdp-project.org#better-ndo_xdp_xmit-resource-management --Jesper ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Resource management for ndo_xdp_xmit (Was: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) 2019-02-07 7:48 ` Resource management for ndo_xdp_xmit (Was: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) Jesper Dangaard Brouer @ 2019-02-07 19:08 ` Saeed Mahameed 2019-02-08 16:55 ` Toke Høiland-Jørgensen 2019-02-08 23:17 ` Saeed Mahameed 0 siblings, 2 replies; 27+ messages in thread From: Saeed Mahameed @ 2019-02-07 19:08 UTC (permalink / raw) To: brouer Cc: thoiland, hawk, virtualization, borkmann, Tariq Toukan, john.fastabend, toke, mst, jakub.kicinski, dsahern, netdev, jasowang, davem, makita.toshiaki On Thu, 2019-02-07 at 08:48 +0100, Jesper Dangaard Brouer wrote: > On Wed, 6 Feb 2019 00:06:33 +0000 Saeed Mahameed <saeedm@mellanox.com > > wrote: > > > On Mon, 2019-02-04 at 19:13 -0800, David Ahern wrote: > [...] > > > mlx5 needs some work. As I recall it still has the bug/panic > > > removing xdp programs - at least I don't recall seeing a patch > > > for > > > it. > > > > Only when xdp_redirect to mlx5, and removing the program while > > redirect is happening, this is actually due to a lack of > > synchronization means between different drivers, we have some ideas > > to overcome this using a standard XDP API, or just use a hack in > > mlx5 > > driver which i don't like: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git/commit/?h=topic/xdp-redirect-fix&id=a3652d03cc35fd3ad62744986c8ccaca74c9f20c > > > > I will be working on this towards the end of this week. > > Toke and I have been discussing how to solve this. > > The main idea for fixing this is to tie resource allocation to > interface > insertion into interface maps (kernel/bpf/devmap.c). As the =devmap= > already have the needed synchronisation mechanisms and steps for > safely > adding and removing =net_devices= (e.g. stopping RX side, flushing > remaining frames, waiting RCU period before freeing objects, etc.) > > As described here: > > https://github.com/xdp-project/xdp-project/blob/master/xdp-project.org#better-ndo_xdp_xmit-resource-management > > --Jesper Yes you already suggested this approach @LPC: So 1) on dev_map_update_elem() we will call dev->dev->ndo_bpf() to notify the device on the intention to start/stop redirect, and wait for it to create/destroy the HW resources before/after actually updating the map But: 2) this won't totally solve our problem, since sometimes the driver can decide to recreate (change of configuration) hw resources on the fly while redirect/devmap is already happening, so we need some kind of a dev_map_notification or a flag with rcu synch, for when the driver want to make the xdp redirect resources unavailable. Thanks, Saeed. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Resource management for ndo_xdp_xmit (Was: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) 2019-02-07 19:08 ` Saeed Mahameed @ 2019-02-08 16:55 ` Toke Høiland-Jørgensen 2019-02-08 22:49 ` Saeed Mahameed 2019-02-08 23:17 ` Saeed Mahameed 1 sibling, 1 reply; 27+ messages in thread From: Toke Høiland-Jørgensen @ 2019-02-08 16:55 UTC (permalink / raw) To: Saeed Mahameed, brouer Cc: hawk, virtualization, borkmann, Tariq Toukan, john.fastabend, mst, jakub.kicinski, dsahern, netdev, jasowang, davem, makita.toshiaki Saeed Mahameed <saeedm@mellanox.com> writes: > But: > 2) this won't totally solve our problem, since sometimes the driver can > decide to recreate (change of configuration) hw resources on the fly > while redirect/devmap is already happening, so we need some kind of a > dev_map_notification or a flag with rcu synch, for when the driver want > to make the xdp redirect resources unavailable. Good point, I'll make a note of this. Do you have a pointer to where the mlx5 driver does this kind of change currently? -Toke ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Resource management for ndo_xdp_xmit (Was: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) 2019-02-08 16:55 ` Toke Høiland-Jørgensen @ 2019-02-08 22:49 ` Saeed Mahameed 0 siblings, 0 replies; 27+ messages in thread From: Saeed Mahameed @ 2019-02-08 22:49 UTC (permalink / raw) To: toke, brouer Cc: hawk, virtualization, borkmann, Tariq Toukan, john.fastabend, jakub.kicinski, mst, dsahern, netdev, jasowang, davem, makita.toshiaki On Fri, 2019-02-08 at 17:55 +0100, Toke Høiland-Jørgensen wrote: > Saeed Mahameed <saeedm@mellanox.com> writes: > > > But: > > 2) this won't totally solve our problem, since sometimes the driver > > can > > decide to recreate (change of configuration) hw resources on the > > fly > > while redirect/devmap is already happening, so we need some kind of > > a > > dev_map_notification or a flag with rcu synch, for when the driver > > want > > to make the xdp redirect resources unavailable. > > Good point, I'll make a note of this. Do you have a pointer to where > the > mlx5 driver does this kind of change currently? > example: ethtool -L to reduce/increase the number of rings e.g. @mlx5e_ethtool_set_ringparam or virtually anywhere mlx5e_switch_priv_channels is called when xdp prog redirect is attached to mlx5. > -Toke ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Resource management for ndo_xdp_xmit (Was: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) 2019-02-07 19:08 ` Saeed Mahameed 2019-02-08 16:55 ` Toke Høiland-Jørgensen @ 2019-02-08 23:17 ` Saeed Mahameed 2019-02-09 0:18 ` Saeed Mahameed 1 sibling, 1 reply; 27+ messages in thread From: Saeed Mahameed @ 2019-02-08 23:17 UTC (permalink / raw) To: brouer Cc: thoiland, hawk, virtualization, borkmann, Tariq Toukan, toke, john.fastabend, mst, jakub.kicinski, dsahern, netdev, jasowang, davem, makita.toshiaki On Thu, 2019-02-07 at 19:08 +0000, Saeed Mahameed wrote: > On Thu, 2019-02-07 at 08:48 +0100, Jesper Dangaard Brouer wrote: > > On Wed, 6 Feb 2019 00:06:33 +0000 Saeed Mahameed < > > saeedm@mellanox.com > > > wrote: > > > On Mon, 2019-02-04 at 19:13 -0800, David Ahern wrote: > > [...] > > > > mlx5 needs some work. As I recall it still has the bug/panic > > > > removing xdp programs - at least I don't recall seeing a patch > > > > for > > > > it. > > > > > > Only when xdp_redirect to mlx5, and removing the program while > > > redirect is happening, this is actually due to a lack of > > > synchronization means between different drivers, we have some > > > ideas > > > to overcome this using a standard XDP API, or just use a hack in > > > mlx5 > > > driver which i don't like: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git/commit/?h=topic/xdp-redirect-fix&id=a3652d03cc35fd3ad62744986c8ccaca74c9f20c > > > > > > I will be working on this towards the end of this week. > > > > Toke and I have been discussing how to solve this. > > > > The main idea for fixing this is to tie resource allocation to > > interface > > insertion into interface maps (kernel/bpf/devmap.c). As the > > =devmap= > > already have the needed synchronisation mechanisms and steps for > > safely > > adding and removing =net_devices= (e.g. stopping RX side, flushing > > remaining frames, waiting RCU period before freeing objects, etc.) > > > > As described here: > > > > https://github.com/xdp-project/xdp-project/blob/master/xdp-project.org#better-ndo_xdp_xmit-resource-management > > > > --Jesper > > Yes you already suggested this approach @LPC: > > So > 1) on dev_map_update_elem() we will call > dev->dev->ndo_bpf() to notify the device on the intention to > start/stop > redirect, and wait for it to create/destroy the HW resources > before/after actually updating the map > silly me, dev_map_update_elem must be atomic, we can't hook driver resource allocation to it, it must come as a separate request (syscall) from user space to request to create XDP redirect resources. > But: > 2) this won't totally solve our problem, since sometimes the driver > can > decide to recreate (change of configuration) hw resources on the fly > while redirect/devmap is already happening, so we need some kind of a > dev_map_notification or a flag with rcu synch, for when the driver > want > to make the xdp redirect resources unavailable. > I will focus on this problem first, then figure out how to create XDP redirect resources without actullay attaching a dummy xdp program. > Thanks, > Saeed. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Resource management for ndo_xdp_xmit (Was: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) 2019-02-08 23:17 ` Saeed Mahameed @ 2019-02-09 0:18 ` Saeed Mahameed 2019-02-09 2:05 ` Jakub Kicinski 2019-02-09 16:56 ` Toke Høiland-Jørgensen 0 siblings, 2 replies; 27+ messages in thread From: Saeed Mahameed @ 2019-02-09 0:18 UTC (permalink / raw) To: brouer Cc: thoiland, hawk, virtualization, borkmann, Tariq Toukan, toke, john.fastabend, mst, jakub.kicinski, dsahern, netdev, jasowang, davem, makita.toshiaki On Fri, 2019-02-08 at 15:17 -0800, Saeed Mahameed wrote: > On Thu, 2019-02-07 at 19:08 +0000, Saeed Mahameed wrote: > > > > So > > 1) on dev_map_update_elem() we will call > > dev->dev->ndo_bpf() to notify the device on the intention to > > start/stop > > redirect, and wait for it to create/destroy the HW resources > > before/after actually updating the map > > > > silly me, dev_map_update_elem must be atomic, we can't hook driver > resource allocation to it, it must come as a separate request > (syscall) > from user space to request to create XDP redirect resources. > Well, it is possible to render dev_map_update_elem non-atomic and fail BPF programs who try to update it in the verifier check_map_func_compatibility. if you know of any case where devmap needs to be updated from the BPF program please let me know. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Resource management for ndo_xdp_xmit (Was: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) 2019-02-09 0:18 ` Saeed Mahameed @ 2019-02-09 2:05 ` Jakub Kicinski 2019-02-09 16:56 ` Toke Høiland-Jørgensen 2019-02-09 16:56 ` Toke Høiland-Jørgensen 1 sibling, 1 reply; 27+ messages in thread From: Jakub Kicinski @ 2019-02-09 2:05 UTC (permalink / raw) To: Saeed Mahameed Cc: brouer, thoiland, hawk, virtualization, borkmann, Tariq Toukan, toke, john.fastabend, mst, dsahern, netdev, jasowang, davem, makita.toshiaki On Sat, 9 Feb 2019 00:18:31 +0000, Saeed Mahameed wrote: > On Fri, 2019-02-08 at 15:17 -0800, Saeed Mahameed wrote: > > On Thu, 2019-02-07 at 19:08 +0000, Saeed Mahameed wrote: > > > > > > So > > > 1) on dev_map_update_elem() we will call > > > dev->dev->ndo_bpf() to notify the device on the intention to > > > start/stop > > > redirect, and wait for it to create/destroy the HW resources > > > before/after actually updating the map > > > > > > > silly me, dev_map_update_elem must be atomic, we can't hook driver > > resource allocation to it, it must come as a separate request > > (syscall) > > from user space to request to create XDP redirect resources. > > > > Well, it is possible to render dev_map_update_elem non-atomic and fail > BPF programs who try to update it in the verifier > check_map_func_compatibility. > > if you know of any case where devmap needs to be updated from the BPF > program please let me know. Did we find a solution to non-map redirect? Sorry if I missed the discussion, I couldn't make the iovisor call this week due to travel. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Resource management for ndo_xdp_xmit (Was: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) 2019-02-09 2:05 ` Jakub Kicinski @ 2019-02-09 16:56 ` Toke Høiland-Jørgensen 0 siblings, 0 replies; 27+ messages in thread From: Toke Høiland-Jørgensen @ 2019-02-09 16:56 UTC (permalink / raw) To: Jakub Kicinski, Saeed Mahameed Cc: brouer, hawk, virtualization, borkmann, Tariq Toukan, john.fastabend, mst, dsahern, netdev, jasowang, davem, makita.toshiaki Jakub Kicinski <jakub.kicinski@netronome.com> writes: > On Sat, 9 Feb 2019 00:18:31 +0000, Saeed Mahameed wrote: >> On Fri, 2019-02-08 at 15:17 -0800, Saeed Mahameed wrote: >> > On Thu, 2019-02-07 at 19:08 +0000, Saeed Mahameed wrote: >> > > >> > > So >> > > 1) on dev_map_update_elem() we will call >> > > dev->dev->ndo_bpf() to notify the device on the intention to >> > > start/stop >> > > redirect, and wait for it to create/destroy the HW resources >> > > before/after actually updating the map >> > > >> > >> > silly me, dev_map_update_elem must be atomic, we can't hook driver >> > resource allocation to it, it must come as a separate request >> > (syscall) >> > from user space to request to create XDP redirect resources. >> > >> >> Well, it is possible to render dev_map_update_elem non-atomic and fail >> BPF programs who try to update it in the verifier >> check_map_func_compatibility. >> >> if you know of any case where devmap needs to be updated from the BPF >> program please let me know. > > Did we find a solution to non-map redirect? See my other reply to Saeed :) -Toke ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Resource management for ndo_xdp_xmit (Was: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) 2019-02-09 0:18 ` Saeed Mahameed 2019-02-09 2:05 ` Jakub Kicinski @ 2019-02-09 16:56 ` Toke Høiland-Jørgensen 1 sibling, 0 replies; 27+ messages in thread From: Toke Høiland-Jørgensen @ 2019-02-09 16:56 UTC (permalink / raw) To: Saeed Mahameed, brouer Cc: hawk, virtualization, borkmann, Tariq Toukan, john.fastabend, mst, jakub.kicinski, dsahern, netdev, jasowang, davem, makita.toshiaki Saeed Mahameed <saeedm@mellanox.com> writes: > On Fri, 2019-02-08 at 15:17 -0800, Saeed Mahameed wrote: >> On Thu, 2019-02-07 at 19:08 +0000, Saeed Mahameed wrote: >> > >> > So >> > 1) on dev_map_update_elem() we will call >> > dev->dev->ndo_bpf() to notify the device on the intention to >> > start/stop >> > redirect, and wait for it to create/destroy the HW resources >> > before/after actually updating the map >> > >> >> silly me, dev_map_update_elem must be atomic, we can't hook driver >> resource allocation to it, it must come as a separate request >> (syscall) >> from user space to request to create XDP redirect resources. >> > > Well, it is possible to render dev_map_update_elem non-atomic and fail > BPF programs who try to update it in the verifier > check_map_func_compatibility. > > if you know of any case where devmap needs to be updated from the BPF > program please let me know. Well, maybe? My plan for dealing with the non-map redirect variant is essentially to have a hidden map that does just-in-time insertion of ifindexes if they are not already there; and rework XDP_REDIRECT to use that. However, this would essentially be an insert from eBPF; but I guess maybe it could be deferred until the RX-side driver exits its NAPI loop (as we're buffering frames in the map anyway)? -Toke ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames 2019-02-06 0:06 ` Saeed Mahameed 2019-02-06 13:48 ` Jesper Dangaard Brouer 2019-02-07 7:48 ` Resource management for ndo_xdp_xmit (Was: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) Jesper Dangaard Brouer @ 2019-02-08 16:02 ` Jesper Dangaard Brouer 2 siblings, 0 replies; 27+ messages in thread From: Jesper Dangaard Brouer @ 2019-02-08 16:02 UTC (permalink / raw) To: Saeed Mahameed Cc: dsahern, thoiland, hawk, virtualization, borkmann, Tariq Toukan, john.fastabend, mst, jakub.kicinski, netdev, jasowang, davem, makita.toshiaki, brouer On Wed, 6 Feb 2019 00:06:33 +0000 Saeed Mahameed <saeedm@mellanox.com> wrote: > 2) Driver should keep track of XDP decisions statistics, report them in > ethtool and in the new API suggested by David. track even (XDP_PASS) ? > > Maybe instead of having all drivers track the statistics on their own, > we should move the responsibility to upper layer. > > Idea: since we already have rxq_info structure per XDP ring (no false > sharing) and available per xdp_buff we can do: > > +++ b/include/linux/filter.h > @@ -651,7 +651,9 @@ 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 ret = BPF_PROG_RUN(prog, xdp); > + xdp->xdp_rxq_info.stats[ret]++ > + return ret; > } > > still we need a way (API) to report the rxq_info to whoever needs to > read current XDP stats I'm capturing this as tasks under the XDP-project github page: https://github.com/xdp-project/xdp-project/pull/13/files -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer ^ permalink raw reply [flat|nested] 27+ messages in thread
* Stats for XDP actions (was: Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) 2019-02-05 3:13 ` David Ahern 2019-02-06 0:06 ` Saeed Mahameed @ 2019-04-18 14:24 ` Toke Høiland-Jørgensen 2019-04-21 0:16 ` Stats for XDP actions David Ahern 1 sibling, 1 reply; 27+ messages in thread From: Toke Høiland-Jørgensen @ 2019-04-18 14:24 UTC (permalink / raw) To: David Ahern, Jesper Dangaard Brouer Cc: David Miller, mst, makita.toshiaki, jasowang, netdev, virtualization, hawk, Jakub Kicinski, John Fastabend, Daniel Borkmann, Saeed Mahameed, Tariq Toukan David Ahern <dsahern@gmail.com> writes: > On 2/4/19 3:53 AM, Jesper Dangaard Brouer wrote: >> On Sat, 2 Feb 2019 14:27:26 -0700 >> David Ahern <dsahern@gmail.com> wrote: >> >>> On 1/31/19 1:15 PM, Jesper Dangaard Brouer wrote: >>>>> >>>>> David, Jesper, care to chime in where we ended up in that last thread >>>>> discussion this? >>>> >>>> IHMO packets RX and TX on a device need to be accounted, in standard >>>> counters, regardless of XDP. For XDP RX the packet is counted as RX, >>>> regardless if XDP choose to XDP_DROP. On XDP TX which is via >>>> XDP_REDIRECT or XDP_TX, the driver that transmit the packet need to >>>> account the packet in a TX counter (this if often delayed to DMA TX >>>> completion handling). We cannot break the expectation that RX and TX >>>> counter are visible to userspace stats tools. XDP should not make these >>>> packets invisible. >>> >>> Agreed. What I was pushing on that last thread was Rx, Tx and dropped >>> are all accounted by the driver in standard stats. Basically if the >>> driver touched it, the driver's counters should indicate that. >> >> Sound like we all agree (except with the dropped counter, see below). >> >> Do notice that mlx5 driver doesn't do this. It is actually rather >> confusing to use XDP on mlx5, as when XDP "consume" which include >> XDP_DROP, XDP_REDIRECT or XDP_TX, then the driver standard stats are >> not incremented... the packet is invisible to "ifconfig" stat based >> tools. > > mlx5 needs some work. As I recall it still has the bug/panic removing > xdp programs - at least I don't recall seeing a patch for it. > >> >> >>> The push back was on dropped packets and whether that counter should be >>> bumped on XDP_DROP. >> >> My opinion is the XDP_DROP action should NOT increment the drivers drop >> counter. First of all the "dropped" counter is also use for other >> stuff, which will confuse that this counter express. Second, choosing >> XDP_DROP is a policy choice, it still means it was RX-ed at the driver >> level. >> > > Understood. Hopefully in March I will get some time to come back to this > and propose an idea on what I would like to see - namely, the admin has > a config option at load time to enable driver counters versus custom map > counters. (meaning the operator of the node chooses standard stats over > strict performance.) But of course that means the drivers have the code > to collect those stats. Hi David I don't recall seeing any follow-up on this. Did you have a chance to formulate your ideas? :) -Toke ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Stats for XDP actions 2019-04-18 14:24 ` Stats for XDP actions (was: Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) Toke Høiland-Jørgensen @ 2019-04-21 0:16 ` David Ahern 2019-06-20 20:42 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 27+ messages in thread From: David Ahern @ 2019-04-21 0:16 UTC (permalink / raw) To: Toke Høiland-Jørgensen, Jesper Dangaard Brouer Cc: David Miller, mst, makita.toshiaki, jasowang, netdev, virtualization, hawk, Jakub Kicinski, John Fastabend, Daniel Borkmann, Saeed Mahameed, Tariq Toukan On 4/18/19 8:24 AM, Toke Høiland-Jørgensen wrote: >>> >> >> Understood. Hopefully in March I will get some time to come back to this >> and propose an idea on what I would like to see - namely, the admin has >> a config option at load time to enable driver counters versus custom map >> counters. (meaning the operator of the node chooses standard stats over >> strict performance.) But of course that means the drivers have the code >> to collect those stats. > > Hi David > > I don't recall seeing any follow-up on this. Did you have a chance to > formulate your ideas? :) > Not yet. Almost done with the nexthop changes. Once that is out of the way I can come back to this. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Stats for XDP actions 2019-04-21 0:16 ` Stats for XDP actions David Ahern @ 2019-06-20 20:42 ` Toke Høiland-Jørgensen 2019-06-21 0:42 ` David Ahern 0 siblings, 1 reply; 27+ messages in thread From: Toke Høiland-Jørgensen @ 2019-06-20 20:42 UTC (permalink / raw) To: David Ahern, Jesper Dangaard Brouer Cc: David Miller, mst, makita.toshiaki, jasowang, netdev, virtualization, hawk, Jakub Kicinski, John Fastabend, Daniel Borkmann, Saeed Mahameed, Tariq Toukan David Ahern <dsahern@gmail.com> writes: > On 4/18/19 8:24 AM, Toke Høiland-Jørgensen wrote: >>>> >>> >>> Understood. Hopefully in March I will get some time to come back to this >>> and propose an idea on what I would like to see - namely, the admin has >>> a config option at load time to enable driver counters versus custom map >>> counters. (meaning the operator of the node chooses standard stats over >>> strict performance.) But of course that means the drivers have the code >>> to collect those stats. >> >> Hi David >> >> I don't recall seeing any follow-up on this. Did you have a chance to >> formulate your ideas? :) >> > > Not yet. Almost done with the nexthop changes. Once that is out of the > way I can come back to this. Ping? :) -Toke ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Stats for XDP actions 2019-06-20 20:42 ` Toke Høiland-Jørgensen @ 2019-06-21 0:42 ` David Ahern 2019-06-21 13:57 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 27+ messages in thread From: David Ahern @ 2019-06-21 0:42 UTC (permalink / raw) To: Toke Høiland-Jørgensen, Jesper Dangaard Brouer Cc: David Miller, mst, makita.toshiaki, jasowang, netdev, virtualization, hawk, Jakub Kicinski, John Fastabend, Daniel Borkmann, Saeed Mahameed, Tariq Toukan On 6/20/19 2:42 PM, Toke Høiland-Jørgensen wrote: >>> I don't recall seeing any follow-up on this. Did you have a chance to >>> formulate your ideas? :) >>> >> >> Not yet. Almost done with the nexthop changes. Once that is out of the >> way I can come back to this. > > Ping? :) > Definitely back to this after the July 4th holiday. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Stats for XDP actions 2019-06-21 0:42 ` David Ahern @ 2019-06-21 13:57 ` Toke Høiland-Jørgensen 0 siblings, 0 replies; 27+ messages in thread From: Toke Høiland-Jørgensen @ 2019-06-21 13:57 UTC (permalink / raw) To: David Ahern, Jesper Dangaard Brouer Cc: David Miller, mst, makita.toshiaki, jasowang, netdev, virtualization, hawk, Jakub Kicinski, John Fastabend, Daniel Borkmann, Saeed Mahameed, Tariq Toukan David Ahern <dsahern@gmail.com> writes: > On 6/20/19 2:42 PM, Toke Høiland-Jørgensen wrote: >>>> I don't recall seeing any follow-up on this. Did you have a chance to >>>> formulate your ideas? :) >>>> >>> >>> Not yet. Almost done with the nexthop changes. Once that is out of the >>> way I can come back to this. >> >> Ping? :) >> > > Definitely back to this after the July 4th holiday. Awesome! I'll wait until then before bugging you again... ;) -Toke ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames 2019-01-31 11:40 [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames Toshiaki Makita 2019-01-31 15:25 ` Michael S. Tsirkin @ 2019-02-04 4:18 ` David Miller 1 sibling, 0 replies; 27+ messages in thread From: David Miller @ 2019-02-04 4:18 UTC (permalink / raw) To: makita.toshiaki; +Cc: mst, jasowang, netdev, virtualization, dsahern From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> Date: Thu, 31 Jan 2019 20:40:30 +0900 > Previously virtnet_xdp_xmit() did not account for device tx counters, > which caused confusions. > To be consistent with SKBs, account them on freeing xdp_frames. > > Reported-by: David Ahern <dsahern@gmail.com> > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> Applied, thank you. ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2019-06-21 13:57 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-31 11:40 [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames Toshiaki Makita 2019-01-31 15:25 ` Michael S. Tsirkin 2019-01-31 17:45 ` David Miller 2019-01-31 20:15 ` Jesper Dangaard Brouer 2019-02-01 1:53 ` Toshiaki Makita 2019-02-02 21:27 ` David Ahern 2019-02-04 11:53 ` Jesper Dangaard Brouer 2019-02-05 3:13 ` David Ahern 2019-02-06 0:06 ` Saeed Mahameed 2019-02-06 13:48 ` Jesper Dangaard Brouer 2019-02-06 15:52 ` Jakub Kicinski 2019-02-07 7:48 ` Resource management for ndo_xdp_xmit (Was: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) Jesper Dangaard Brouer 2019-02-07 19:08 ` Saeed Mahameed 2019-02-08 16:55 ` Toke Høiland-Jørgensen 2019-02-08 22:49 ` Saeed Mahameed 2019-02-08 23:17 ` Saeed Mahameed 2019-02-09 0:18 ` Saeed Mahameed 2019-02-09 2:05 ` Jakub Kicinski 2019-02-09 16:56 ` Toke Høiland-Jørgensen 2019-02-09 16:56 ` Toke Høiland-Jørgensen 2019-02-08 16:02 ` [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames Jesper Dangaard Brouer 2019-04-18 14:24 ` Stats for XDP actions (was: Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames) Toke Høiland-Jørgensen 2019-04-21 0:16 ` Stats for XDP actions David Ahern 2019-06-20 20:42 ` Toke Høiland-Jørgensen 2019-06-21 0:42 ` David Ahern 2019-06-21 13:57 ` Toke Høiland-Jørgensen 2019-02-04 4:18 ` [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).