consistency for statistics with XDP mode

consistency for statistics with XDP mode

[David Ahern]

Paweł ran some more XDP tests yesterday and from it found a couple of
issues. One is a panic in the mlx5 driver unloading the bpf program
(mlx5e_xdp_xmit); he will send a send a separate email for that problem.

The problem I wanted to discuss here is statistics for XDP context. The
short of it is that we need consistency in the counters across NIC
drivers and virtual devices. Right now stats are specific to a driver
with no clear accounting for the packets and bytes handled in XDP.

For example virtio has some stats as device private data extracted via
ethtool:
$ ethtool -S eth2 | grep xdp
    ...
     rx_queue_3_xdp_packets: 5291
     rx_queue_3_xdp_tx: 0
     rx_queue_3_xdp_redirects: 5163
     rx_queue_3_xdp_drops: 0
    ...
     tx_queue_3_xdp_tx: 5163
     tx_queue_3_xdp_tx_drops: 0

And the standard counters appear to track bytes and packets for Rx, but
not Tx if the packet is forwarded in XDP.

Similarly, mlx5 has some counters (thanks to Jesper and Toke for helping
out here):

$ ethtool -S mlx5p1 | grep xdp
     rx_xdp_drop: 86468350180
     rx_xdp_redirect: 18860584
     rx_xdp_tx_xmit: 0
     rx_xdp_tx_full: 0
     rx_xdp_tx_err: 0
     rx_xdp_tx_cqe: 0
     tx_xdp_xmit: 0
     tx_xdp_full: 0
     tx_xdp_err: 0
     tx_xdp_cqes: 0
    ...
     rx3_xdp_drop: 86468350180
     rx3_xdp_redirect: 18860556
     rx3_xdp_tx_xmit: 0
     rx3_xdp_tx_full: 0
     rx3_xdp_tx_err: 0
     rx3_xdp_tx_cqes: 0
    ...
     tx0_xdp_xmit: 0
     tx0_xdp_full: 0
     tx0_xdp_err: 0
     tx0_xdp_cqes: 0
    ...

And no accounting in standard stats for packets handled in XDP.

And then if I understand Jesper's data correctly, the i40e driver does
not have device specific data:

$ ethtool -S i40e1  | grep xdp
[NOTHING]


But rather bumps the standard counters:

sudo ./xdp_rxq_info --dev i40e1 --action XDP_DROP

Running XDP on dev:i40e1 (ifindex:3) action:XDP_DROP options:no_touch
XDP stats       CPU     pps         issue-pps
XDP-RX CPU      1       36,156,872  0
XDP-RX CPU      total   36,156,872

RXQ stats       RXQ:CPU pps         issue-pps
rx_queue_index    1:1   36,156,878  0
rx_queue_index    1:sum 36,156,878


$ ethtool_stats.pl --dev i40e1

Show adapter(s) (i40e1) statistics (ONLY that changed!)
Ethtool(i40e1   ) stat:   2711292859 (  2,711,292,859) <= port.rx_bytes /sec
Ethtool(i40e1   ) stat:      6274204 (      6,274,204) <=
port.rx_dropped /sec
Ethtool(i40e1   ) stat:     42363867 (     42,363,867) <=
port.rx_size_64 /sec
Ethtool(i40e1   ) stat:     42363950 (     42,363,950) <=
port.rx_unicast /sec
Ethtool(i40e1   ) stat:   2165051990 (  2,165,051,990) <= rx-1.bytes /sec
Ethtool(i40e1   ) stat:     36084200 (     36,084,200) <= rx-1.packets /sec
Ethtool(i40e1   ) stat:         5385 (          5,385) <= rx_dropped /sec
Ethtool(i40e1   ) stat:     36089727 (     36,089,727) <= rx_unicast /sec


We really need consistency in the counters and at a minimum, users
should be able to track packet and byte counters for both Rx and Tx
including XDP.

It seems to me the Rx and Tx packet, byte and dropped counters returned
for the standard device stats (/proc/net/dev, ip -s li show, ...) should
include all packets managed by the driver regardless of whether they are
forwarded / dropped in XDP or go up the Linux stack. This also aligns
with mlxsw and the stats it shows which are packets handled by the hardware.

>From there the private stats can include XDP specifics as desired --
like the drops and redirects but that those should be add-ons and even
here some consistency makes life easier for users.

The same standards should be also be applied to virtual devices built on
top of the ports -- e.g,  vlans. I have an API now that allows bumping
stats for vlan devices.

Keeping the basic xdp packets in the standard counters allows Paweł, for
example, to continue to monitor /proc/net/dev.

Can we get agreement on this? And from there, get updates to the mlx5
and virtio drivers?

David

Re: consistency for statistics with XDP mode

[Toke Høiland-Jørgensen]

David Ahern <dsahern@gmail.com> writes:

> Paweł ran some more XDP tests yesterday and from it found a couple of
> issues. One is a panic in the mlx5 driver unloading the bpf program
> (mlx5e_xdp_xmit); he will send a send a separate email for that
> problem.

Same as this one, I guess?

https://marc.info/?l=linux-netdev&m=153855905619717&w=2

> The problem I wanted to discuss here is statistics for XDP context. The
> short of it is that we need consistency in the counters across NIC
> drivers and virtual devices. Right now stats are specific to a driver
> with no clear accounting for the packets and bytes handled in XDP.
>
> For example virtio has some stats as device private data extracted via
> ethtool:
> $ ethtool -S eth2 | grep xdp
>     ...
>      rx_queue_3_xdp_packets: 5291
>      rx_queue_3_xdp_tx: 0
>      rx_queue_3_xdp_redirects: 5163
>      rx_queue_3_xdp_drops: 0
>     ...
>      tx_queue_3_xdp_tx: 5163
>      tx_queue_3_xdp_tx_drops: 0
>
> And the standard counters appear to track bytes and packets for Rx, but
> not Tx if the packet is forwarded in XDP.
>
> Similarly, mlx5 has some counters (thanks to Jesper and Toke for helping
> out here):
>
> $ ethtool -S mlx5p1 | grep xdp
>      rx_xdp_drop: 86468350180
>      rx_xdp_redirect: 18860584
>      rx_xdp_tx_xmit: 0
>      rx_xdp_tx_full: 0
>      rx_xdp_tx_err: 0
>      rx_xdp_tx_cqe: 0
>      tx_xdp_xmit: 0
>      tx_xdp_full: 0
>      tx_xdp_err: 0
>      tx_xdp_cqes: 0
>     ...
>      rx3_xdp_drop: 86468350180
>      rx3_xdp_redirect: 18860556
>      rx3_xdp_tx_xmit: 0
>      rx3_xdp_tx_full: 0
>      rx3_xdp_tx_err: 0
>      rx3_xdp_tx_cqes: 0
>     ...
>      tx0_xdp_xmit: 0
>      tx0_xdp_full: 0
>      tx0_xdp_err: 0
>      tx0_xdp_cqes: 0
>     ...
>
> And no accounting in standard stats for packets handled in XDP.
>
> And then if I understand Jesper's data correctly, the i40e driver does
> not have device specific data:
>
> $ ethtool -S i40e1  | grep xdp
> [NOTHING]
>
>
> But rather bumps the standard counters:
>
> sudo ./xdp_rxq_info --dev i40e1 --action XDP_DROP
>
> Running XDP on dev:i40e1 (ifindex:3) action:XDP_DROP options:no_touch
> XDP stats       CPU     pps         issue-pps
> XDP-RX CPU      1       36,156,872  0
> XDP-RX CPU      total   36,156,872
>
> RXQ stats       RXQ:CPU pps         issue-pps
> rx_queue_index    1:1   36,156,878  0
> rx_queue_index    1:sum 36,156,878
>
>
> $ ethtool_stats.pl --dev i40e1
>
> Show adapter(s) (i40e1) statistics (ONLY that changed!)
> Ethtool(i40e1   ) stat:   2711292859 (  2,711,292,859) <= port.rx_bytes /sec
> Ethtool(i40e1   ) stat:      6274204 (      6,274,204) <=
> port.rx_dropped /sec
> Ethtool(i40e1   ) stat:     42363867 (     42,363,867) <=
> port.rx_size_64 /sec
> Ethtool(i40e1   ) stat:     42363950 (     42,363,950) <=
> port.rx_unicast /sec
> Ethtool(i40e1   ) stat:   2165051990 (  2,165,051,990) <= rx-1.bytes /sec
> Ethtool(i40e1   ) stat:     36084200 (     36,084,200) <= rx-1.packets /sec
> Ethtool(i40e1   ) stat:         5385 (          5,385) <= rx_dropped /sec
> Ethtool(i40e1   ) stat:     36089727 (     36,089,727) <= rx_unicast /sec
>
>
> We really need consistency in the counters and at a minimum, users
> should be able to track packet and byte counters for both Rx and Tx
> including XDP.
>
> It seems to me the Rx and Tx packet, byte and dropped counters returned
> for the standard device stats (/proc/net/dev, ip -s li show, ...) should
> include all packets managed by the driver regardless of whether they are
> forwarded / dropped in XDP or go up the Linux stack. This also aligns
> with mlxsw and the stats it shows which are packets handled by the hardware.
>
> From there the private stats can include XDP specifics as desired --
> like the drops and redirects but that those should be add-ons and even
> here some consistency makes life easier for users.
>
> The same standards should be also be applied to virtual devices built on
> top of the ports -- e.g,  vlans. I have an API now that allows bumping
> stats for vlan devices.
>
> Keeping the basic xdp packets in the standard counters allows Paweł, for
> example, to continue to monitor /proc/net/dev.
>
> Can we get agreement on this? And from there, get updates to the mlx5
> and virtio drivers?

