2023-05-15 13:39:30

by Chuck Lever

Subject: [PATCH 3/4] SUNRPC: Improve observability in svc_tcp_accept()

From: Chuck Lever <[email protected]>

The -ENOMEM arm could fire repeatedly if the system runs low on
memory, so remove it.

Don't bother to trace -EAGAIN error events, since those fire after
a listener is created (with no work done) and once again after an
accept has been handled successfully (again, with no work done).

Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/svcsock.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index e0fb65e90af2..2058641ab9f6 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -885,13 +885,8 @@ static struct svc_xprt *svc_tcp_accept(struct svc_xprt *xprt)
clear_bit(XPT_CONN, &svsk->sk_xprt.xpt_flags);
err = kernel_accept(sock, &newsock, O_NONBLOCK);
if (err < 0) {
- if (err == -ENOMEM)
- printk(KERN_WARNING "%s: no more sockets!\n",
- serv->sv_name);
- else if (err != -EAGAIN)
- net_warn_ratelimited("%s: accept failed (err %d)!\n",
- serv->sv_name, -err);
- trace_svcsock_accept_err(xprt, serv->sv_name, err);
+ if (err != -EAGAIN)
+ trace_svcsock_accept_err(xprt, serv->sv_name, err);
return NULL;
}
set_bit(XPT_CONN, &svsk->sk_xprt.xpt_flags);

2023-05-15 16:55:47

by Jeff Layton

Subject: Re: [PATCH 3/4] SUNRPC: Improve observability in svc_tcp_accept()

On Mon, 2023-05-15 at 09:32 -0400, Chuck Lever wrote:
> From: Chuck Lever <[email protected]>
>
> The -ENOMEM arm could fire repeatedly if the system runs low on
> memory, so remove it.
>
> Don't bother to trace -EAGAIN error events, since those fire after
> a listener is created (with no work done) and once again after an
> accept has been handled successfully (again, with no work done).
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> net/sunrpc/svcsock.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index e0fb65e90af2..2058641ab9f6 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -885,13 +885,8 @@ static struct svc_xprt *svc_tcp_accept(struct svc_xprt *xprt)
> clear_bit(XPT_CONN, &svsk->sk_xprt.xpt_flags);
> err = kernel_accept(sock, &newsock, O_NONBLOCK);
> if (err < 0) {
> - if (err == -ENOMEM)
> - printk(KERN_WARNING "%s: no more sockets!\n",
> - serv->sv_name);
> - else if (err != -EAGAIN)
> - net_warn_ratelimited("%s: accept failed (err %d)!\n",
> - serv->sv_name, -err);
> - trace_svcsock_accept_err(xprt, serv->sv_name, err);
> + if (err != -EAGAIN)
> + trace_svcsock_accept_err(xprt, serv->sv_name, err);

Would this be better done as a TP_CONDITION tracepoint?

> return NULL;
> }
> set_bit(XPT_CONN, &svsk->sk_xprt.xpt_flags);
>
>

--
Jeff Layton <[email protected]>

2023-05-15 17:10:29

by Chuck Lever III

Subject: Re: [PATCH 3/4] SUNRPC: Improve observability in svc_tcp_accept()

> On May 15, 2023, at 12:54 PM, Jeff Layton <[email protected]> wrote:
>
> On Mon, 2023-05-15 at 09:32 -0400, Chuck Lever wrote:
>> From: Chuck Lever <[email protected]>
>>
>> The -ENOMEM arm could fire repeatedly if the system runs low on
>> memory, so remove it.
>>
>> Don't bother to trace -EAGAIN error events, since those fire after
>> a listener is created (with no work done) and once again after an
>> accept has been handled successfully (again, with no work done).
>>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>> net/sunrpc/svcsock.c | 9 ++-------
>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>> index e0fb65e90af2..2058641ab9f6 100644
>> --- a/net/sunrpc/svcsock.c
>> +++ b/net/sunrpc/svcsock.c
>> @@ -885,13 +885,8 @@ static struct svc_xprt *svc_tcp_accept(struct svc_xprt *xprt)
>> clear_bit(XPT_CONN, &svsk->sk_xprt.xpt_flags);
>> err = kernel_accept(sock, &newsock, O_NONBLOCK);
>> if (err < 0) {
>> - if (err == -ENOMEM)
>> - printk(KERN_WARNING "%s: no more sockets!\n",
>> - serv->sv_name);
>> - else if (err != -EAGAIN)
>> - net_warn_ratelimited("%s: accept failed (err %d)!\n",
>> - serv->sv_name, -err);
>> - trace_svcsock_accept_err(xprt, serv->sv_name, err);
>> + if (err != -EAGAIN)
>> + trace_svcsock_accept_err(xprt, serv->sv_name, err);
>
> Would this be better done as a TP_CONDITION tracepoint?

I looked at doing that.

svcsock_accept_err is in a declared class. I would need to
split that into two trace events, one CONDITIONAL and the
other not. svc_tcp_accept() is not a hot path, so it really
isn't worth a whole lot of churn.

Let me know if there's some benefit aside from a bit of
code cleanliness that I forgot.


>> return NULL;
>> }
>> set_bit(XPT_CONN, &svsk->sk_xprt.xpt_flags);
>>
>>
>
> --
> Jeff Layton <[email protected]>


--
Chuck Lever