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

[Paolo Abeni]

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