I'd say it sounds reasonable to include XDP in the normal traffic
counters, but having the detailed XDP-specific counters is quite useful
as well... So can't we do both (for all drivers)?

-Toke

Re: consistency for statistics with XDP mode

[Paweł Staszewski]

W dniu 21.11.2018 o 22:14, Toke Høiland-Jørgensen pisze:
> David Ahern <dsahern@gmail.com> writes:
>
>> Paweł ran some more XDP tests yesterday and from it found a couple of
>> issues. One is a panic in the mlx5 driver unloading the bpf program
>> (mlx5e_xdp_xmit); he will send a send a separate email for that
>> problem.
> Same as this one, I guess?
>
> https://marc.info/?l=linux-netdev&m=153855905619717&w=2

Yes same as this one.

When there is no traffic (for example with xdp_fwd program loaded) or 
there is not much traffic like 1k frames per second for icmp - i can 
load/unload without crashing kernel

But when i push tests with pktgen and use more than >50k pps for udp - 
then unbinding xdp_fwd program makes kernel to panic :)



>
>> The problem I wanted to discuss here is statistics for XDP context. The
>> short of it is that we need consistency in the counters across NIC
>> drivers and virtual devices. Right now stats are specific to a driver
>> with no clear accounting for the packets and bytes handled in XDP.
>>
>> For example virtio has some stats as device private data extracted via
>> ethtool:
>> $ ethtool -S eth2 | grep xdp
>>      ...
>>       rx_queue_3_xdp_packets: 5291
>>       rx_queue_3_xdp_tx: 0
>>       rx_queue_3_xdp_redirects: 5163
>>       rx_queue_3_xdp_drops: 0
>>      ...
>>       tx_queue_3_xdp_tx: 5163
>>       tx_queue_3_xdp_tx_drops: 0
>>
>> And the standard counters appear to track bytes and packets for Rx, but
>> not Tx if the packet is forwarded in XDP.
>>
>> Similarly, mlx5 has some counters (thanks to Jesper and Toke for helping
>> out here):
>>
>> $ ethtool -S mlx5p1 | grep xdp
>>       rx_xdp_drop: 86468350180
>>       rx_xdp_redirect: 18860584
>>       rx_xdp_tx_xmit: 0
>>       rx_xdp_tx_full: 0
>>       rx_xdp_tx_err: 0
>>       rx_xdp_tx_cqe: 0
>>       tx_xdp_xmit: 0
>>       tx_xdp_full: 0
>>       tx_xdp_err: 0
>>       tx_xdp_cqes: 0
>>      ...
>>       rx3_xdp_drop: 86468350180
>>       rx3_xdp_redirect: 18860556
>>       rx3_xdp_tx_xmit: 0
>>       rx3_xdp_tx_full: 0
>>       rx3_xdp_tx_err: 0
>>       rx3_xdp_tx_cqes: 0
>>      ...
>>       tx0_xdp_xmit: 0
>>       tx0_xdp_full: 0
>>       tx0_xdp_err: 0
>>       tx0_xdp_cqes: 0
>>      ...
>>
>> And no accounting in standard stats for packets handled in XDP.
>>
>> And then if I understand Jesper's data correctly, the i40e driver does
>> not have device specific data:
>>
>> $ ethtool -S i40e1  | grep xdp
>> [NOTHING]
>>
>>
>> But rather bumps the standard counters:
>>
>> sudo ./xdp_rxq_info --dev i40e1 --action XDP_DROP
>>
>> Running XDP on dev:i40e1 (ifindex:3) action:XDP_DROP options:no_touch
>> XDP stats       CPU     pps         issue-pps
>> XDP-RX CPU      1       36,156,872  0
>> XDP-RX CPU      total   36,156,872
>>
>> RXQ stats       RXQ:CPU pps         issue-pps
>> rx_queue_index    1:1   36,156,878  0
>> rx_queue_index    1:sum 36,156,878
>>
>>
>> $ ethtool_stats.pl --dev i40e1
>>
>> Show adapter(s) (i40e1) statistics (ONLY that changed!)
>> Ethtool(i40e1   ) stat:   2711292859 (  2,711,292,859) <= port.rx_bytes /sec
>> Ethtool(i40e1   ) stat:      6274204 (      6,274,204) <=
>> port.rx_dropped /sec
>> Ethtool(i40e1   ) stat:     42363867 (     42,363,867) <=
>> port.rx_size_64 /sec
>> Ethtool(i40e1   ) stat:     42363950 (     42,363,950) <=
>> port.rx_unicast /sec
>> Ethtool(i40e1   ) stat:   2165051990 (  2,165,051,990) <= rx-1.bytes /sec
>> Ethtool(i40e1   ) stat:     36084200 (     36,084,200) <= rx-1.packets /sec
>> Ethtool(i40e1   ) stat:         5385 (          5,385) <= rx_dropped /sec
>> Ethtool(i40e1   ) stat:     36089727 (     36,089,727) <= rx_unicast /sec
>>
>>
>> We really need consistency in the counters and at a minimum, users
>> should be able to track packet and byte counters for both Rx and Tx
>> including XDP.
>>
>> It seems to me the Rx and Tx packet, byte and dropped counters returned
>> for the standard device stats (/proc/net/dev, ip -s li show, ...) should
>> include all packets managed by the driver regardless of whether they are
>> forwarded / dropped in XDP or go up the Linux stack. This also aligns
>> with mlxsw and the stats it shows which are packets handled by the hardware.
>>
>>  From there the private stats can include XDP specifics as desired --
>> like the drops and redirects but that those should be add-ons and even
>> here some consistency makes life easier for users.
>>
>> The same standards should be also be applied to virtual devices built on
>> top of the ports -- e.g,  vlans. I have an API now that allows bumping
>> stats for vlan devices.
>>
>> Keeping the basic xdp packets in the standard counters allows Paweł, for
>> example, to continue to monitor /proc/net/dev.
>>
>> Can we get agreement on this? And from there, get updates to the mlx5
>> and virtio drivers?
> I'd say it sounds reasonable to include XDP in the normal traffic
> counters, but having the detailed XDP-specific counters is quite useful
> as well... So can't we do both (for all drivers)?
>
> -Toke
>

Re: consistency for statistics with XDP mode

[Saeed Mahameed]

On Wed, 2018-11-21 at 22:29 +0100, Paweł Staszewski wrote:
> W dniu 21.11.2018 o 22:14, Toke Høiland-Jørgensen pisze:
> > David Ahern <dsahern@gmail.com> writes:
> > 
> > > Paweł ran some more XDP tests yesterday and from it found a
> > > couple of
> > > issues. One is a panic in the mlx5 driver unloading the bpf
> > > program
> > > (mlx5e_xdp_xmit); he will send a send a separate email for that
> > > problem.
> > Same as this one, I guess?
> > 
> > https://marc.info/?l=linux-netdev&m=153855905619717&w=2
> 
> Yes same as this one.
> 
> When there is no traffic (for example with xdp_fwd program loaded)
> or 
> there is not much traffic like 1k frames per second for icmp - i can 
> load/unload without crashing kernel
> 
> But when i push tests with pktgen and use more than >50k pps for udp
> - 
> then unbinding xdp_fwd program makes kernel to panic :)
> 

Yea, this is not precisely mlx5 issue. this is one of the issues we
discussed at LPC, and i think we all agreed that the XDP redirect
infrastructure must allow different driver to sync when they are
changing configurations or disabling XPD tx for a moment, so the fix
must be in the XDP redirect infrastructure.

