netdev - Re: [PATCH v6 net-next 08/15] net: softnet_data: Make xmit.recursion per task.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <834b61b93df3cbf5053e459f337e622e2c510fbd.camel@redhat.com>
Date: Fri, 14 Jun 2024 16:08:42 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>, Eric Dumazet
	 <edumazet@...gle.com>
Cc: Steven Rostedt <rostedt@...dmis.org>, linux-kernel@...r.kernel.org, 
 netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, Daniel
 Bristot de Oliveira <bristot@...nel.org>, Boqun Feng
 <boqun.feng@...il.com>, Daniel Borkmann <daniel@...earbox.net>, Frederic
 Weisbecker <frederic@...nel.org>, Ingo Molnar <mingo@...hat.com>, Jakub
 Kicinski <kuba@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Thomas
 Gleixner <tglx@...utronix.de>, Waiman Long <longman@...hat.com>, Will
 Deacon <will@...nel.org>, Ben Segall <bsegall@...gle.com>, Daniel Bristot
 de Oliveira <bristot@...hat.com>,  Dietmar Eggemann
 <dietmar.eggemann@....com>, Juri Lelli <juri.lelli@...hat.com>, Mel Gorman
 <mgorman@...e.de>,  Valentin Schneider <vschneid@...hat.com>, Vincent
 Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH v6 net-next 08/15] net: softnet_data: Make
 xmit.recursion per task.

On Fri, 2024-06-14 at 11:48 +0200, Sebastian Andrzej Siewior wrote:
> 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?
> > 
> > It depends if this adds another cache line miss/dirtying or not.
> > 
> > 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 */
> > 
> >         /* XXX 4 bytes hole, try to pack */
> > 
> >         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 = current->flags;
> …
> >         current->flags &= ~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…
> 
> Is this what we want or not?

I personally think (fear mostly) there is still the potential for some
(performance) regression. I think it would be safer to introduce this
change under a compiler conditional and eventually follow-up with a
patch making the code generic.

Should such later change prove to be problematic, we could revert it
without impacting the series as a whole. 

Thanks!

Paolo