Received-SPF: pass (google.com: domain of linux-kernel+bounces-214694-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 14 Jun 2024 11:48:09 +0200
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: Eric Dumazet <edumazet@google.com>
Cc: Steven Rostedt <rostedt@goodmis.org>, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
	Daniel Bristot de Oliveira <bristot@kernel.org>,
	Boqun Feng <boqun.feng@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Frederic Weisbecker <frederic@kernel.org>,
	Ingo Molnar <mingo@redhat.com>, Jakub Kicinski <kuba@kernel.org>,
	Paolo Abeni <pabeni@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Waiman Long <longman@redhat.com>, Will Deacon <will@kernel.org>,
	Ben Segall <bsegall@google.com>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Juri Lelli <juri.lelli@redhat.com>, Mel Gorman <mgorman@suse.de>,
	Valentin Schneider <vschneid@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>
Subject: Re: [PATCH v6 net-next 08/15] net: softnet_data: Make xmit.recursion
 per task.
Message-ID: <20240614094809.gvOugqZT@linutronix.de>
References: <20240612170303.3896084-1-bigeasy@linutronix.de>
 <20240612170303.3896084-9-bigeasy@linutronix.de>
 <20240612131829.2e33ca71@rorschach.local.home>
 <20240614082758.6pSMV3aq@linutronix.de>
 <CANn89i+YfdmKSMgHni4ogMDq0BpFQtjubA0RxXcfZ8fpgV5_fw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CANn89i+YfdmKSMgHni4ogMDq0BpFQtjubA0RxXcfZ8fpgV5_fw@mail.gmail.com>

On 2024-06-14 10:38:15 [+0200], Eric Dumazet wrote:
> > I think it should work fine. netdev folks, you want me to remove that
> > ifdef and use a per-Task counter unconditionally?
>=20
> It depends if this adds another cache line miss/dirtying or not.
>=20
> What about other fields from softnet_data.xmit ?

duh. Looking at the `more' member I realise that this needs to move to
task_struct on RT, too. Therefore I would move the whole xmit struct.

The xmit cacheline starts within the previous member (xfrm_backlog) and
ends before the following member starts. So it kind of has its own
cacheline.
With defconfig, if we move it to the front of task struct then we go from

| struct task_struct {
|         struct thread_info         thread_info;          /*     0    24 */
|         unsigned int               __state;              /*    24     4 */
|         unsigned int               saved_state;          /*    28     4 */
|         void *                     stack;                /*    32     8 */
|         refcount_t                 usage;                /*    40     4 */
|         unsigned int               flags;                /*    44     4 */
|         unsigned int               ptrace;               /*    48     4 */
|         int                        on_cpu;               /*    52     4 */
|         struct __call_single_node  wake_entry;           /*    56    16 */
|         /* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
|         unsigned int               wakee_flips;          /*    72     4 */
|=20
|         /* XXX 4 bytes hole, try to pack */
|=20
|         long unsigned int          wakee_flip_decay_ts;  /*    80     8 */

to

| struct task_struct {
|         struct thread_info         thread_info;          /*     0    24 */
|         unsigned int               __state;              /*    24     4 */
|         unsigned int               saved_state;          /*    28     4 */
|         void *                     stack;                /*    32     8 */
|         refcount_t                 usage;                /*    40     4 */
|         unsigned int               flags;                /*    44     4 */
|         unsigned int               ptrace;               /*    48     4 */
|         struct {
|                 u16                recursion;            /*    52     2 */
|                 u8                 more;                 /*    54     1 */
|                 u8                 skip_txqueue;         /*    55     1 */
|         } xmit;                                          /*    52     4 */
|         struct __call_single_node  wake_entry;           /*    56    16 */
|         /* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
|         int                        on_cpu;               /*    72     4 */
|         unsigned int               wakee_flips;          /*    76     4 */
|         long unsigned int          wakee_flip_decay_ts;  /*    80     8 */


stuffed a hole due to adding `xmit' and moving `on_cpu'. In the end the
total size of task_struct remained the same.
The cache line should be hot due to `flags' usage in

| static void handle_softirqs(bool ksirqd)
| {
|          unsigned long old_flags =3D current->flags;
=E2=80=A6
|         current->flags &=3D ~PF_MEMALLOC;

Then there is a bit of code before net_XX_action() and the usage of
either of the members so not sure if it is gone by then=E2=80=A6

Is this what we want or not?

Sebastian