here is the issue description and a temp fix that i provided to Toke:
https://marc.info/?l=linux-netdev&m=154023109526642&w=2

patch:
https://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git/commit/?h=topic/xdp-redirect-fix&id=a3652d03cc35fd3ad62744986c8ccaca74c9f20c


> 
> 
> > > The problem I wanted to discuss here is statistics for XDP
> > > context. The
> > > short of it is that we need consistency in the counters across
> > > NIC
> > > drivers and virtual devices. Right now stats are specific to a
> > > driver
> > > with no clear accounting for the packets and bytes handled in
> > > XDP.
> > > 
> > > For example virtio has some stats as device private data
> > > extracted via
> > > ethtool:
> > > $ ethtool -S eth2 | grep xdp
> > >      ...
> > >       rx_queue_3_xdp_packets: 5291
> > >       rx_queue_3_xdp_tx: 0
> > >       rx_queue_3_xdp_redirects: 5163
> > >       rx_queue_3_xdp_drops: 0
> > >      ...
> > >       tx_queue_3_xdp_tx: 5163
> > >       tx_queue_3_xdp_tx_drops: 0
> > > 
> > > And the standard counters appear to track bytes and packets for
> > > Rx, but
> > > not Tx if the packet is forwarded in XDP.
> > > 
> > > Similarly, mlx5 has some counters (thanks to Jesper and Toke for
> > > helping
> > > out here):
> > > 
> > > $ ethtool -S mlx5p1 | grep xdp
> > >       rx_xdp_drop: 86468350180
> > >       rx_xdp_redirect: 18860584
> > >       rx_xdp_tx_xmit: 0
> > >       rx_xdp_tx_full: 0
> > >       rx_xdp_tx_err: 0
> > >       rx_xdp_tx_cqe: 0
> > >       tx_xdp_xmit: 0
> > >       tx_xdp_full: 0
> > >       tx_xdp_err: 0
> > >       tx_xdp_cqes: 0
> > >      ...
> > >       rx3_xdp_drop: 86468350180
> > >       rx3_xdp_redirect: 18860556
> > >       rx3_xdp_tx_xmit: 0
> > >       rx3_xdp_tx_full: 0
> > >       rx3_xdp_tx_err: 0
> > >       rx3_xdp_tx_cqes: 0
> > >      ...
> > >       tx0_xdp_xmit: 0
> > >       tx0_xdp_full: 0
> > >       tx0_xdp_err: 0
> > >       tx0_xdp_cqes: 0
> > >      ...
> > > 
> > > And no accounting in standard stats for packets handled in XDP.
> > > 
> > > And then if I understand Jesper's data correctly, the i40e driver
> > > does
> > > not have device specific data:
> > > 
> > > $ ethtool -S i40e1  | grep xdp
> > > [NOTHING]
> > > 
> > > 
> > > But rather bumps the standard counters:
> > > 
> > > sudo ./xdp_rxq_info --dev i40e1 --action XDP_DROP
> > > 
> > > Running XDP on dev:i40e1 (ifindex:3) action:XDP_DROP
> > > options:no_touch
> > > XDP stats       CPU     pps         issue-pps
> > > XDP-RX CPU      1       36,156,872  0
> > > XDP-RX CPU      total   36,156,872
> > > 
> > > RXQ stats       RXQ:CPU pps         issue-pps
> > > rx_queue_index    1:1   36,156,878  0
> > > rx_queue_index    1:sum 36,156,878
> > > 
> > > 
> > > $ ethtool_stats.pl --dev i40e1
> > > 
> > > Show adapter(s) (i40e1) statistics (ONLY that changed!)
> > > Ethtool(i40e1   ) stat:   2711292859 (  2,711,292,859) <=
> > > port.rx_bytes /sec
> > > Ethtool(i40e1   ) stat:      6274204 (      6,274,204) <=
> > > port.rx_dropped /sec
> > > Ethtool(i40e1   ) stat:     42363867 (     42,363,867) <=
> > > port.rx_size_64 /sec
> > > Ethtool(i40e1   ) stat:     42363950 (     42,363,950) <=
> > > port.rx_unicast /sec
> > > Ethtool(i40e1   ) stat:   2165051990 (  2,165,051,990) <= rx-
> > > 1.bytes /sec
> > > Ethtool(i40e1   ) stat:     36084200 (     36,084,200) <= rx-
> > > 1.packets /sec
> > > Ethtool(i40e1   ) stat:         5385 (          5,385) <=
> > > rx_dropped /sec
> > > Ethtool(i40e1   ) stat:     36089727 (     36,089,727) <=
> > > rx_unicast /sec
> > > 
> > > 
> > > We really need consistency in the counters and at a minimum,
> > > users
> > > should be able to track packet and byte counters for both Rx and
> > > Tx
> > > including XDP.
> > > 
> > > It seems to me the Rx and Tx packet, byte and dropped counters
> > > returned
> > > for the standard device stats (/proc/net/dev, ip -s li show, ...)
> > > should
> > > include all packets managed by the driver regardless of whether
> > > they are
> > > forwarded / dropped in XDP or go up the Linux stack. This also
> > > aligns
> > > with mlxsw and the stats it shows which are packets handled by
> > > the hardware.
> > > 
> > >  From there the private stats can include XDP specifics as
> > > desired --
> > > like the drops and redirects but that those should be add-ons and
> > > even
> > > here some consistency makes life easier for users.
> > > 
> > > The same standards should be also be applied to virtual devices
> > > built on
> > > top of the ports -- e.g,  vlans. I have an API now that allows
> > > bumping
> > > stats for vlan devices.
> > > 
> > > Keeping the basic xdp packets in the standard counters allows
> > > Paweł, for
> > > example, to continue to monitor /proc/net/dev.
> > > 
> > > Can we get agreement on this? And from there, get updates to the
> > > mlx5
> > > and virtio drivers?
> > I'd say it sounds reasonable to include XDP in the normal traffic
> > counters, but having the detailed XDP-specific counters is quite
> > useful
> > as well... So can't we do both (for all drivers)?
> > 

What are you thinking ? 
reporting XDP_DROP in interface dropped counter ?
and XDP_TX/REDIRECT in the TX counter ?
XDP_ABORTED in the  err/drop counter ?

how about having a special XDP command in the .ndo_bpf that would query
the standardized XDP stats ?

> > -Toke
> > 

Re: consistency for statistics with XDP mode

[Toshiaki Makita]

On 2018/11/22 6:06, David Ahern wrote:
> Paweł ran some more XDP tests yesterday and from it found a couple of
> issues. One is a panic in the mlx5 driver unloading the bpf program
> (mlx5e_xdp_xmit); he will send a send a separate email for that problem.
> 
> The problem I wanted to discuss here is statistics for XDP context. The
> short of it is that we need consistency in the counters across NIC
> drivers and virtual devices. Right now stats are specific to a driver
> with no clear accounting for the packets and bytes handled in XDP.
> 
> For example virtio has some stats as device private data extracted via
> ethtool:
> $ ethtool -S eth2 | grep xdp
>     ...
>      rx_queue_3_xdp_packets: 5291
>      rx_queue_3_xdp_tx: 0
>      rx_queue_3_xdp_redirects: 5163
>      rx_queue_3_xdp_drops: 0
>     ...
>      tx_queue_3_xdp_tx: 5163
>      tx_queue_3_xdp_tx_drops: 0
> 
> And the standard counters appear to track bytes and packets for Rx, but
> not Tx if the packet is forwarded in XDP.
> 
> Similarly, mlx5 has some counters (thanks to Jesper and Toke for helping
> out here):
> 
> $ ethtool -S mlx5p1 | grep xdp
>      rx_xdp_drop: 86468350180
>      rx_xdp_redirect: 18860584
>      rx_xdp_tx_xmit: 0
>      rx_xdp_tx_full: 0
>      rx_xdp_tx_err: 0
>      rx_xdp_tx_cqe: 0
>      tx_xdp_xmit: 0
>      tx_xdp_full: 0
>      tx_xdp_err: 0
>      tx_xdp_cqes: 0
>     ...
>      rx3_xdp_drop: 86468350180
>      rx3_xdp_redirect: 18860556
>      rx3_xdp_tx_xmit: 0
>      rx3_xdp_tx_full: 0
>      rx3_xdp_tx_err: 0
>      rx3_xdp_tx_cqes: 0
>     ...
>      tx0_xdp_xmit: 0
>      tx0_xdp_full: 0
>      tx0_xdp_err: 0
>      tx0_xdp_cqes: 0
>     ...
> 
> And no accounting in standard stats for packets handled in XDP.
> 
> And then if I understand Jesper's data correctly, the i40e driver does
> not have device specific data:
> 
> $ ethtool -S i40e1  | grep xdp
> [NOTHING]
> 
> 
> But rather bumps the standard counters:
> 
> sudo ./xdp_rxq_info --dev i40e1 --action XDP_DROP
> 
> Running XDP on dev:i40e1 (ifindex:3) action:XDP_DROP options:no_touch
> XDP stats       CPU     pps         issue-pps
> XDP-RX CPU      1       36,156,872  0
> XDP-RX CPU      total   36,156,872
> 
> RXQ stats       RXQ:CPU pps         issue-pps
> rx_queue_index    1:1   36,156,878  0
> rx_queue_index    1:sum 36,156,878
> 
> 
> $ ethtool_stats.pl --dev i40e1
> 
> Show adapter(s) (i40e1) statistics (ONLY that changed!)
> Ethtool(i40e1   ) stat:   2711292859 (  2,711,292,859) <= port.rx_bytes /sec
> Ethtool(i40e1   ) stat:      6274204 (      6,274,204) <=
> port.rx_dropped /sec
> Ethtool(i40e1   ) stat:     42363867 (     42,363,867) <=
> port.rx_size_64 /sec
> Ethtool(i40e1   ) stat:     42363950 (     42,363,950) <=
> port.rx_unicast /sec
> Ethtool(i40e1   ) stat:   2165051990 (  2,165,051,990) <= rx-1.bytes /sec
> Ethtool(i40e1   ) stat:     36084200 (     36,084,200) <= rx-1.packets /sec
> Ethtool(i40e1   ) stat:         5385 (          5,385) <= rx_dropped /sec
> Ethtool(i40e1   ) stat:     36089727 (     36,089,727) <= rx_unicast /sec
> 
> 
> We really need consistency in the counters and at a minimum, users
> should be able to track packet and byte counters for both Rx and Tx
> including XDP.
> 
> It seems to me the Rx and Tx packet, byte and dropped counters returned
> for the standard device stats (/proc/net/dev, ip -s li show, ...) should
> include all packets managed by the driver regardless of whether they are
> forwarded / dropped in XDP or go up the Linux stack. This also aligns

