netdev - Re: [PATCH v8 bpf-next 00/10] veth: Driver XDP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 3 Aug 2018 20:14:20 +0900
From:   Toshiaki Makita <toshiaki.makita1@...il.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     Toshiaki Makita <makita.toshiaki@....ntt.co.jp>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        John Fastabend <john.fastabend@...il.com>,
        Tariq Toukan <tariqt@...lanox.com>,
        Björn Töpel <bjorn.topel@...el.com>
Subject: Re: [PATCH v8 bpf-next 00/10] veth: Driver XDP

On 18/08/03 (金) 18:45, Jesper Dangaard Brouer wrote:
> On Fri,  3 Aug 2018 16:58:08 +0900
> Toshiaki Makita <makita.toshiaki@....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 <brouer@...hat.com>

Thank you for your thorough review!

Toshiaki Makita