diff mbox series

[v4,net-next,3/6] net-smnp: reorganize SNMP fast path variables

Message ID 20231026081959.3477034-4-lixiaoyan@google.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Analyze and Reorganize core Networking Structs to optimize cacheline consumption | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5697 this patch: 5697
netdev/cc_maintainers warning 4 maintainers not CCed: davem@davemloft.net heng.guo@windriver.com jamie.bainbridge@gmail.com ycheng@google.com
netdev/build_clang success Errors and warnings before: 1691 this patch: 1691
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 6060 this patch: 6060
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 84 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Coco Li Oct. 26, 2023, 8:19 a.m. UTC 
From: Chao Wu <wwchao@google.com>

Reorganize fast path variables on tx-txrx-rx order.
Fast path cacheline ends afer LINUX_MIB_DELAYEDACKLOCKED.
There are only read-write variables here.

NOTE: Kernel exports these counters with a leading line with the
names of the metrics. User space binaries not ignoreing the
metric names will not be affected by the change of order here. An
example can be seen by looking at /proc/net/netstat.

Below data generated with pahole on x86 architecture.

Fast path variables span cache lines before change: 12
Fast path variables span cache lines after change: 2

Signed-off-by: Chao Wu <wwchao@google.com>
Signed-off-by: Coco Li <lixiaoyan@google.com>
Suggested-by: Eric Dumazet <edumazet@google.com>
Reviewed-by: David Ahern <dsahern@kernel.org>
---
 include/uapi/linux/snmp.h | 41 ++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

Comments

Jakub Kicinski Oct. 26, 2023, 2:20 p.m. UTC | #1 
On Thu, 26 Oct 2023 08:19:56 +0000 Coco Li wrote:
> Subject: [PATCH v4 net-next 3/6] net-smnp: reorganize SNMP fast path variables

s/smnp/snmp/

> names of the metrics. User space binaries not ignoreing the

ignoring

