[PATCH] [net] add rx_otherhost_dropped sysfs entry

[PATCH] [net] add rx_otherhost_dropped sysfs entry

[nick black]

Add the sysfs export for rx_otherhost_dropped, added
in 794c24e9921f ("rx_otherhost_dropped to core_stats").
All other rtnl_link_stats64 entries are already present
as sysfs nodes; this completes the set.

Signed-off-by: nick black <dankamongmen@gmail.com>
---
 Documentation/ABI/testing/sysfs-class-net-statistics | 8 ++++++++
 net/core/net-sysfs.c                                 | 1 +
 2 files changed, 9 insertions(+)

diff --git Documentation/ABI/testing/sysfs-class-net-statistics Documentation/ABI/testing/sysfs-class-net-statistics
index 55db27815361..97ac76af30a4 100644
--- Documentation/ABI/testing/sysfs-class-net-statistics
+++ Documentation/ABI/testing/sysfs-class-net-statistics
@@ -104,6 +104,14 @@ Description:
 		Indicates the number of received packets that were dropped on
 		an inactive device by the network core.
 
+What:		/sys/class/<iface>/statistics/rx_otherhost_dropped
+Date:		February 2023
+KernelVersion:	6.3
+Contact:	netdev@vger.kernel.org
+Description:
+		Indicates the number of received packets that were dropped due
+		to mismatch in destination MAC address.
+
 What:		/sys/class/<iface>/statistics/rx_over_errors
 Date:		April 2005
 KernelVersion:	2.6.12
diff --git net/core/net-sysfs.c net/core/net-sysfs.c
index 15e3f4606b5f..fe8012308696 100644
--- net/core/net-sysfs.c
+++ net/core/net-sysfs.c
@@ -713,6 +713,7 @@ NETSTAT_ENTRY(tx_window_errors);
 NETSTAT_ENTRY(rx_compressed);
 NETSTAT_ENTRY(tx_compressed);
 NETSTAT_ENTRY(rx_nohandler);
+NETSTAT_ENTRY(rx_otherhost_dropped);
 
 static struct attribute *netstat_attrs[] __ro_after_init = {
 	&dev_attr_rx_packets.attr,
-- 
2.39.2

Re: [PATCH] [net] add rx_otherhost_dropped sysfs entry

[Jakub Kicinski]

On Sat, 25 Feb 2023 16:12:16 -0500 nick black wrote:
> Add the sysfs export for rx_otherhost_dropped, added
> in 794c24e9921f ("rx_otherhost_dropped to core_stats").
> All other rtnl_link_stats64 entries are already present
> as sysfs nodes; this completes the set.

"All the other stats are there" is not a strong enough reason
to waste memory on all systems. You need to justify the change
based on how important the counter is. I'd prefer to draw a
line on adding the sysfs stats entries. We don't want to have 
to invent a new stats struct just to avoid having sysfs entries
for each stat.

Re: [PATCH] [net] add rx_otherhost_dropped sysfs entry

[nick black]

Jakub Kicinski left as an exercise for the reader:
> "All the other stats are there" is not a strong enough reason
> to waste memory on all systems. You need to justify the change
> based on how important the counter is. I'd prefer to draw a
> line on adding the sysfs stats entries. We don't want to have 
> to invent a new stats struct just to avoid having sysfs entries
> for each stat.

In that case, I think a comment here is warranted explaining why
this stat, out of 24 total, isn't important enough to reproduce
in sysfs. I'm not sure what this comment would be:
rx_otherhost_dropped certainly seems as useful as, say
rx_compressed (only valid on e.g. CSLIP and PPP).

If this stat is left out of the sysfs interface, I'm likely to
just grab the rtnl_link_stats64 directly via netlink, and forgo
the sysfs interface entirely. If, in a modern switched world,
I'm receiving many packets destined for other hosts, that's at
least as interesting to me as several other classes of RX error.

-- 
nick black -=- https://www.nick-black.com
to make an apple pie from scratch,
you need first invent a universe.

Re: [PATCH] [net] add rx_otherhost_dropped sysfs entry

[Eric Dumazet]

On Mon, Feb 27, 2023 at 7:29 PM nick black <dankamongmen@gmail.com> wrote:
>
> Jakub Kicinski left as an exercise for the reader:
> > "All the other stats are there" is not a strong enough reason
> > to waste memory on all systems. You need to justify the change
> > based on how important the counter is. I'd prefer to draw a
> > line on adding the sysfs stats entries. We don't want to have
> > to invent a new stats struct just to avoid having sysfs entries
> > for each stat.
>
> In that case, I think a comment here is warranted explaining why
> this stat, out of 24 total, isn't important enough to reproduce
> in sysfs. I'm not sure what this comment would be:
> rx_otherhost_dropped certainly seems as useful as, say
> rx_compressed (only valid on e.g. CSLIP and PPP).
>
> If this stat is left out of the sysfs interface, I'm likely to
> just grab the rtnl_link_stats64 directly via netlink, and forgo
> the sysfs interface entirely. If, in a modern switched world,
> I'm receiving many packets destined for other hosts, that's at
> least as interesting to me as several other classes of RX error.

We do not want to add more sysfs files, unless absolutely needed.

This sysfs thing adds costs at every interface creation and dismantle,
even if the sysfs file is never to be used.

netlink is the primary interface, sysfs is legacy from old days.
netlink code is basically free if code is not run.

So please, forget about sysfs, and switch to netlink :)

