Support IFCOUNTER_OBYTES and IFCOUNTER_OMCASTS even with VTNET_LEGACY_TX defined

[joyul_juniper.net]

Summary

vtxs_obytes and vtxs_omcasts are updated in vtnet_txq_eof, which is not bound with VTNET_LEGACY_TX. Thus, both should be supported regardless of whether VTNET_LEGACY_TX is defined or not.

Test Plan

Use if_get_counter API with IFCOUNTER_OBYTES and make sure output byte counts are updated.

[glebius]

Looks like driver does "hardware" accounting and software accounting at the same time. That's why we see correct counts without your patches. If vtnet_accum_stats() provides us with all what we need, then the driver should declare IFCAP_HWSTATS in its capabilities. That will turn off software accounting by the network stack. May I ask you to add this and also combine D44816 and D44817 into a single review?

[zlei]

sys/dev/virtio/network/if_vtnet.c

3015

I believe the condition is intentional.

VTNET_LEGACY_TX is defined when ALTQ is enabled [1]. ALTQ will do statistics by itself [2].

So this removal will end up double accounting of IFCOUNTER_OBYTES and IFCOUNTER_OMCASTS ( when ALTQ is enabled ).

1. https://cgit.freebsd.org/src/tree/sys/dev/virtio/network/if_vtnetvar.h#n33
2. https://cgit.freebsd.org/src/tree/sys/net/ifq.h#n251

[zlei]

In D44817#1021811, @glebius wrote:

Looks like driver does "hardware" accounting and software accounting at the same time. That's why we see correct counts without your patches. If vtnet_accum_stats() provides us with all what we need, then the driver should declare IFCAP_HWSTATS in its capabilities. That will turn off software accounting by the network stack. May I ask you to add this and also combine D44816 and D44817 into a single review?

In D44817#1021811, @glebius wrote:

Looks like driver does "hardware" accounting and software accounting at the same time. That's why we see correct counts without your patches. If vtnet_accum_stats() provides us with all what we need, then the driver should declare IFCAP_HWSTATS in its capabilities. That will turn off software accounting by the network stack. May I ask you to add this and also combine D44816 and D44817 into a single review?

sys/dev/virtio/network/if_vtnet.c

3015

I believe the condition is intentional.

VTNET_LEGACY_TX is defined when ALTQ is enabled [1]. ALTQ will do statistics by itself [2].

So this removal will end up double accounting of IFCOUNTER_OBYTES and IFCOUNTER_OMCASTS ( when ALTQ is enabled ).

I must be drunk. That should be harmless regardless ALTQ. vtnet_accum_stats() accounts hardware statistics only. No double accounting indeed.

1. https://cgit.freebsd.org/src/tree/sys/dev/virtio/network/if_vtnetvar.h#n33
2. https://cgit.freebsd.org/src/tree/sys/net/ifq.h#n251

@glebius
Currently IFCAP_HWSTATS is only for inbound packets. Shall we extend it also for outbound packets ?

[zlei]

I'd like to commit this before branching stable/15. @glebius Any thoughs ?

[glebius]

Currently IFCAP_HWSTATS is only for inbound packets. Shall we extend it also for outbound packets ?

This is historical misfeature of the BSD stack. In the software mode drivers are expected to increment the outbound counters themselves, while inbound is incremented in ether_input_internal().

I'll appreciate if you handle this submission before stable/15. Please address the IFCAP_HWSTATS, too. Thanks!