Agreed. When I introduced virtio_net XDP counters, I just forgot to
update tx packets/bytes counters on ndo_xdp_xmit. Probably I thought it
is handled by free_old_xmit_skbs.

Toshiaki Makita

> with mlxsw and the stats it shows which are packets handled by the hardware.
> 
>>From there the private stats can include XDP specifics as desired --
> like the drops and redirects but that those should be add-ons and even
> here some consistency makes life easier for users.
> 
> The same standards should be also be applied to virtual devices built on
> top of the ports -- e.g,  vlans. I have an API now that allows bumping
> stats for vlan devices.
> 
> Keeping the basic xdp packets in the standard counters allows Paweł, for
> example, to continue to monitor /proc/net/dev.
> 
> Can we get agreement on this? And from there, get updates to the mlx5
> and virtio drivers?
> 
> David
> 
> 

Re: consistency for statistics with XDP mode

[Toke Høiland-Jørgensen]

Saeed Mahameed <saeedm@mellanox.com> writes:

>> > I'd say it sounds reasonable to include XDP in the normal traffic
>> > counters, but having the detailed XDP-specific counters is quite
>> > useful
>> > as well... So can't we do both (for all drivers)?
>> > 
>
> What are you thinking ? 
> reporting XDP_DROP in interface dropped counter ?
> and XDP_TX/REDIRECT in the TX counter ?
> XDP_ABORTED in the  err/drop counter ?
>
> how about having a special XDP command in the .ndo_bpf that would query
> the standardized XDP stats ?

Don't have any strong opinions on the mechanism; just pointing out that
the XDP-specific stats are useful to have separately as well :)

-Toke

Re: consistency for statistics with XDP mode

[David Ahern]

On 11/21/18 5:53 PM, Toshiaki Makita wrote:
>> We really need consistency in the counters and at a minimum, users
>> should be able to track packet and byte counters for both Rx and Tx
>> including XDP.
>>
>> It seems to me the Rx and Tx packet, byte and dropped counters returned
>> for the standard device stats (/proc/net/dev, ip -s li show, ...) should
>> include all packets managed by the driver regardless of whether they are
>> forwarded / dropped in XDP or go up the Linux stack. This also aligns
> 
> Agreed. When I introduced virtio_net XDP counters, I just forgot to
> update tx packets/bytes counters on ndo_xdp_xmit. Probably I thought it
> is handled by free_old_xmit_skbs.

Do you have some time to look at adding the Tx counters to virtio_net?

Re: consistency for statistics with XDP mode

[David Ahern]

On 11/22/18 1:26 AM, Toke Høiland-Jørgensen wrote:
> Saeed Mahameed <saeedm@mellanox.com> writes:
> 
>>>> I'd say it sounds reasonable to include XDP in the normal traffic
>>>> counters, but having the detailed XDP-specific counters is quite
>>>> useful
>>>> as well... So can't we do both (for all drivers)?
>>>>
>>
>> What are you thinking ? 
>> reporting XDP_DROP in interface dropped counter ?
>> and XDP_TX/REDIRECT in the TX counter ?
>> XDP_ABORTED in the  err/drop counter ?
>>
>> how about having a special XDP command in the .ndo_bpf that would query
>> the standardized XDP stats ?
> 
> Don't have any strong opinions on the mechanism; just pointing out that
> the XDP-specific stats are useful to have separately as well :)
>

I would like to see basic packets, bytes, and dropped counters tracked
for Rx and Tx via the standard netdev counters for all devices. This is
for ease in accounting as well as speed and simplicity for bumping
counters for virtual devices from bpf helpers.

>From there, the XDP ones can be in the driver private stats as they are
currently but with some consistency across drivers for redirects, drops,
any thing else.

So not a radical departure from where we are today, just getting the
agreement for consistency and driver owners to make the changes.

Re: consistency for statistics with XDP mode

[Toke Høiland-Jørgensen]

David Ahern <dsahern@gmail.com> writes:

> On 11/22/18 1:26 AM, Toke Høiland-Jørgensen wrote:
>> Saeed Mahameed <saeedm@mellanox.com> writes:
>> 
>>>>> I'd say it sounds reasonable to include XDP in the normal traffic
>>>>> counters, but having the detailed XDP-specific counters is quite
>>>>> useful
>>>>> as well... So can't we do both (for all drivers)?
>>>>>
>>>
>>> What are you thinking ? 
>>> reporting XDP_DROP in interface dropped counter ?
>>> and XDP_TX/REDIRECT in the TX counter ?
>>> XDP_ABORTED in the  err/drop counter ?
>>>
>>> how about having a special XDP command in the .ndo_bpf that would query
>>> the standardized XDP stats ?
>> 
>> Don't have any strong opinions on the mechanism; just pointing out that
>> the XDP-specific stats are useful to have separately as well :)
>>
>
> I would like to see basic packets, bytes, and dropped counters tracked
> for Rx and Tx via the standard netdev counters for all devices. This is
> for ease in accounting as well as speed and simplicity for bumping
> counters for virtual devices from bpf helpers.
>
> From there, the XDP ones can be in the driver private stats as they are
> currently but with some consistency across drivers for redirects, drops,
> any thing else.
>
> So not a radical departure from where we are today, just getting the
> agreement for consistency and driver owners to make the changes.

Sounds good to me :)

-Toke

Re: consistency for statistics with XDP mode

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Thu, 22 Nov 2018 09:51:27 -0700

> I would like to see basic packets, bytes, and dropped counters tracked
> for Rx and Tx via the standard netdev counters for all devices. This is
> for ease in accounting as well as speed and simplicity for bumping
> counters for virtual devices from bpf helpers.
> 
> From there, the XDP ones can be in the driver private stats as they are
> currently but with some consistency across drivers for redirects, drops,
> any thing else.

I would go so far as to say we should provide generic infrastructure
for this, in the format of a template of statistic name strings, a
templace structure to hold the counters, etc.

Re: consistency for statistics with XDP mode

[Toshiaki Makita]

On 2018/11/23 1:43, David Ahern wrote:
> On 11/21/18 5:53 PM, Toshiaki Makita wrote:
>>> We really need consistency in the counters and at a minimum, users
>>> should be able to track packet and byte counters for both Rx and Tx
>>> including XDP.
>>>
>>> It seems to me the Rx and Tx packet, byte and dropped counters returned
>>> for the standard device stats (/proc/net/dev, ip -s li show, ...) should
>>> include all packets managed by the driver regardless of whether they are
>>> forwarded / dropped in XDP or go up the Linux stack. This also aligns
>>
>> Agreed. When I introduced virtio_net XDP counters, I just forgot to
>> update tx packets/bytes counters on ndo_xdp_xmit. Probably I thought it
>> is handled by free_old_xmit_skbs.
> 
> Do you have some time to look at adding the Tx counters to virtio_net?

