netdev - Re: [RFC PATCH 0/2] udp: avoid false sharing on sk

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] [day] [month] [year] [list]

Message-ID: <67aac121f14e6_a7f6e2949b@willemb.c.googlers.com.notmuch>
Date: Mon, 10 Feb 2025 22:16:49 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Eric Dumazet <edumazet@...gle.com>, 
 Paolo Abeni <pabeni@...hat.com>
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 netdev@...r.kernel.org, 
 Kuniyuki Iwashima <kuniyu@...zon.com>, 
 "David S. Miller" <davem@...emloft.net>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Simon Horman <horms@...nel.org>, 
 Neal Cardwell <ncardwell@...gle.com>, 
 David Ahern <dsahern@...nel.org>
Subject: Re: [RFC PATCH 0/2] udp: avoid false sharing on sk_tsflags

Eric Dumazet wrote:
> On Mon, Feb 10, 2025 at 10:24 PM Paolo Abeni <pabeni@...hat.com> wrote:
> >
> > On 2/10/25 5:16 PM, Paolo Abeni wrote:
> > > I expect the change you propose would perform alike the RFC patches, but
> > > I'll try to do an explicit test later (and report here the results).
> >
> > I ran my test on the sock layout change, and it gave the same (good)
> > results as the RFC. Note that such test uses a single socket receiver,
> > so it's not affected in any way by the eventual increase of touched
> > 'struct sock' cachelines.
> >
> > BTW it just occurred to me that if we could use another bit from
> > sk_flags, something alike the following (completely untested!!!) would
> > do, without changing the struct sock layout and without adding other
> > sock proto ops:
> >
> > ---
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index 8036b3b79cd8..a526db7f5c60 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -954,6 +954,7 @@ enum sock_flags {
> >         SOCK_TSTAMP_NEW, /* Indicates 64 bit timestamps always */
> >         SOCK_RCVMARK, /* Receive SO_MARK  ancillary data with packet */
> >         SOCK_RCVPRIORITY, /* Receive SO_PRIORITY ancillary data with packet */
> > +       SOCK_TIMESTAMPING_ANY, /* sk_tsflags & TSFLAGS_ANY */
> >  };
> >
> >  #define SK_FLAGS_TIMESTAMP ((1UL << SOCK_TIMESTAMP) | (1UL <<
> > SOCK_TIMESTAMPING_RX_SOFTWARE))
> > @@ -2665,12 +2666,12 @@ static inline void sock_recv_cmsgs(struct msghdr
> > *msg, struct sock *sk,
> >  #define FLAGS_RECV_CMSGS ((1UL << SOCK_RXQ_OVFL)                       | \
> >                            (1UL << SOCK_RCVTSTAMP)                      | \
> >                            (1UL << SOCK_RCVMARK)                        |\
> > -                          (1UL << SOCK_RCVPRIORITY))
> > +                          (1UL << SOCK_RCVPRIORITY)                    |\
> > +                          (1UL << SOCK_TIMESTAMPING_ANY))
> >  #define TSFLAGS_ANY      (SOF_TIMESTAMPING_SOFTWARE                    | \
> >                            SOF_TIMESTAMPING_RAW_HARDWARE)
> >
> > -       if (sk->sk_flags & FLAGS_RECV_CMSGS ||
> > -           READ_ONCE(sk->sk_tsflags) & TSFLAGS_ANY)
> > +       if (sk->sk_flags & FLAGS_RECV_CMSGS)
> >                 __sock_recv_cmsgs(msg, sk, skb);
> >         else if (unlikely(sock_flag(sk, SOCK_TIMESTAMP)))
> >                 sock_write_timestamp(sk, skb->tstamp);
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index eae2ae70a2e0..a197f0a0b878 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -938,6 +938,7 @@ int sock_set_timestamping(struct sock *sk, int optname,
> >
> >         WRITE_ONCE(sk->sk_tsflags, val);
> >         sock_valbool_flag(sk, SOCK_TSTAMP_NEW, optname == SO_TIMESTAMPING_NEW);
> > +       sock_valbool_flag(sk, SOCK_TIMESTAMPING_ANY, !!(val & TSFLAGS_ANY));
> >
> >         if (val & SOF_TIMESTAMPING_RX_SOFTWARE)
> >                 sock_enable_timestamp(sk,
> 
> This looks nice indeed.

+1

I thought we could refine to use RX bits rather than TSFLAGS_ANY.
But not trivial, and today already uses TSFLAGS_ANY so out of scope
for this patch. sock_recv_timestamp has weird behavior: returning a
timestamp also if SOF_TIMESTAMPING_SOFTWARE is set without an RX flag.