On 18/08/03 (金) 18:45, Jesper Dangaard Brouer wrote:
On Fri, 3 Aug 2018 16:58:08 +0900
Toshiaki Makita <makita.toshi...@lab.ntt.co.jp> wrote:
This patch set introduces driver XDP for veth.
Basically this is used in conjunction with redirect action of another XDP
program.
NIC -----------> veth===veth
(XDP) (redirect) (XDP)
I'm was playing with V7 on my testlab yesterday and I noticed one
fundamental issue. You are not updating the "ifconfig" stats counters,
when in XDP mode. This makes receive or send via XDP invisible to
sysadm/management tools. This for-sure is going to cause confusion...
Yes, I did not update stats on ndo_xdp_xmit. My intention was that I'm
going to make another patch set to make stats nice after this, but did
not state that in the cover letter. Sorry about that.
I took a closer look at other driver. The ixgbe driver is doing the
right thing. Driver i40e have a bug, where RX/TX stats are swapped
getting (strange!). The mlx5 driver is not updating the regular RX/TX
counters, but A LOT of other ethtool stats counters (which are the ones
I usually monitor when testing).
So, given other drivers also didn't get this right, we need to have a
discussion outside your/this patchset. Thus, I don't want to
stop/stall this patchset, but this is something we need to fixup in a
followup patchset to other drivers as well.
One of the reason why I did not include the stats patches in this series
is that as you say basically stats in many drivers do not look correct
and I thought the correctness is not strictly required for now.
In fact I recently fixed virtio_net stats which only updated packets
counter but not bytes counter on XDP_DROP.
Another reason is that it will hurt the performance without more
aggressive stats structure change. Drop counter is currently atomic so
it would cause heavy cache contention on multiqueue env. The plan is to
make this per-cpu or per-queue first. Also I want to introduce per-queue
stats for ethtool, so the change would be relatively big and probably
not fit in this series all together.
Thus, I'm acking the patchset, but I request that we do a joint effort
of fixing this as followup patches.
Sure, at least for veth I'm going to make a followup patches.
Acked-by: Jesper Dangaard Brouer <bro...@redhat.com>
Thank you for your thorough review!
Toshiaki Makita