hoping I can make some time within a couple of days.

-- 
Toshiaki Makita

Re: consistency for statistics with XDP mode

[Jakub Kicinski]

On Wed, 21 Nov 2018 14:06:49 -0700, David Ahern wrote:
> Keeping the basic xdp packets in the standard counters allows Paweł, for
> example, to continue to monitor /proc/net/dev.
> 
> Can we get agreement on this? And from there, get updates to the mlx5
> and virtio drivers?

I have a long standing itch to add more detailed but standardized
netlink stats, including per queue statistics.  Per queue stats should
cover XDP naturally.  I will try to sketch it out send an RFC later this
week FWIW.

Re: consistency for statistics with XDP mode

[Toshiaki Makita]

On 2018/11/26 10:37, Toshiaki Makita wrote:
> On 2018/11/23 1:43, David Ahern wrote:
>> On 11/21/18 5:53 PM, Toshiaki Makita wrote:
>>>> We really need consistency in the counters and at a minimum, users
>>>> should be able to track packet and byte counters for both Rx and Tx
>>>> including XDP.
>>>>
>>>> It seems to me the Rx and Tx packet, byte and dropped counters returned
>>>> for the standard device stats (/proc/net/dev, ip -s li show, ...) should
>>>> include all packets managed by the driver regardless of whether they are
>>>> forwarded / dropped in XDP or go up the Linux stack. This also aligns
>>>
>>> Agreed. When I introduced virtio_net XDP counters, I just forgot to
>>> update tx packets/bytes counters on ndo_xdp_xmit. Probably I thought it
>>> is handled by free_old_xmit_skbs.
>>
>> Do you have some time to look at adding the Tx counters to virtio_net?
> 
> hoping I can make some time within a couple of days.

Hmm... It looks like free_old_xmit_skbs() calls dev_consume_skb_any()
for xdp_frame when napi_tx is enabled. I will fix this beforehand.

-- 
Toshiaki Makita

Re: consistency for statistics with XDP mode

[Jason Wang]

On 2018/11/27 下午3:04, Toshiaki Makita wrote:
> On 2018/11/26 10:37, Toshiaki Makita wrote:
>> On 2018/11/23 1:43, David Ahern wrote:
>>> On 11/21/18 5:53 PM, Toshiaki Makita wrote:
>>>>> We really need consistency in the counters and at a minimum, users
>>>>> should be able to track packet and byte counters for both Rx and Tx
>>>>> including XDP.
>>>>>
>>>>> It seems to me the Rx and Tx packet, byte and dropped counters returned
>>>>> for the standard device stats (/proc/net/dev, ip -s li show, ...) should
>>>>> include all packets managed by the driver regardless of whether they are
>>>>> forwarded / dropped in XDP or go up the Linux stack. This also aligns
>>>> Agreed. When I introduced virtio_net XDP counters, I just forgot to
>>>> update tx packets/bytes counters on ndo_xdp_xmit. Probably I thought it
>>>> is handled by free_old_xmit_skbs.
>>> Do you have some time to look at adding the Tx counters to virtio_net?
>> hoping I can make some time within a couple of days.
> Hmm... It looks like free_old_xmit_skbs() calls dev_consume_skb_any()
> for xdp_frame when napi_tx is enabled. I will fix this beforehand.


Good catch. But the fix may require some thought. E.g one idea is to not 
call free_old_xmit_skbs() for XDP TX ring?

Thanks

Re: consistency for statistics with XDP mode

[Toshiaki Makita]

On 2018/11/28 13:03, Jason Wang wrote:
> On 2018/11/27 下午3:04, Toshiaki Makita wrote:
>> On 2018/11/26 10:37, Toshiaki Makita wrote:
>>> On 2018/11/23 1:43, David Ahern wrote:
>>>> On 11/21/18 5:53 PM, Toshiaki Makita wrote:
>>>>>> We really need consistency in the counters and at a minimum, users
>>>>>> should be able to track packet and byte counters for both Rx and Tx
>>>>>> including XDP.
>>>>>>
>>>>>> It seems to me the Rx and Tx packet, byte and dropped counters
>>>>>> returned
>>>>>> for the standard device stats (/proc/net/dev, ip -s li show, ...)
>>>>>> should
>>>>>> include all packets managed by the driver regardless of whether
>>>>>> they are
>>>>>> forwarded / dropped in XDP or go up the Linux stack. This also aligns
>>>>> Agreed. When I introduced virtio_net XDP counters, I just forgot to
>>>>> update tx packets/bytes counters on ndo_xdp_xmit. Probably I
>>>>> thought it
>>>>> is handled by free_old_xmit_skbs.
>>>> Do you have some time to look at adding the Tx counters to virtio_net?
>>> hoping I can make some time within a couple of days.
>> Hmm... It looks like free_old_xmit_skbs() calls dev_consume_skb_any()
>> for xdp_frame when napi_tx is enabled. I will fix this beforehand.
> 
> 
> Good catch. But the fix may require some thought. E.g one idea is to not
> call free_old_xmit_skbs() for XDP TX ring?

Yes, that's what I'm planning to do.

-- 
Toshiaki Makita

Re: consistency for statistics with XDP mode

[Saeed Mahameed]

On Thu, 2018-11-22 at 18:00 +0100, Toke Høiland-Jørgensen wrote:
> David Ahern <dsahern@gmail.com> writes:
> 
> > On 11/22/18 1:26 AM, Toke Høiland-Jørgensen wrote:
> > > Saeed Mahameed <saeedm@mellanox.com> writes:
> > > 
> > > > > > I'd say it sounds reasonable to include XDP in the normal
> > > > > > traffic
> > > > > > counters, but having the detailed XDP-specific counters is
> > > > > > quite
> > > > > > useful
> > > > > > as well... So can't we do both (for all drivers)?
> > > > > > 
> > > > 
> > > > What are you thinking ? 
> > > > reporting XDP_DROP in interface dropped counter ?
> > > > and XDP_TX/REDIRECT in the TX counter ?
> > > > XDP_ABORTED in the  err/drop counter ?
> > > > 
> > > > how about having a special XDP command in the .ndo_bpf that
> > > > would query
> > > > the standardized XDP stats ?
> > > the XDP-specific stats are useful to have separately as well :)
> > > 
> > 
> > I would like to see basic packets, bytes, and dropped counters
> > tracked
> > for Rx and Tx via the standard netdev counters for all devices. 

The problem of reporting XDP_DROP in the netedev drop counter is that
they don't fit this counter description : "no space in linux buffers"
and it will be hard for the user to determine whether these drops are
coming from XDP or because no buffer is available, which will make it
impossible to estimate packet rate performance without looking at
ethtool stats.
And reporting XDP_DROP in the netdev rx packets counter is somehow
misleading.. since those packets never made it out of this driver.. 


And reporting XDP_DROP in the netdev rx packets counter is somehow
misleading.. since those packets never made it out of this driver..
> > for ease in accounting as well as speed and simplicity for bumping
> > counters for virtual devices from bpf helpers.
> > 
> > From there, the XDP ones can be in the driver private stats as they
> > are
> > currently but with some consistency across drivers for redirects,
> > drops,
> > any thing else.
> > 
> > So not a radical departure from where we are today, just getting
> > the
> > agreement for consistency and driver owners to make the changes.
> 
> Sounds good to me :)
> 
> -Toke

Re: consistency for statistics with XDP mode

[Michael S. Tsirkin]

