Re: [PATCH] ksmbd: server: avoid busy polling in accept loop

[Namjae Jeon]

On Tue, Nov 11, 2025 at 5:03 PM Qingfang Deng <dqfext@xxxxxxxxx> wrote:
>
> Hi Stefan,
>
> On Tue, Nov 11, 2025 at 3:16 PM Stefan Metzmacher <metze@xxxxxxxxx> wrote:
> > >> Also remove:
> > >>    - TCP_NODELAY, which has no effect on a listening socket.
> > >>    - sk_rcvtimeo and sk_sndtimeo assignments, which only caused accept()
> > >>      to return -EAGAIN prematurely.
> > >
> > > Aren't these inherited to the accepted sockets?
> > > So we need to apply them to the accepted sockets now
> > > instead of dropping them completely?
>
> You're right, TCP_NODELAY of a new accepted socket is inherited from
> the listen socket, so it should not be removed.
>
> >
> > Actually the timeouts are added to the client connection,
> > but not the TCP_NODELAY.
> >
> > But looking at it more detailed I'm wondering if this might
> > introduce a deadlock.
> >
> > We have this in the accepting thread:
> >
> >          while (!kthread_should_stop()) {
> >                  mutex_lock(&iface->sock_release_lock);
> >                  if (!iface->ksmbd_socket) {
> >                          mutex_unlock(&iface->sock_release_lock);
> >                          break;
> >                  }
> >                  ret = kernel_accept(iface->ksmbd_socket, &client_sk, 0);
> >                  mutex_unlock(&iface->sock_release_lock);
> >                  if (ret)
> >                          continue;
> >
> >
> > And in the stopping code this:
> >
> >          case NETDEV_DOWN:
> >                  iface = ksmbd_find_netdev_name_iface_list(netdev->name);
> >                  if (iface && iface->state == IFACE_STATE_CONFIGURED) {
> >                          ksmbd_debug(CONN, "netdev-down event: netdev(%s) is going down\n",
> >                                          iface->name);
> >                          tcp_stop_kthread(iface->ksmbd_kthread);
> >                          iface->ksmbd_kthread = NULL;
> >                          mutex_lock(&iface->sock_release_lock);
> >                          tcp_destroy_socket(iface->ksmbd_socket);
> >                          iface->ksmbd_socket = NULL;
> >                          mutex_unlock(&iface->sock_release_lock);
> >
> >                          iface->state = IFACE_STATE_DOWN;
> >                          break;
> >                  }
> >
> >
> >
> > I guess that now kernel_accept() call waits forever holding iface->sock_release_lock
> > and tcp_stop_kthread(iface->ksmbd_kthread); doesn't have any impact anymore
> > as we may never reach kthread_should_stop() anymore.
> >
> > We may want to do a kernel_sock_shutdown(ksmbd_socket, SHUT_RDWR) after
> > tcp_stop_kthread(iface->ksmbd_kthread); but before mutex_lock(&iface->sock_release_lock);
> > so that kernel_accept() hopefully returns directly.
> > And we only call sock_release(ksmbd_socket); under the iface->sock_release_lock mutex.
>
> In kernel v6.1 or later, kthread_stop() in tcp_stop_kthread() will
> send a signal to the ksmbd kthread so accept() will return -EINTR.
> Before v6.1 it can actually get stuck, as accept() will block forever.
>
> If you're fixing the issue when this patch was backported to versions
> before v6.1, this will not work, because kthread_stop() blocks until
> the target kthread returns, so shutdown() will never get called. The
> sock_release_lock mutex seems redundant because of that.
> Instead, shutdown() can be called _before_ kthread_stop() so accept()
> will return -EINVAL.
>
> Namjae, should I send a v2 with both issues addressed?
Yes, please send the v2 patch.
Thanks.
>
> -- Qingfang