Re: [PATCH net-next v5 3/4] net: devmem: use niov array for token management

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Tue, Oct 28, 2025 at 07:04:15PM -0700, Mina Almasry wrote:
> On Tue, Oct 28, 2025 at 1:49 PM Bobby Eshleman <bobbyeshleman@gmail.com> wrote:
> ...
> > > > @@ -307,6 +331,7 @@ net_devmem_bind_dmabuf(struct net_device *dev,
> > > >                 goto err_free_chunks;
> > > >
> > > >         list_add(&binding->list, &priv->bindings);
> > > > +       binding->autorelease = true;
> > > >
> > >
> > > So autorelease is indeed a property of the binding. Not sure why a
> > > copy exists in sk_devmem_info. Perf optimization to reduce pointer
> > > chasing?
> > >
> >
> > Just stale code from prior design... Originally, I was going to try to
> > allow the autorelease == true case to be free of the
> > one-binding-per-socket restriction, in which case sk_devmem_info.binding
> > would be NULL (or otherwise meaningless). sk_devmem_info.autorelease
> > allowed sock_devmem_dontneed to choose the right path even when
> > sk_devmem_info.binding == NULL.
> >
> > ...but then I realized we still needed some restriction to avoid sockets
> > from steering into different dmabufs with different autorelease configs,
> > so kept the one-binding restriction for both modes. I abandoned the
> > effort, but forgot to revert this change.
> >
> > Now I'm realizing that we could relax the restriction more though... We
> > could allow sockets to steer into other bindings if they all have the
> > same autorelease value? Then we could still use
> > sk_devmem_info.binding->autorelease in the sock_devmem_dontneed path and
> > relax the restriction to "steering must only be to bindings of the same
> > autorelease mode"?
> >
> 
> Hmpf. I indeed forgot to think thoroughly about the case where, for
> some god-forsaken reason, we have bindings on the system with
> different auto-release values.
> 
> But now that I think more, I don't fully grasp why that would be a
> problem. I think we can make it all work by making autorelease a
> property of the socket, not the binding:
> 
> So if sk->devmem_info.autorelease is on, in recevmsg we store the
> token in the xarray and dontneed frees from the xarray (both can check
> skb->devmem_info.autorelease).
> 
> If sk->devmem_info.autorelease is off, then in recvmsg we grab the
> binding from sk->devmem_info.binding, and we do a uref inc and netmem
> get ref, then in dontneed dec uref and napi_pp_put_page if necessary.
> 
> The side effect of that is that for the same binding, we may
> simultaneously have refs in the sk->xarray and in the binding->uref,
> because the data landing on the binding sometimes belonged to a socket
> with sk->devmem_info.autorelease on or off, but I don't immediately
> see why that would be a problem. The xarray refs would be removed on
> socket close, the urefs would be freed on unbind.
> 
> Doesn't it all work? Or am I insane?
> 

No not insane. I think that works really well and will simplify things a
lot. Let's give that a whirl for the next rev.

[...]

> > > >
> > > > +static noinline_for_stack int
> > > > +sock_devmem_dontneed_manual_release(struct sock *sk, struct dmabuf_token *tokens,
> > > > +                                   unsigned int num_tokens)
> > > > +{
> > > > +       unsigned int netmem_num = 0;
> > > > +       int ret = 0, num_frags = 0;
> > > > +       netmem_ref netmems[16];
> > > > +       struct net_iov *niov;
> > > > +       unsigned int i, j, k;
> > > > +
> > > > +       for (i = 0; i < num_tokens; i++) {
> > > > +               for (j = 0; j < tokens[i].token_count; j++) {
> > > > +                       struct net_iov *niov;
> > > > +                       unsigned int token;
> > > > +                       netmem_ref netmem;
> > > > +
> > > > +                       token = tokens[i].token_start + j;
> > > > +                       if (token >= sk->sk_devmem_info.binding->dmabuf->size / PAGE_SIZE)
> > > > +                               break;
> > > > +
> > >
> > > This requires some thought. The correct thing to do here is EINVAL
> > > without modifying the urefs at all I think. You may need an
> > > input-verification loop. Breaking and returning success here is not
> > > great, I think.
> > >
> >
> > Should this also be changed for the other path as well? Right now if
> > __xa_erase returns NULL (e.g., user passed in a bad token), then we hit
> > "continue" and process the next token... eventually just returning the
> > number of tokens that were successfully processed and omitting the wrong
> > ones.
> >
> 
> Ugh. I did not notice that :(
> 
> I guess the existing dontneed doesn't handle that well anyway. Lets
> not fix too much in this series. It's fine to carry that behavior in
> the new implementation and if anything improve this in a separate
> patch (for me at least). It'd be a bit weird if the userspace is
> sending us bad tokens anyway, in theory.
> 

Duly noted. I'll leave that for future work.

[...]

> > > >  static noinline_for_stack int
> > > >  sock_devmem_dontneed_autorelease(struct sock *sk, struct dmabuf_token *tokens,
> > > >                                  unsigned int num_tokens)
> > > > @@ -1089,32 +1142,32 @@ sock_devmem_dontneed_autorelease(struct sock *sk, struct dmabuf_token *tokens,
> > > >         int ret = 0, num_frags = 0;
> > > >         netmem_ref netmems[16];
> > > >
> > > > -       xa_lock_bh(&sk->sk_user_frags);
> > > > +       xa_lock_bh(&sk->sk_devmem_info.frags);
> > > >         for (i = 0; i < num_tokens; i++) {
> > > >                 for (j = 0; j < tokens[i].token_count; j++) {
> > > >                         if (++num_frags > MAX_DONTNEED_FRAGS)
> > > >                                 goto frag_limit_reached;
> > > >
> > > >                         netmem_ref netmem = (__force netmem_ref)__xa_erase(
> > > > -                               &sk->sk_user_frags, tokens[i].token_start + j);
> > > > +                               &sk->sk_devmem_info.frags, tokens[i].token_start + j);
> > > >
> > > >                         if (!netmem || WARN_ON_ONCE(!netmem_is_net_iov(netmem)))
> > > >                                 continue;
> > > >
> > > >                         netmems[netmem_num++] = netmem;
> > > >                         if (netmem_num == ARRAY_SIZE(netmems)) {
> > > > -                               xa_unlock_bh(&sk->sk_user_frags);
> > > > +                               xa_unlock_bh(&sk->sk_devmem_info.frags);
> > > >                                 for (k = 0; k < netmem_num; k++)
> > > >                                         WARN_ON_ONCE(!napi_pp_put_page(netmems[k]));
> > > >                                 netmem_num = 0;
> > > > -                               xa_lock_bh(&sk->sk_user_frags);
> > > > +                               xa_lock_bh(&sk->sk_devmem_info.frags);
> > > >                         }
> > > >                         ret++;
> > > >                 }
> > > >         }
> > > >
> > > >  frag_limit_reached:
> > > > -       xa_unlock_bh(&sk->sk_user_frags);
> > > > +       xa_unlock_bh(&sk->sk_devmem_info.frags);
> > > >         for (k = 0; k < netmem_num; k++)
> > > >                 WARN_ON_ONCE(!napi_pp_put_page(netmems[k]));
> > > >
> > > > @@ -1135,6 +1188,12 @@ sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen)
> > > >             optlen > sizeof(*tokens) * MAX_DONTNEED_TOKENS)
> > > >                 return -EINVAL;
> > > >
> > > > +       /* recvmsg() has never returned a token for this socket, which needs to
> > > > +        * happen before we know if the dmabuf has autorelease set or not.
> > > > +        */
> > > > +       if (!sk->sk_devmem_info.binding)
> > > > +               return -EINVAL;
> > > > +
> > >
> > > Hmm. At first glance I don't think enforcing this condition if
> > > binding->autorelease is necessary, no?
> > >
> > > If autorelease is on, then we track the tokens the old way, and we
> > > don't need a binding, no? If it's off, then we need an associated
> > > binding, to look up the urefs array.
> > >
> >
> > We at least need the binding to know if binding->autorelease is on,
> > since without that we don't know whether the tokens are in the xarray or
> > binding->vec... but I guess we could also check if the xarray is
> > non-empty and infer that autorelease == true from that?
> >
> 
> I think as above, if autorelease is (only) a property of the sockets,
> then the xarray path works without introducing the socket-to-binding
> mapping restriction, yes?

Indeed, makes sense!

Best,
Bobby