On Fri, Nov 30, 2018 at 08:10:58PM +0000, Saeed Mahameed wrote:
> On Thu, 2018-11-22 at 18:00 +0100, Toke Høiland-Jørgensen wrote:
> > David Ahern <dsahern@gmail.com> writes:
> > 
> > > On 11/22/18 1:26 AM, Toke Høiland-Jørgensen wrote:
> > > > Saeed Mahameed <saeedm@mellanox.com> writes:
> > > > 
> > > > > > > I'd say it sounds reasonable to include XDP in the normal
> > > > > > > traffic
> > > > > > > counters, but having the detailed XDP-specific counters is
> > > > > > > quite
> > > > > > > useful
> > > > > > > as well... So can't we do both (for all drivers)?
> > > > > > > 
> > > > > 
> > > > > What are you thinking ? 
> > > > > reporting XDP_DROP in interface dropped counter ?
> > > > > and XDP_TX/REDIRECT in the TX counter ?
> > > > > XDP_ABORTED in the  err/drop counter ?
> > > > > 
> > > > > how about having a special XDP command in the .ndo_bpf that
> > > > > would query
> > > > > the standardized XDP stats ?
> > > > the XDP-specific stats are useful to have separately as well :)
> > > > 
> > > 
> > > I would like to see basic packets, bytes, and dropped counters
> > > tracked
> > > for Rx and Tx via the standard netdev counters for all devices. 
> 
> The problem of reporting XDP_DROP in the netedev drop counter is that
> they don't fit this counter description : "no space in linux buffers"
> and it will be hard for the user to determine whether these drops are
> coming from XDP or because no buffer is available, which will make it
> impossible to estimate packet rate performance without looking at
> ethtool stats.
> And reporting XDP_DROP in the netdev rx packets counter is somehow
> misleading.. since those packets never made it out of this driver.. 
> 
> 
> And reporting XDP_DROP in the netdev rx packets counter is somehow
> misleading.. since those packets never made it out of this driver..

I think I agree. XDP needs minimal overhead - if user wants to do
counters then user can via maps. And in a sense XDP dropping packet
is much like e.g. TCP dropping packet - it is not counted
against the driver since it's not driver's fault.


> > > for ease in accounting as well as speed and simplicity for bumping
> > > counters for virtual devices from bpf helpers.
> > > 
> > > From there, the XDP ones can be in the driver private stats as they
> > > are
> > > currently but with some consistency across drivers for redirects,
> > > drops,
> > > any thing else.
> > > 
> > > So not a radical departure from where we are today, just getting
> > > the
> > > agreement for consistency and driver owners to make the changes.
> > 
> > Sounds good to me :)
> > 
> > -Toke

Re: consistency for statistics with XDP mode

[David Ahern]

On 11/30/18 1:30 PM, Michael S. Tsirkin wrote:
>>>> I would like to see basic packets, bytes, and dropped counters
>>>> tracked
>>>> for Rx and Tx via the standard netdev counters for all devices. 
>>
>> The problem of reporting XDP_DROP in the netedev drop counter is that
>> they don't fit this counter description : "no space in linux buffers"
>> and it will be hard for the user to determine whether these drops are
>> coming from XDP or because no buffer is available, which will make it
>> impossible to estimate packet rate performance without looking at
>> ethtool stats.
>> And reporting XDP_DROP in the netdev rx packets counter is somehow
>> misleading.. since those packets never made it out of this driver.. 
>>
>>
>> And reporting XDP_DROP in the netdev rx packets counter is somehow
>> misleading.. since those packets never made it out of this driver..
> 
> I think I agree. XDP needs minimal overhead - if user wants to do
> counters then user can via maps. And in a sense XDP dropping packet
> is much like e.g. TCP dropping packet - it is not counted
> against the driver since it's not driver's fault.
> 

XDP dropping a packet is completely different.

stats are important. packets disappearing with no counters -- standard
counters visible by standard tools -- is a user nightmare. If the
agreement is for XDP drops to be in driver level (e.g., xdp_drop) that
is fine since it is still retrievable by ethtool -S (existing APIs and
existing tools).

Re: consistency for statistics with XDP mode

[Saeed Mahameed]

On Fri, 2018-11-30 at 15:30 -0500, Michael S. Tsirkin wrote:
> On Fri, Nov 30, 2018 at 08:10:58PM +0000, Saeed Mahameed wrote:
> > On Thu, 2018-11-22 at 18:00 +0100, Toke Høiland-Jørgensen wrote:
> > > David Ahern <dsahern@gmail.com> writes:
> > > 
> > > > On 11/22/18 1:26 AM, Toke Høiland-Jørgensen wrote:
> > > > > Saeed Mahameed <saeedm@mellanox.com> writes:
> > > > > 
> > > > > > > > I'd say it sounds reasonable to include XDP in the
> > > > > > > > normal
> > > > > > > > traffic
> > > > > > > > counters, but having the detailed XDP-specific counters
> > > > > > > > is
> > > > > > > > quite
> > > > > > > > useful
> > > > > > > > as well... So can't we do both (for all drivers)?
> > > > > > > > 
> > > > > > 
> > > > > > What are you thinking ? 
> > > > > > reporting XDP_DROP in interface dropped counter ?
> > > > > > and XDP_TX/REDIRECT in the TX counter ?
> > > > > > XDP_ABORTED in the  err/drop counter ?
> > > > > > 
> > > > > > how about having a special XDP command in the .ndo_bpf that
> > > > > > would query
> > > > > > the standardized XDP stats ?
> > > > > the XDP-specific stats are useful to have separately as well
> > > > > :)
> > > > > 
> > > > 
> > > > I would like to see basic packets, bytes, and dropped counters
> > > > tracked
> > > > for Rx and Tx via the standard netdev counters for all
> > > > devices. 
> > 
> > The problem of reporting XDP_DROP in the netedev drop counter is
> > that
> > they don't fit this counter description : "no space in linux
> > buffers"
> > and it will be hard for the user to determine whether these drops
> > are
> > coming from XDP or because no buffer is available, which will make
> > it
> > impossible to estimate packet rate performance without looking at
> > ethtool stats.
> > And reporting XDP_DROP in the netdev rx packets counter is somehow
> > misleading.. since those packets never made it out of this
> > driver.. 
> > 
> > 
> > And reporting XDP_DROP in the netdev rx packets counter is somehow
> > misleading.. since those packets never made it out of this driver..
> 
> I think I agree. XDP needs minimal overhead - if user wants to do
> counters then user can via maps. And in a sense XDP dropping packet
> is much like e.g. TCP dropping packet - it is not counted
> against the driver since it's not driver's fault.

So we should count all XDP RX packets as successful rx packets i.e
netdev->stats.rx_packets++; regardless of the XDP program decision ? 

this implies that XDP_TX packets will be counted twice once in 
netdev->stats.rx_packets and once in netdev->stats.tx_packets

I think this is the only valid option if we are going to use standard
netdev stats for XDP use cases.

Re: consistency for statistics with XDP mode

[Jakub Kicinski]

On Fri, 30 Nov 2018 13:35:53 -0700, David Ahern wrote:
> On 11/30/18 1:30 PM, Michael S. Tsirkin wrote:
> >>>> I would like to see basic packets, bytes, and dropped counters
> >>>> tracked
> >>>> for Rx and Tx via the standard netdev counters for all devices.   
> >>
> >> The problem of reporting XDP_DROP in the netedev drop counter is that
> >> they don't fit this counter description : "no space in linux buffers"
> >> and it will be hard for the user to determine whether these drops are
> >> coming from XDP or because no buffer is available, which will make it
> >> impossible to estimate packet rate performance without looking at
> >> ethtool stats.
> >> And reporting XDP_DROP in the netdev rx packets counter is somehow
> >> misleading.. since those packets never made it out of this driver.. 
> >>
> >>
> >> And reporting XDP_DROP in the netdev rx packets counter is somehow
> >> misleading.. since those packets never made it out of this driver..  
> > 
> > I think I agree. XDP needs minimal overhead - if user wants to do
> > counters then user can via maps. And in a sense XDP dropping packet
> > is much like e.g. TCP dropping packet - it is not counted
> > against the driver since it's not driver's fault.
> >   
> 
> XDP dropping a packet is completely different.
> 
> stats are important. packets disappearing with no counters -- standard
> counters visible by standard tools -- is a user nightmare. If the
> agreement is for XDP drops to be in driver level (e.g., xdp_drop) that
> is fine since it is still retrievable by ethtool -S (existing APIs and
> existing tools).

I don't think that's completely fair.  Disappearing packets are a
nightmare, but if the user installed a program which silently drops
packets without incrementing any counter it's their own fault.  If
cls_bpf returns STOLEN or TRAP, I don't think that's gonna get counted
anywhere.

I don't think DPDK drivers maintain "just in case" statistics, either..

Re: consistency for statistics with XDP mode

[Jesper Dangaard Brouer]

On Fri, 30 Nov 2018 20:41:48 -0800
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:

