On Tue, Oct 28, 2025 at 07:04:15PM -0700, Mina Almasry wrote:

On Tue, Oct 28, 2025 at 1:49 PM Bobby Eshleman [off-list ref] wrote:
...

quoted

quoted

quoted

@@ -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.

[...]

quoted

quoted

quoted

+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.

[...]

quoted

quoted

quoted

 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