Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Feb 2022 20:59:16 -0800
From:   Jakub Kicinski <kuba@kernel.org>
To:     Jeffrey Ji <jeffreyjilinux@gmail.com>
Cc:     Eric Dumazet <edumazet@google.com>,
        "David S . Miller" <davem@davemloft.net>,
        Brian Vazquez <brianvv@google.com>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        jeffreyji <jeffreyji@google.com>
Subject: Re: [PATCH v6 net-next] net-core: add InMacErrors counter
Message-ID: <20220202205916.58f4a592@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
In-Reply-To: <20220201222845.3640041-1-jeffreyji@google.com>
References: <20220201222845.3640041-1-jeffreyji@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue,  1 Feb 2022 22:28:45 +0000 Jeffrey Ji wrote:
> From: jeffreyji <jeffreyji@google.com>
> 
> Increment InMacErrors counter when packet dropped due to incorrect dest
> MAC addr.
> 
> An example when this drop can occur is when manually crafting raw
> packets that will be consumed by a user space application via a tap
> device. For testing purposes local traffic was generated using trafgen
> for the client and netcat to start a server
> 
> example output from nstat:
> \~# nstat -a | grep InMac
> Ip6InMacErrors                  0                  0.0
> IpExtInMacErrors                1                  0.0

I had another thing and this still doesn't sit completely well 
with me :(

Shouldn't we count those drops as skb->dev->rx_dropped?
Commonly NICs will do such filtering and if I got it right
in struct rtnl_link_stats64 kdoc - report them as rx_dropped.
It'd be inconsistent if on a physical interface we count
these as rx_dropped and on SW interface (or with promisc enabled 
etc.) in the SNMP counters. 
Or we can add a new link stat that NICs can use as well.

In fact I'm not sure this is really a IP AKA L3 statistic,
it's the L2 address that doesn't match.


If everyone disagrees - should we at least rename the MIB counter
similarly to the drop reason? Experience shows that users call for 
help when they see counters with Error in their name, I'd vote for
IpExtInDropOtherhost or some such. The statistic should also be
documented in Documentation/networking/snmp_counter.rst