From: Qingfang Deng <dqfext@gmail.com>
To: Stefan Metzmacher <metze@samba.org>
Cc: Namjae Jeon <linkinjeon@kernel.org>,
Steve French <smfrench@gmail.com>,
Sergey Senozhatsky <senozhatsky@chromium.org>,
Tom Talpey <tom@talpey.com>,
Ronnie Sahlberg <lsahlber@redhat.com>,
Hyunchul Lee <hyc.lee@gmail.com>,
linux-cifs@vger.kernel.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org
Subject: Re: [PATCH] ksmbd: server: avoid busy polling in accept loop
Date: Tue, 11 Nov 2025 16:03:03 +0800 [thread overview]
Message-ID: <CALW65jav2wiWzz6q6vdnjL88GJB1eWJtLVzH3M1CkOHbdgSDWw@mail.gmail.com> (raw)
In-Reply-To: <10da0cb9-8c92-413d-b8df-049279100458@samba.org>
Hi Stefan,
On Tue, Nov 11, 2025 at 3:16 PM Stefan Metzmacher <metze@samba.org> 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?
-- Qingfang
next prev parent reply other threads:[~2025-11-11 8:03 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-30 6:47 [PATCH] ksmbd: server: avoid busy polling in accept loop Qingfang Deng
2025-10-30 8:11 ` Namjae Jeon
2025-10-31 7:32 ` Qingfang Deng
2025-10-31 7:44 ` Greg Kroah-Hartman
2025-10-31 7:49 ` Qingfang Deng
2025-10-31 23:50 ` Namjae Jeon
2025-11-11 6:55 ` Stefan Metzmacher
2025-11-11 7:16 ` Stefan Metzmacher
2025-11-11 8:03 ` Qingfang Deng [this message]
2025-11-11 8:47 ` Namjae Jeon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CALW65jav2wiWzz6q6vdnjL88GJB1eWJtLVzH3M1CkOHbdgSDWw@mail.gmail.com \
--to=dqfext@gmail.com \
--cc=hyc.lee@gmail.com \
--cc=linkinjeon@kernel.org \
--cc=linux-cifs@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lsahlber@redhat.com \
--cc=metze@samba.org \
--cc=netdev@vger.kernel.org \
--cc=senozhatsky@chromium.org \
--cc=smfrench@gmail.com \
--cc=tom@talpey.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).