Re: [PATCH v1 bpf-next 7/8] bpf: tcp: Add SOCK_OPS rcvlowat hook.

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 05/08, Kuniyuki Iwashima wrote:
> On Fri, May 8, 2026 at 8:28 AM Stanislav Fomichev <sdf.kernel@gmail.com> wrote:
> >
> > On 05/08, Kuniyuki Iwashima wrote:
> > > Now, it is time to add the new hooks for BPF_SOCK_OPS_RCVLOWAT_CB.
> > >
> > > Let's invoke the BPF SOCK_OPS prog when
> > >
> > >   1. TCP stack enqueues skb to sk->sk_receive_queue
> > >      -> tcp_queue_rcv(), tcp_ofo_queue(), and tcp_fastopen_add_skb()
> > >
> > >   2. TCP recvmsg() completes
> > >      -> __tcp_cleanup_rbuf()
> > >
> > > This will allow the BPF prog to parse each skb and dynamically
> > > adjust sk->sk_rcvlowat to suppress unnecessary EPOLLIN wakeups
> > > until sufficient data (e.g., a full RPC frame) is available
> > > in the receive queue.
> > >
> > > Note that the direct access to bpf_sock_ops.data is intentionally
> > > disabled by passing 0 as end_offset.
> > >
> > > Instead, the BPF prog is supposed to use bpf_skb_load_bytes()
> > > with bpf_sock_ops because payload is not in the linear area
> > > with TCP header/data split on and skb may contain a RPC
> > > descriptor in skb frag.  This also simplifies the BPF prog.
> > >
> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
> >
> > I was reading the series expecting to find some skb_queue_walk-like
> > implementation, but since it's a cgroup hook we obviously don't
> > need to do that.. So at this point BPF_SOCK_OPS_RCVLOWAT_CB_FLAG
> > is basically a "rx queue skb" hook, right?
> 
> If recvmsg() hook w/o skb is not confusing, yes.
> 
> > So should we make
> > the name more generic? There is really nothing lowat-specific
> > here besides your new kfunc to read the payload?
> 
> Do you have any suggestions?  BPF_SOCK_OPS_RCVQ ?

Yeah, idk, maybe something like this :-)

> 
> 
> >
> > > ---
> > >  include/net/tcp.h       | 14 ++++++++++++++
> > >  net/ipv4/tcp.c          |  2 ++
> > >  net/ipv4/tcp_fastopen.c |  2 ++
> > >  net/ipv4/tcp_input.c    | 10 ++++++++++
> > >  4 files changed, 28 insertions(+)
> > >
> > > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > > index 4e9e634e276b..003e46c9b500 100644
> > > --- a/include/net/tcp.h
> > > +++ b/include/net/tcp.h
> > > @@ -737,6 +737,20 @@ static inline struct request_sock *cookie_bpf_check(struct net *net, struct sock
> > >  }
> > >  #endif
> > >
> > > +#ifdef CONFIG_CGROUP_BPF
> > > +void bpf_skops_rcvlowat(struct sock *sk, struct sk_buff *skb);
> > > +
> > > +static inline void tcp_bpf_rcvlowat(struct sock *sk, struct sk_buff *skb)
> > > +{
> > > +     if (BPF_SOCK_OPS_TEST_FLAG(tcp_sk(sk), BPF_SOCK_OPS_RCVLOWAT_CB_FLAG))
> > > +             bpf_skops_rcvlowat(sk, skb);
> > > +}
> > > +#else
> > > +static inline void tcp_bpf_rcvlowat(struct sock *sk, struct sk_buff *skb)
> > > +{
> > > +}
> > > +#endif
> > > +
> > >  /* From net/ipv6/syncookies.c */
> > >  int __cookie_v6_check(const struct ipv6hdr *iph, const struct tcphdr *th);
> > >  struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb);
> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > index 1d9e52fc454f..80144b97a87a 100644
> > > --- a/net/ipv4/tcp.c
> > > +++ b/net/ipv4/tcp.c
> > > @@ -1602,6 +1602,8 @@ void __tcp_cleanup_rbuf(struct sock *sk, int copied)
> > >               tcp_mstamp_refresh(tp);
> > >               tcp_send_ack(sk);
> > >       }
> > > +
> > > +     tcp_bpf_rcvlowat(sk, NULL);
> > >  }
> > >
> > >  void tcp_cleanup_rbuf(struct sock *sk, int copied)
> > > diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> > > index 471c78be5513..91bf421fc5b6 100644
> > > --- a/net/ipv4/tcp_fastopen.c
> > > +++ b/net/ipv4/tcp_fastopen.c
> > > @@ -281,6 +281,8 @@ void tcp_fastopen_add_skb(struct sock *sk, struct sk_buff *skb)
> > >       TCP_SKB_CB(skb)->seq++;
> > >       TCP_SKB_CB(skb)->tcp_flags &= ~TCPHDR_SYN;
> > >
> > > +     tcp_bpf_rcvlowat(sk, skb);
> > > +
> >
> > I'm also not sure about the particular placement of some of these..
> > For example here, why do it before updating tp? Why not after?
> >
> > (and same for tcp_ofo_queue)
> 
> Basically, it's to give the same snapshot with tcp_queue_rcv().
> 
> For example, if you call tcp_bpf_rcvlowat() before updating
> TCP_SKB_CB(skb)->seq in tcp_fastopen_add_skb(), BPF
> prog will need to implement an unlikely if branch to strip SYN.
> 
> Also, TCP stack can queue overlapping skb into recvq.
> Once you update rcv_nxt with a new skb, you cannot infer
> the previous one from skb->len.

I see, makes sense, thanks!