> On Fri, 30 Nov 2018 13:35:53 -0700, David Ahern wrote:
> > On 11/30/18 1:30 PM, Michael S. Tsirkin wrote:  
> > >>>> I would like to see basic packets, bytes, and dropped counters
> > >>>> tracked
> > >>>> for Rx and Tx via the standard netdev counters for all devices.     
> > >>
> > >> The problem of reporting XDP_DROP in the netedev drop counter is that
> > >> they don't fit this counter description : "no space in linux buffers"
> > >> and it will be hard for the user to determine whether these drops are
> > >> coming from XDP or because no buffer is available, which will make it
> > >> impossible to estimate packet rate performance without looking at
> > >> ethtool stats.
> > >> And reporting XDP_DROP in the netdev rx packets counter is somehow
> > >> misleading.. since those packets never made it out of this driver.. 
> > >>
> > >>
> > >> And reporting XDP_DROP in the netdev rx packets counter is somehow
> > >> misleading.. since those packets never made it out of this driver..    
> > > 
> > > I think I agree. XDP needs minimal overhead - if user wants to do
> > > counters then user can via maps. And in a sense XDP dropping packet
> > > is much like e.g. TCP dropping packet - it is not counted
> > > against the driver since it's not driver's fault.
> > >     
> > 
> > XDP dropping a packet is completely different.
> > 
> > stats are important. packets disappearing with no counters -- standard
> > counters visible by standard tools -- is a user nightmare. If the
> > agreement is for XDP drops to be in driver level (e.g., xdp_drop) that
> > is fine since it is still retrievable by ethtool -S (existing APIs and
> > existing tools).  
> 
> I don't think that's completely fair.  Disappearing packets are a
> nightmare, but if the user installed a program which silently drops
> packets without incrementing any counter it's their own fault.  If
> cls_bpf returns STOLEN or TRAP, I don't think that's gonna get counted
> anywhere.

Yes, exactly. I think we are all agreeing.

RX packets in the driver NAPI-poll need to be counted as we have always
done in ifconfig stats.  These RX counters are incremented, regardless
of TC/netfilter drop the packet later.  Like wise XDP is a user
installed program, and a user XDP_DROP action/choice should not "hide"
this packet from the drivers RX-counter (as that IMHO could be more
confusing to the user).


> I don't think DPDK drivers maintain "just in case" statistics, either..

I agree. Maintaining counters for everything will cause performance
issues.  That said if we do choose to standardize XDP stats per action,
as most driver do maintain some ethtool counter anyhow, then we have to
make sure this doesn't hurt performance.  One simple trick is to bulk
update these counters at the end of drivers napi_pool function (like
most drivers do for the RX-counter stats).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: consistency for statistics with XDP mode

[Jesper Dangaard Brouer]

On Fri, 30 Nov 2018 23:54:10 +0000
Saeed Mahameed <saeedm@mellanox.com> wrote:

> On Fri, 2018-11-30 at 15:30 -0500, Michael S. Tsirkin wrote:
> > On Fri, Nov 30, 2018 at 08:10:58PM +0000, Saeed Mahameed wrote:  
> > > On Thu, 2018-11-22 at 18:00 +0100, Toke Høiland-Jørgensen wrote:  
> > > > David Ahern <dsahern@gmail.com> writes:
> > > >   
> > > > > On 11/22/18 1:26 AM, Toke Høiland-Jørgensen wrote:  
> > > > > > Saeed Mahameed <saeedm@mellanox.com> writes:
> > > > > >   
> > > > > > > > > I'd say it sounds reasonable to include XDP in the
> > > > > > > > > normal
> > > > > > > > > traffic
> > > > > > > > > counters, but having the detailed XDP-specific counters
> > > > > > > > > is
> > > > > > > > > quite
> > > > > > > > > useful
> > > > > > > > > as well... So can't we do both (for all drivers)?
> > > > > > > > >   
> > > > > > > 
> > > > > > > What are you thinking ? 
> > > > > > > reporting XDP_DROP in interface dropped counter ?
> > > > > > > and XDP_TX/REDIRECT in the TX counter ?
> > > > > > > XDP_ABORTED in the  err/drop counter ?
> > > > > > > 
> > > > > > > how about having a special XDP command in the .ndo_bpf that
> > > > > > > would query
> > > > > > > the standardized XDP stats ?  
> > > > > > the XDP-specific stats are useful to have separately as well
> > > > > > :)
> > > > > >   
> > > > > 
> > > > > I would like to see basic packets, bytes, and dropped counters
> > > > > tracked
> > > > > for Rx and Tx via the standard netdev counters for all
> > > > > devices.   
> > > 
> > > The problem of reporting XDP_DROP in the netedev drop counter is
> > > that
> > > they don't fit this counter description : "no space in linux
> > > buffers"
> > > and it will be hard for the user to determine whether these drops
> > > are
> > > coming from XDP or because no buffer is available, which will make
> > > it
> > > impossible to estimate packet rate performance without looking at
> > > ethtool stats.
> > > And reporting XDP_DROP in the netdev rx packets counter is somehow
> > > misleading.. since those packets never made it out of this
> > > driver.. 
> > > 
> > > 
> > > And reporting XDP_DROP in the netdev rx packets counter is somehow
> > > misleading.. since those packets never made it out of this driver..  
> > 
> > I think I agree. XDP needs minimal overhead - if user wants to do
> > counters then user can via maps. And in a sense XDP dropping packet
> > is much like e.g. TCP dropping packet - it is not counted
> > against the driver since it's not driver's fault.  
> 
> So we should count all XDP RX packets as successful rx packets i.e
> netdev->stats.rx_packets++; regardless of the XDP program decision ? 

Yes.

> this implies that XDP_TX packets will be counted twice once in 
> netdev->stats.rx_packets and once in netdev->stats.tx_packets

Yes, because the packet was RX'ed on the interface, and then TX'ed on
the interface.  Users expect to see these packets (ac)counted.

> I think this is the only valid option if we are going to use standard
> netdev stats for XDP use cases.

IMHO XDP_DROP should not be accounted as netdev stats drops, this is a
user installed program like tc/iptables, that can also choose to drop
packets.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: consistency for statistics with XDP mode

[David Ahern]

On 12/1/18 4:22 AM, Jesper Dangaard Brouer wrote:
>>
>> So we should count all XDP RX packets as successful rx packets i.e
>> netdev->stats.rx_packets++; regardless of the XDP program decision ? 
> 
> Yes.
> 
>> this implies that XDP_TX packets will be counted twice once in 
>> netdev->stats.rx_packets and once in netdev->stats.tx_packets
> 
> Yes, because the packet was RX'ed on the interface, and then TX'ed on
> the interface.  Users expect to see these packets (ac)counted.

+1

> 
>> I think this is the only valid option if we are going to use standard
>> netdev stats for XDP use cases.
> 
> IMHO XDP_DROP should not be accounted as netdev stats drops, this is a
> user installed program like tc/iptables, that can also choose to drop
> packets.
> 

sure and both tc and iptables have counters that can see the dropped
packets. A counter in the driver level stats ("xdp_drop" is fine with
with me).

Re: consistency for statistics with XDP mode

[David Ahern]

On 12/1/18 4:14 AM, Jesper Dangaard Brouer wrote:
>>> XDP dropping a packet is completely different.
>>>
>>> stats are important. packets disappearing with no counters -- standard
>>> counters visible by standard tools -- is a user nightmare. If the
>>> agreement is for XDP drops to be in driver level (e.g., xdp_drop) that
>>> is fine since it is still retrievable by ethtool -S (existing APIs and
>>> existing tools).  
>>
>> I don't think that's completely fair.  Disappearing packets are a
>> nightmare, but if the user installed a program which silently drops
>> packets without incrementing any counter it's their own fault.  If
>> cls_bpf returns STOLEN or TRAP, I don't think that's gonna get counted
>> anywhere.
> 
> Yes, exactly. I think we are all agreeing.

The person debugging a problem is most likely NOT the person who wrote
the bpf program or loaded it where ever. I have saying this for a while
now and it is happening: ebpf programs will be coming from multiple
sources and transparently loaded (transparent to the person using an OS
or a service). That is already happening today and Ubuntu 18.04 is a
good example of that. systemd installs 'firewall' programs on services,
and those programs can drop packets. 'ip vrf exec' is loading programs
on cgroups. This will spread, and it will become a nightmare to debug
why a service is not working as expected.

What am I asking for here with XDP is to give the poor TAC/customer
support person I fighting chance at isolating where a problem is
occurring which means standard counters accessible from standard tools.

> 
> RX packets in the driver NAPI-poll need to be counted as we have always
> done in ifconfig stats.  These RX counters are incremented, regardless
> of TC/netfilter drop the packet later.  Like wise XDP is a user
> installed program, and a user XDP_DROP action/choice should not "hide"
> this packet from the drivers RX-counter (as that IMHO could be more
> confusing to the user).
> 
> 
>> I don't think DPDK drivers maintain "just in case" statistics, either..
> 
> I agree. Maintaining counters for everything will cause performance
> issues.  That said if we do choose to standardize XDP stats per action,
> as most driver do maintain some ethtool counter anyhow, then we have to
> make sure this doesn't hurt performance.  One simple trick is to bulk
> update these counters at the end of drivers napi_pool function (like
> most drivers do for the RX-counter stats).
> 