> +/* Enums in this file are exported by their name and by
> + * their values. User space binaries should ingest both
> + * of the above, and therefore ordering changes in this
> + * file does not break user space. For an example, please
> + * see the output of /proc/net/netstat.

I don't understand, what does it mean to be exposed by value?
User space uses the enum to offset into something or not?
If not why don't we move the enum out of uAPI entirely?

> +	/* Caacheline organization can be found documented in

Cacheline

Please invest (your time) a spell check :S
Coco Li Oct. 26, 2023, 11:52 p.m. UTC | #2 
On Thu, Oct 26, 2023 at 7:20 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 26 Oct 2023 08:19:56 +0000 Coco Li wrote:
> > Subject: [PATCH v4 net-next 3/6] net-smnp: reorganize SNMP fast path variables
>
> s/smnp/snmp/
>
> > names of the metrics. User space binaries not ignoreing the
>
> ignoring
>
> > +/* Enums in this file are exported by their name and by
> > + * their values. User space binaries should ingest both
> > + * of the above, and therefore ordering changes in this
> > + * file does not break user space. For an example, please
> > + * see the output of /proc/net/netstat.
>
> I don't understand, what does it mean to be exposed by value?
> User space uses the enum to offset into something or not?
> If not why don't we move the enum out of uAPI entirely?
>
I mostly meant that i.e. cat /proc/net/netstat will export enum names
first, and that userspace binary should consume both the name and the
value.

I have no objections to moving the enums outside, but that seems a bit
tangential to the purpose of this patch series.

> > +     /* Caacheline organization can be found documented in
>
> Cacheline
>
> Please invest (your time) a spell check :S

Much apologies, will run through spell checkers in the future.
Jakub Kicinski Oct. 27, 2023, 1:23 a.m. UTC | #3 
On Thu, 26 Oct 2023 16:52:35 -0700 Coco Li wrote:
> I have no objections to moving the enums outside, but that seems a bit
> tangential to the purpose of this patch series.

My thinking is - we assume we can reshuffle this enum, because nobody
uses the enum values directly. If someone does, tho, we would be
breaking binary compatibility.

Moving it out of include/uapi/ would break the build for anyone trying
to refer to the enum, That gives us quicker signal that we may have
broken someone's code.
Eric Dumazet Oct. 27, 2023, 7:55 a.m. UTC | #4 
On Fri, Oct 27, 2023 at 3:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 26 Oct 2023 16:52:35 -0700 Coco Li wrote:
> > I have no objections to moving the enums outside, but that seems a bit
> > tangential to the purpose of this patch series.
>
> My thinking is - we assume we can reshuffle this enum, because nobody
> uses the enum values directly. If someone does, tho, we would be
> breaking binary compatibility.
>
> Moving it out of include/uapi/ would break the build for anyone trying
> to refer to the enum, That gives us quicker signal that we may have
> broken someone's code.

Note that we already in the past shuffled values without anyone objecting...

We probably can move the enums out of uapi, I suggest we remove this
patch from the series
and do that in the next cycle, I think other reorgs are more important.
Coco Li Oct. 27, 2023, 8:18 p.m. UTC | #5 
On Fri, Oct 27, 2023, 12:55 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Oct 27, 2023 at 3:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Thu, 26 Oct 2023 16:52:35 -0700 Coco Li wrote:
> > > I have no objections to moving the enums outside, but that seems a bit
> > > tangential to the purpose of this patch series.
> >
> > My thinking is - we assume we can reshuffle this enum, because nobody
> > uses the enum values directly. If someone does, tho, we would be
> > breaking binary compatibility.
> >
> > Moving it out of include/uapi/ would break the build for anyone trying
> > to refer to the enum, That gives us quicker signal that we may have
> > broken someone's code.
>
> Note that we already in the past shuffled values without anyone objecting...
>
> We probably can move the enums out of uapi, I suggest we remove this
> patch from the series
> and do that in the next cycle, I think other reorgs are more important.

I agree. Will remove this commit in v5 of patch which I will update
soon, and send in a separate patch series to move enum out of uapi and
reorganize snmp counters.

Thanks for the discussion!
diff mbox series

Patch

diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h
index b2b72886cb6d1..70be81c1fdb6d 100644
--- a/include/uapi/linux/snmp.h
+++ b/include/uapi/linux/snmp.h
@@ -8,6 +8,13 @@ 
 #ifndef _LINUX_SNMP_H
 #define _LINUX_SNMP_H
 
+/* Enums in this file are exported by their name and by
+ * their values. User space binaries should ingest both
+ * of the above, and therefore ordering changes in this
+ * file does not break user space. For an example, please
+ * see the output of /proc/net/netstat.
+ */
+
 /* ipstats mib definitions */
 /*
  * RFC 1213:  MIB-II
@@ -170,7 +177,28 @@  enum
 /* linux mib definitions */
 enum
 {
+	/* Caacheline organization can be found documented in
+	 * Documentation/networking/net_cachelines/snmp.rst.
+	 * Please update the document when adding new fields.
+	 */
+
 	LINUX_MIB_NUM = 0,
+	/* TX hotpath */
+	LINUX_MIB_TCPAUTOCORKING,		/* TCPAutoCorking */
+	LINUX_MIB_TCPFROMZEROWINDOWADV,		/* TCPFromZeroWindowAdv */
+	LINUX_MIB_TCPTOZEROWINDOWADV,		/* TCPToZeroWindowAdv */
+	LINUX_MIB_TCPWANTZEROWINDOWADV,		/* TCPWantZeroWindowAdv */
+	LINUX_MIB_TCPORIGDATASENT,		/* TCPOrigDataSent */
+	LINUX_MIB_TCPPUREACKS,			/* TCPPureAcks */
+	LINUX_MIB_TCPHPACKS,			/* TCPHPAcks */
+	LINUX_MIB_TCPDELIVERED,			/* TCPDelivered */
+	/* RX hotpath */
+	LINUX_MIB_TCPHPHITS,			/* TCPHPHits */
+	LINUX_MIB_TCPRCVCOALESCE,		/* TCPRcvCoalesce */
+	LINUX_MIB_TCPKEEPALIVE,			/* TCPKeepAlive */
+	LINUX_MIB_DELAYEDACKS,			/* DelayedACKs */
+	LINUX_MIB_DELAYEDACKLOCKED,		/* DelayedACKLocked */
+	/* End of hotpath variables */
 	LINUX_MIB_SYNCOOKIESSENT,		/* SyncookiesSent */
 	LINUX_MIB_SYNCOOKIESRECV,		/* SyncookiesRecv */
 	LINUX_MIB_SYNCOOKIESFAILED,		/* SyncookiesFailed */
@@ -186,14 +214,9 @@  enum
 	LINUX_MIB_TIMEWAITKILLED,		/* TimeWaitKilled */
 	LINUX_MIB_PAWSACTIVEREJECTED,		/* PAWSActiveRejected */
 	LINUX_MIB_PAWSESTABREJECTED,		/* PAWSEstabRejected */
-	LINUX_MIB_DELAYEDACKS,			/* DelayedACKs */
-	LINUX_MIB_DELAYEDACKLOCKED,		/* DelayedACKLocked */
 	LINUX_MIB_DELAYEDACKLOST,		/* DelayedACKLost */
 	LINUX_MIB_LISTENOVERFLOWS,		/* ListenOverflows */
 	LINUX_MIB_LISTENDROPS,			/* ListenDrops */
-	LINUX_MIB_TCPHPHITS,			/* TCPHPHits */
-	LINUX_MIB_TCPPUREACKS,			/* TCPPureAcks */
-	LINUX_MIB_TCPHPACKS,			/* TCPHPAcks */
 	LINUX_MIB_TCPRENORECOVERY,		/* TCPRenoRecovery */
 	LINUX_MIB_TCPSACKRECOVERY,		/* TCPSackRecovery */
 	LINUX_MIB_TCPSACKRENEGING,		/* TCPSACKReneging */
@@ -247,7 +270,6 @@  enum
 	LINUX_MIB_TCPREQQFULLDOCOOKIES,		/* TCPReqQFullDoCookies */
 	LINUX_MIB_TCPREQQFULLDROP,		/* TCPReqQFullDrop */
 	LINUX_MIB_TCPRETRANSFAIL,		/* TCPRetransFail */
-	LINUX_MIB_TCPRCVCOALESCE,		/* TCPRcvCoalesce */
 	LINUX_MIB_TCPBACKLOGCOALESCE,		/* TCPBacklogCoalesce */
 	LINUX_MIB_TCPOFOQUEUE,			/* TCPOFOQueue */
 	LINUX_MIB_TCPOFODROP,			/* TCPOFODrop */
@@ -263,12 +285,7 @@  enum
 	LINUX_MIB_TCPFASTOPENBLACKHOLE,		/* TCPFastOpenBlackholeDetect */
 	LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES, /* TCPSpuriousRtxHostQueues */
 	LINUX_MIB_BUSYPOLLRXPACKETS,		/* BusyPollRxPackets */
-	LINUX_MIB_TCPAUTOCORKING,		/* TCPAutoCorking */
-	LINUX_MIB_TCPFROMZEROWINDOWADV,		/* TCPFromZeroWindowAdv */
-	LINUX_MIB_TCPTOZEROWINDOWADV,		/* TCPToZeroWindowAdv */
-	LINUX_MIB_TCPWANTZEROWINDOWADV,		/* TCPWantZeroWindowAdv */
 	LINUX_MIB_TCPSYNRETRANS,		/* TCPSynRetrans */
-	LINUX_MIB_TCPORIGDATASENT,		/* TCPOrigDataSent */
 	LINUX_MIB_TCPHYSTARTTRAINDETECT,	/* TCPHystartTrainDetect */
 	LINUX_MIB_TCPHYSTARTTRAINCWND,		/* TCPHystartTrainCwnd */
 	LINUX_MIB_TCPHYSTARTDELAYDETECT,	/* TCPHystartDelayDetect */
@@ -280,10 +297,8 @@  enum
 	LINUX_MIB_TCPACKSKIPPEDTIMEWAIT,	/* TCPACKSkippedTimeWait */
 	LINUX_MIB_TCPACKSKIPPEDCHALLENGE,	/* TCPACKSkippedChallenge */
 	LINUX_MIB_TCPWINPROBE,			/* TCPWinProbe */
-	LINUX_MIB_TCPKEEPALIVE,			/* TCPKeepAlive */
 	LINUX_MIB_TCPMTUPFAIL,			/* TCPMTUPFail */
 	LINUX_MIB_TCPMTUPSUCCESS,		/* TCPMTUPSuccess */
-	LINUX_MIB_TCPDELIVERED,			/* TCPDelivered */
 	LINUX_MIB_TCPDELIVEREDCE,		/* TCPDeliveredCE */
 	LINUX_MIB_TCPACKCOMPRESSED,		/* TCPAckCompressed */
 	LINUX_MIB_TCPZEROWINDOWDROP,		/* TCPZeroWindowDrop */