Re: [PATCH] [net] add rx_otherhost_dropped sysfs entry

[Jakub Kicinski]

On Mon, 27 Feb 2023 13:29:54 -0500 nick black wrote:
> In that case, I think a comment here is warranted explaining why
> this stat, out of 24 total, isn't important enough to reproduce
> in sysfs. I'm not sure what this comment would be:
> rx_otherhost_dropped certainly seems as useful as, say
> rx_compressed (only valid on e.g. CSLIP and PPP).

How about a banner before rx_otherhost_dropped? Maybe:

	/* end of old stats -- new stats via rtnetlink only */

> If this stat is left out of the sysfs interface, I'm likely to
> just grab the rtnl_link_stats64 directly via netlink, and forgo
> the sysfs interface entirely. If, in a modern switched world,
> I'm receiving many packets destined for other hosts, that's at
> least as interesting to me as several other classes of RX error.

Right, I wish we could just remove the old entries since no driver 
from the last decade will fill those in, anyway.

Re: [PATCH] [net] add rx_otherhost_dropped sysfs entry

[nick black]

Jakub Kicinski left as an exercise for the reader:
> How about a banner before rx_otherhost_dropped? Maybe:
> 
> 	/* end of old stats -- new stats via rtnetlink only */

sounds good to me; I'll also annotate the sysfs documentation to
emphasize that netlink is recommended, and that new stats will
not be reflected there. from the perspective of an outsider like
myself, it just looks like this stat was missed.

i'll have a new patch for you shortly. thank you for your review!

-- 
nick black -=- https://www.nick-black.com
to make an apple pie from scratch,
you need first invent a universe.

Re: [PATCH] [net] add rx_otherhost_dropped sysfs entry

[nick black]

We do not want to export any further stats via sysfs.
Applications ought be using netlink. Note this at the end
of the NETSTAT_ENTRIES.
---
 net/core/net-sysfs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git net/core/net-sysfs.c net/core/net-sysfs.c
index 15e3f4606b5f..c00a0f332a22 100644
--- net/core/net-sysfs.c
+++ net/core/net-sysfs.c
@@ -714,6 +714,10 @@ NETSTAT_ENTRY(rx_compressed);
 NETSTAT_ENTRY(tx_compressed);
 NETSTAT_ENTRY(rx_nohandler);
 
+/* end of old stats -- new stats via rtnetlink only. we do not want
+ * more sysfs entries.
+ */
+
 static struct attribute *netstat_attrs[] __ro_after_init = {
 	&dev_attr_rx_packets.attr,
 	&dev_attr_tx_packets.attr,
-- 
2.39.2

Re: [PATCH] [net] add rx_otherhost_dropped sysfs entry

[Jakub Kicinski]

On Tue, 28 Feb 2023 08:08:59 -0500 nick black wrote:
> --- net/core/net-sysfs.c
> +++ net/core/net-sysfs.c
> @@ -714,6 +714,10 @@ NETSTAT_ENTRY(rx_compressed);
>  NETSTAT_ENTRY(tx_compressed);
>  NETSTAT_ENTRY(rx_nohandler);
>  

I'd skip the empty line here to make it clear that the comment pertains
to what's above it, otherwise people may miss it.

When reposting pls make a new thread (patchwork won't pick up patches
sent with Re: at all:
https://patchwork.kernel.org/project/netdevbpf/patch/Y/386wA5az1Yixyp@schwarzgerat.orthanc/

> +/* end of old stats -- new stats via rtnetlink only. we do not want
> + * more sysfs entries.
> + */