netdev - Re: [PATCH v2 net-next 6/8] net: rps: change input_queue_tail_incr

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]

Date: Sat, 30 Mar 2024 17:01:35 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH v2 net-next 6/8] net: rps: change input_queue_tail_incr_save()

On Sat, Mar 30, 2024 at 3:47 PM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> Hello Eric,
>
> On Fri, Mar 29, 2024 at 11:43 PM Eric Dumazet <edumazet@...gle.com> wrote:
> >
> > input_queue_tail_incr_save() is incrementing the sd queue_tail
> > and save it in the flow last_qtail.
> >
> > Two issues here :
> >
> > - no lock protects the write on last_qtail, we should use appropriate
> >   annotations.
> >
> > - We can perform this write after releasing the per-cpu backlog lock,
> >   to decrease this lock hold duration (move away the cache line miss)
> >
> > Also move input_queue_head_incr() and rps helpers to include/net/rps.h,
> > while adding rps_ prefix to better reflect their role.
> >
> > v2: Fixed a build issue (Jakub and kernel build bots)
> >
> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > ---
> >  include/linux/netdevice.h | 15 ---------------
> >  include/net/rps.h         | 23 +++++++++++++++++++++++
> >  net/core/dev.c            | 20 ++++++++++++--------
> >  3 files changed, 35 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 1c31cd2691d32064613836141fbdeeebc831b21f..14f19cc2616452d7e6afbbaa52f8ad3e61a419e9 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -3249,21 +3249,6 @@ struct softnet_data {
> >         call_single_data_t      defer_csd;
> >  };
> >
> > -static inline void input_queue_head_incr(struct softnet_data *sd)
> > -{
> > -#ifdef CONFIG_RPS
> > -       sd->input_queue_head++;
> > -#endif
> > -}
> > -
> > -static inline void input_queue_tail_incr_save(struct softnet_data *sd,
> > -                                             unsigned int *qtail)
> > -{
> > -#ifdef CONFIG_RPS
> > -       *qtail = ++sd->input_queue_tail;
> > -#endif
> > -}
> > -
> >  DECLARE_PER_CPU_ALIGNED(struct softnet_data, softnet_data);
> >
> >  static inline int dev_recursion_level(void)
> > diff --git a/include/net/rps.h b/include/net/rps.h
> > index 7660243e905b92651a41292e04caf72c5f12f26e..10ca25731c1ef766715fe7ee415ad0b71ec643a8 100644
> > --- a/include/net/rps.h
> > +++ b/include/net/rps.h
> > @@ -122,4 +122,27 @@ static inline void sock_rps_record_flow(const struct sock *sk)
> >  #endif
> >  }
> >
> > +static inline u32 rps_input_queue_tail_incr(struct softnet_data *sd)
> > +{
> > +#ifdef CONFIG_RPS
> > +       return ++sd->input_queue_tail;
> > +#else
> > +       return 0;
> > +#endif
> > +}
> > +
> > +static inline void rps_input_queue_tail_save(u32 *dest, u32 tail)
> > +{
> > +#ifdef CONFIG_RPS
> > +       WRITE_ONCE(*dest, tail);
> > +#endif
> > +}
>
> I wonder if we should also call this new helper to WRITE_ONCE
> last_qtail in the set_rps_cpu()?
>

Absolutely, I have another patch series to address remaining races
(rflow->cpu, rflow->filter ...)

I chose to make a small one, to ease reviews.