Message-ID: <3b5ffccc-f1cd-b2f8-1e78-72997b43c12c@gmail.com>
Date:   Mon, 20 Mar 2023 11:06:49 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH net] net: dsa: report rx_bytes unadjusted for ETH_HLEN
Content-Language: en-US
To:     Vladimir Oltean <vladimir.oltean@nxp.com>, netdev@vger.kernel.org
Cc:     Andrew Lunn <andrew@lunn.ch>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org
References: <20230317231900.3944446-1-vladimir.oltean@nxp.com>
From:   Florian Fainelli <f.fainelli@gmail.com>
In-Reply-To: <20230317231900.3944446-1-vladimir.oltean@nxp.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/17/23 16:19, Vladimir Oltean wrote:
> We collect the software statistics counters for RX bytes (reported to
> /proc/net/dev and to ethtool -S $dev | grep 'rx_bytes: ") at a time when
> skb->len has already been adjusted by the eth_type_trans() ->
> skb_pull_inline(skb, ETH_HLEN) call to exclude the L2 header.
> 
> This means that when connecting 2 DSA interfaces back to back and
> sending 1 packet with length 100, the sending interface will report
> tx_bytes as incrementing by 100, and the receiving interface will report
> rx_bytes as incrementing by 86.
> 
> Since accounting for that in scripts is quirky and is something that
> would be DSA-specific behavior (requiring users to know that they are
> running on a DSA interface in the first place), the proposal is that we
> treat it as a bug and fix it.
> 
> This design bug has always existed in DSA, according to my analysis:
> commit 91da11f870f0 ("net: Distributed Switch Architecture protocol
> support") also updates skb->dev->stats.rx_bytes += skb->len after the
> eth_type_trans() call. Technically, prior to Florian's commit
> a86d8becc3f0 ("net: dsa: Factor bottom tag receive functions"), each and
> every vendor-specific tagging protocol driver open-coded the same bug,
> until the buggy code was consolidated into something resembling what can
> be seen now. So each and every driver should have its own Fixes: tag,
> because of their different histories until the convergence point.
> I'm not going to do that, for the sake of simplicity, but just blame the
> oldest appearance of buggy code.
> 
> There are 2 ways to fix the problem. One is the obvious way, and the
> other is how I ended up doing it. Obvious would have been to move
> dev_sw_netstats_rx_add() one line above eth_type_trans(), and below
> skb_push(skb, ETH_HLEN). But DSA processing is not as simple as that.
> We count the bytes after removing everything DSA-related from the
> packet, to emulate what the packet's length was, on the wire, when the
> user port received it.
> 
> When eth_type_trans() executes, dsa_untag_bridge_pvid() has not run yet,
> so in case the switch driver requests this behavior - commit
> 412a1526d067 ("net: dsa: untag the bridge pvid from rx skbs") has the
> details - the obvious variant of the fix wouldn't have worked, because
> the positioning there would have also counted the not-yet-stripped VLAN
> header length, something which is absent from the packet as seen on the
> wire (there it may be untagged, whereas software will see it as
> PVID-tagged).
> 
> Fixes: f613ed665bb3 ("net: dsa: Add support for 64-bit statistics")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Already applied, but I agree with you that this is the simplest way to 
go about fixign this. Thanks!
-- 
Florian