Use of DPDK means everything is done with custom tools. There is no
comparison to what I am pushing here. I do not want Linux + XDP to
require custom anything to debug basic functionality - such as isolating
a packet drop to a specific point.

Re: consistency for statistics with XDP mode

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Mon, 3 Dec 2018 08:45:12 -0700

> On 12/1/18 4:22 AM, Jesper Dangaard Brouer wrote:
>> IMHO XDP_DROP should not be accounted as netdev stats drops, this is a
>> user installed program like tc/iptables, that can also choose to drop
>> packets.
> 
> sure and both tc and iptables have counters that can see the dropped
> packets. A counter in the driver level stats ("xdp_drop" is fine with
> with me).

Part of the problem I have with this kind of logic is we take the choice
away from the XDP program.

If I feel that the xdp_drop counter bump is too much overhead during a
DDoS attack and I want to avoid it, you don't give me a choice in the
matter.

If I want to represent the statistics for that event differently, you
also give me no choice about it.

Really, if XDP_DROP is returned, zero resources should be devoted to
the frame past that point.

I know you want to live in this magical world where XDP stuff behaves
like the existing stack and give you all of the visibility to events
and objects.

But that is your choice.

Please give others the choice to not live in that world and allow XDP
programs to live in their own entirely different environment, with
custom statistics and complete control over how counters are
incremented and how objects are used and represented, if they choose
to do so.

XDP is about choice.

Re: consistency for statistics with XDP mode

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Mon, 3 Dec 2018 08:56:28 -0700

> I do not want Linux + XDP to require custom anything to debug basic
> functionality - such as isolating a packet drop to a specific point.

Definitely we need to let people choose to arrange things that way
if they want to do so.  It is not our place to make to make that kind
of decision for them.

If they want custom statistics and a custom representation of objects
in their tools, etc.  That is completely fine.

XDP is not about forcing a model of these things upon people.

Re: consistency for statistics with XDP mode

[Arnaldo Carvalho de Melo]

Em Mon, Dec 03, 2018 at 11:30:01AM -0800, David Miller escreveu:
> From: David Ahern <dsahern@gmail.com>
> Date: Mon, 3 Dec 2018 08:45:12 -0700
> 
> > On 12/1/18 4:22 AM, Jesper Dangaard Brouer wrote:
> >> IMHO XDP_DROP should not be accounted as netdev stats drops, this is a
> >> user installed program like tc/iptables, that can also choose to drop
> >> packets.
> > 
> > sure and both tc and iptables have counters that can see the dropped
> > packets. A counter in the driver level stats ("xdp_drop" is fine with
> > with me).
> 
> Part of the problem I have with this kind of logic is we take the choice
> away from the XDP program.
> 
> If I feel that the xdp_drop counter bump is too much overhead during a
> DDoS attack and I want to avoid it, you don't give me a choice in the
> matter.
> 
> If I want to represent the statistics for that event differently, you
> also give me no choice about it.
> 
> Really, if XDP_DROP is returned, zero resources should be devoted to
> the frame past that point.
> 
> I know you want to live in this magical world where XDP stuff behaves
> like the existing stack and give you all of the visibility to events
> and objects.
> 
> But that is your choice.
> 
> Please give others the choice to not live in that world and allow XDP
> programs to live in their own entirely different environment, with
> custom statistics and complete control over how counters are
> incremented and how objects are used and represented, if they choose
> to do so.
> 
> XDP is about choice.

Coming out of the blue...: the presence of a "struct xdp_stats" in the
XDP program BPF object file .BTF section, one could query and the parse
to figure out what stats, if any, are provided.

/me goes back to tweaking his btf_loader in pahole... :-)

- Arnaldo

Re: consistency for statistics with XDP mode

[Toke Høiland-Jørgensen]

David Miller <davem@davemloft.net> writes:

> From: David Ahern <dsahern@gmail.com>
> Date: Mon, 3 Dec 2018 08:45:12 -0700
>
>> On 12/1/18 4:22 AM, Jesper Dangaard Brouer wrote:
>>> IMHO XDP_DROP should not be accounted as netdev stats drops, this is
>>> a user installed program like tc/iptables, that can also choose to
>>> drop packets.
>> 
>> sure and both tc and iptables have counters that can see the dropped
>> packets. A counter in the driver level stats ("xdp_drop" is fine with
>> with me).
>
> Part of the problem I have with this kind of logic is we take the
> choice away from the XDP program.

I wonder if it would be possible to support both the "give me user
normal stats" case and the "let me do whatever I want" case by a
combination of userspace tooling and maybe a helper or two?

I.e., create a "do_stats()" helper (please pick a better name), which
will either just increment the desired counters, or set a flag so the
driver can do it at napi poll exit. With this, the userspace tooling
could have a "--give-me-normal-stats" switch (or some other interface),
which would inject a call instruction to that helper at the start of the
program.

This would enable the normal counters in a relatively painless way,
while still letting people opt out if they don't want to pay the cost in
terms of overhead. And having the userspace tooling inject the helper
call helps support the case where the admin didn't write the XDP
programs being loaded.

Any reason why that wouldn't work?

-Toke

Re: consistency for statistics with XDP mode

[David Miller]

From: Toke Høiland-Jørgensen <toke@toke.dk>
Date: Mon, 03 Dec 2018 22:00:32 +0200

> I wonder if it would be possible to support both the "give me user
> normal stats" case and the "let me do whatever I want" case by a
> combination of userspace tooling and maybe a helper or two?
> 
> I.e., create a "do_stats()" helper (please pick a better name), which
> will either just increment the desired counters, or set a flag so the
> driver can do it at napi poll exit. With this, the userspace tooling
> could have a "--give-me-normal-stats" switch (or some other interface),
> which would inject a call instruction to that helper at the start of the
> program.
> 
> This would enable the normal counters in a relatively painless way,
> while still letting people opt out if they don't want to pay the cost in
> terms of overhead. And having the userspace tooling inject the helper
> call helps support the case where the admin didn't write the XDP
> programs being loaded.
> 
> Any reason why that wouldn't work?

I think this is a good idea, or even an attribute tag that gets added
to the XDP program that controls stats handling.

Re: consistency for statistics with XDP mode

[David Ahern]

On 12/3/18 5:00 PM, David Miller wrote:
> From: Toke Høiland-Jørgensen <toke@toke.dk>
> Date: Mon, 03 Dec 2018 22:00:32 +0200
> 
>> I wonder if it would be possible to support both the "give me user
>> normal stats" case and the "let me do whatever I want" case by a
>> combination of userspace tooling and maybe a helper or two?
>>
>> I.e., create a "do_stats()" helper (please pick a better name), which
>> will either just increment the desired counters, or set a flag so the
>> driver can do it at napi poll exit. With this, the userspace tooling
>> could have a "--give-me-normal-stats" switch (or some other interface),
>> which would inject a call instruction to that helper at the start of the
>> program.
>>
>> This would enable the normal counters in a relatively painless way,
>> while still letting people opt out if they don't want to pay the cost in
>> terms of overhead. And having the userspace tooling inject the helper
>> call helps support the case where the admin didn't write the XDP
>> programs being loaded.
>>
>> Any reason why that wouldn't work?
> 
> I think this is a good idea, or even an attribute tag that gets added
> to the XDP program that controls stats handling.
> 

My argument is that the ebpf program writer should *not* get that
choice; the admin of the box should. Program writers make mistakes. Box
admins / customer support are the ones that have to deal with those
mistakes. Program writers - especially for xdp - are going to be focused
on benchmarks; admins are focused on the big picture and should be given
the option of trading a small amount of performance for simpler management.

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?

Re: consistency for statistics with XDP mode

[David Miller]

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?

I'm all for choice so how can I object? :)

Re: consistency for statistics with XDP mode

[Toke Høiland-Jørgensen]

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?

-Toke

Re: consistency for statistics with XDP mode

[Jakub Kicinski]

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.

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..

Re: consistency for statistics with XDP mode

[Jesper Dangaard Brouer]

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]++;
+       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

Re: consistency for statistics with XDP mode

[Jakub Kicinski]

On Tue, 4 Dec 2018 10:29:04 +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]++;
> +       return action;
>  }

Now if we can turn it into a trampoline dynamically inserted on program
invocation that would be perfect :)

>  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.

I see, I was thinking xdp_frame work made it generally less of an issue
but I haven't double checked.  Perhaps having first cache line as read
only is a good rule to stick to regardless.

Re: consistency for statistics with XDP mode

[Michael S. Tsirkin]

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