| Message ID | 20240328144032.1864988-2-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 60557969951304dad829f2829019907dfb43ecb3 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | udp: small changes on receive path | expand |
Eric Dumazet wrote: > sk->sk_rcvbuf is read locklessly twice, while other threads > could change its value. > > Use a READ_ONCE() to annotate the race. > > Signed-off-by: Eric Dumazet <edumazet@google.com> > --- > net/ipv4/udp.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c > index 661d0e0d273f616ad82746b69b2c76d056633017..f2736e8958187e132ef45d8e25ab2b4ea7bcbc3d 100644 > --- a/net/ipv4/udp.c > +++ b/net/ipv4/udp.c > @@ -1492,13 +1492,14 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb) > struct sk_buff_head *list = &sk->sk_receive_queue; > int rmem, err = -ENOMEM; > spinlock_t *busy = NULL; > - int size; > + int size, rcvbuf; > > - /* try to avoid the costly atomic add/sub pair when the receive > - * queue is full; always allow at least a packet > + /* Immediately drop when the receive queue is full. > + * Always allow at least one packet. > */ > rmem = atomic_read(&sk->sk_rmem_alloc); > - if (rmem > sk->sk_rcvbuf) > + rcvbuf = READ_ONCE(sk->sk_rcvbuf); > + if (rmem > rcvbuf) > goto drop; > > /* Under mem pressure, it might be helpful to help udp_recvmsg() > @@ -1507,7 +1508,7 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb) > * - Less cache line misses at copyout() time > * - Less work at consume_skb() (less alien page frag freeing) > */ > - if (rmem > (sk->sk_rcvbuf >> 1)) { > + if (rmem > (rcvbuf >> 1)) { > skb_condense(skb); > > busy = busylock_acquire(sk); There's a third read in this function: /* we drop only if the receive buf is full and the receive * queue contains some other skb */ rmem = atomic_add_return(size, &sk->sk_rmem_alloc); if (rmem > (size + (unsigned int)sk->sk_rcvbuf)) goto uncharge_drop; Another READ_ONCE if intent is to not use the locally cached copy?
Willem de Bruijn wrote: > Eric Dumazet wrote: > > sk->sk_rcvbuf is read locklessly twice, while other threads > > could change its value. > > > > Use a READ_ONCE() to annotate the race. > > > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > --- > > net/ipv4/udp.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c > > index 661d0e0d273f616ad82746b69b2c76d056633017..f2736e8958187e132ef45d8e25ab2b4ea7bcbc3d 100644 > > --- a/net/ipv4/udp.c > > +++ b/net/ipv4/udp.c > > @@ -1492,13 +1492,14 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb) > > struct sk_buff_head *list = &sk->sk_receive_queue; > > int rmem, err = -ENOMEM; > > spinlock_t *busy = NULL; > > - int size; > > + int size, rcvbuf; > > > > - /* try to avoid the costly atomic add/sub pair when the receive > > - * queue is full; always allow at least a packet > > + /* Immediately drop when the receive queue is full. > > + * Always allow at least one packet. > > */ > > rmem = atomic_read(&sk->sk_rmem_alloc); > > - if (rmem > sk->sk_rcvbuf) > > + rcvbuf = READ_ONCE(sk->sk_rcvbuf); > > + if (rmem > rcvbuf) > > goto drop; > > > > /* Under mem pressure, it might be helpful to help udp_recvmsg() > > @@ -1507,7 +1508,7 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb) > > * - Less cache line misses at copyout() time > > * - Less work at consume_skb() (less alien page frag freeing) > > */ > > - if (rmem > (sk->sk_rcvbuf >> 1)) { > > + if (rmem > (rcvbuf >> 1)) { > > skb_condense(skb); > > > > busy = busylock_acquire(sk); > > There's a third read in this function: But you remove that in the next patch. Ok.
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index 661d0e0d273f616ad82746b69b2c76d056633017..f2736e8958187e132ef45d8e25ab2b4ea7bcbc3d 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -1492,13 +1492,14 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb) struct sk_buff_head *list = &sk->sk_receive_queue; int rmem, err = -ENOMEM; spinlock_t *busy = NULL; - int size; + int size, rcvbuf; - /* try to avoid the costly atomic add/sub pair when the receive - * queue is full; always allow at least a packet + /* Immediately drop when the receive queue is full. + * Always allow at least one packet. */ rmem = atomic_read(&sk->sk_rmem_alloc); - if (rmem > sk->sk_rcvbuf) + rcvbuf = READ_ONCE(sk->sk_rcvbuf); + if (rmem > rcvbuf) goto drop; /* Under mem pressure, it might be helpful to help udp_recvmsg() @@ -1507,7 +1508,7 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb) * - Less cache line misses at copyout() time * - Less work at consume_skb() (less alien page frag freeing) */ - if (rmem > (sk->sk_rcvbuf >> 1)) { + if (rmem > (rcvbuf >> 1)) { skb_condense(skb); busy = busylock_acquire(sk);
sk->sk_rcvbuf is read locklessly twice, while other threads could change its value. Use a READ_ONCE() to annotate the race. Signed-off-by: Eric Dumazet <edumazet@google.com> --- net/ipv4/udp.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)