Re: [PATCH] SUNRPC: use assign_bit() where applicable

[Peng Fan <peng.fan@oss.nxp.com>]

On Mon, Sep 21, 2026 at 09:46:05AM -0400, Jeff Layton wrote:
>On Mon, 2026-09-21 at 08:51 +0800, Peng Fan wrote:
>> On Sun, Sep 20, 2026 at 01:22:00PM -0400, Chuck Lever wrote:
>> > 
>> > 
>> > On Sat, Sep 19, 2026, at 10:27 PM, Peng Fan (OSS) wrote:
>> > > From: Peng Fan <peng.fan@nxp.com>
>> > > 
>> > > Convert open-coded if/else with set_bit/clear_bit the assign_bit API.
>> > 
>> > The above sentence explains the same thing that the diff body shows me,
>> > so it does not add value.
>> > 
>> > But I don't have any context here: why is this being done? Is there some
>> > kind of tree-wide clean-up underway so that a new feature can be added,
>> > or is this patch just a one-off change?
>> 
>> It is just a one-off change. I don't group other patches to use assign_bit()
>> into a large patchset, just separate patches.
>> 
>> Free to drop if it does not make sense to you.
>> 
>> 
>
>I think the problem is that you haven't explained the benefit of using
>assign_bit() here. It's now 2 lines instead of 4, but that doesn't seem
>like a substantive change. Does this create better assembly or
>something? 
>
>Without a clear benefit, this seems like pointless churn.

commit 5259401ef8f4b ("bitops: let the compiler optimize {__,}assign_bit()")
may give some information:
"
    Atomic assign_bit() is less affected due to its nature, but let's
    convert it to a macro as well to keep the code consistent and not
    leave a place for possible suboptimal codegen. Moreover, with certain
    kernel configuration it actually gives some saves (x86):

    do_ip_setsockopt    4154    4099     -55
"

I did not do such build test to compare vmlinux size.

If you need me to do a image comparation w/o this patch, I could setup a build
to see whether there are size reduction for the functions that this patch
touched and update commit log in V2. Or we just drop this patch. Both fine
to me.

Thanks
Peng

>
>> > Including a URL that points to an explainer, or making this patch part
>> > of a series would help orient reviewers.
>> > 
>> > 
>> > > Done with Coccinelle semantic patch:
>> > >     // set_bit -> clear_bit => assign_bit
>> > > 
>> > >     @@
>> > >     expression cond, bit, addr;
>> > >     @@
>> > > 
>> > >     -if (cond)
>> > >     -        set_bit(bit, addr);
>> > >     -else
>> > >     -        clear_bit(bit, addr);
>> > >     +assign_bit(bit, addr, cond);
>> > > 
>> > >     @@
>> > >     expression cond, bit, addr;
>> > >     @@
>> > > 
>> > >     -if (cond)
>> > >     -        clear_bit(bit, addr);
>> > >     -else
>> > >     -        set_bit(bit, addr);
>> > >     +assign_bit(bit, addr, !cond);
>> > > 
>> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> > > ---
>> > >  net/sunrpc/svcsock.c | 18 ++++++------------
>> > >  1 file changed, 6 insertions(+), 12 deletions(-)
>> > > 
>> > > diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>> > > index ef7ac080fcd3..d7fa0d1de3ef 100644
>> > > --- a/net/sunrpc/svcsock.c
>> > > +++ b/net/sunrpc/svcsock.c
>> > > @@ -352,10 +352,8 @@ static void svc_sock_setbufsize(struct svc_sock 
>> > > *svsk, unsigned int nreqs)
>> > > 
>> > >  static void svc_sock_secure_port(struct svc_rqst *rqstp)
>> > >  {
>> > > -	if (svc_port_is_privileged(svc_addr(rqstp)))
>> > > -		set_bit(RQ_SECURE, &rqstp->rq_flags);
>> > > -	else
>> > > -		clear_bit(RQ_SECURE, &rqstp->rq_flags);
>> > > +	assign_bit(RQ_SECURE, &rqstp->rq_flags,
>> > > +		   svc_port_is_privileged(svc_addr(rqstp)));
>> > >  }
>> > > 
>> > >  /*
>> > > @@ -941,10 +939,8 @@ static struct svc_xprt *svc_tcp_accept(struct 
>> > > svc_xprt *xprt)
>> > >  		slen = offsetof(struct sockaddr, sa_data);
>> > >  	svc_xprt_set_local(&newsvsk->sk_xprt, sin, slen);
>> > > 
>> > > -	if (sock_is_loopback(newsock->sk))
>> > > -		set_bit(XPT_LOCAL, &newsvsk->sk_xprt.xpt_flags);
>> > > -	else
>> > > -		clear_bit(XPT_LOCAL, &newsvsk->sk_xprt.xpt_flags);
>> > > +	assign_bit(XPT_LOCAL, &newsvsk->sk_xprt.xpt_flags,
>> > > +		   sock_is_loopback(newsock->sk));
>> > >  	if (serv->sv_stats)
>> > >  		serv->sv_stats->nettcpconn++;
>> > > 
>> > > @@ -1290,10 +1286,8 @@ static int svc_tcp_recvfrom(struct svc_rqst *rqstp)
>> > > 
>> > >  	rqstp->rq_xprt_ctxt   = NULL;
>> > >  	rqstp->rq_prot	      = IPPROTO_TCP;
>> > > -	if (test_bit(XPT_LOCAL, &svsk->sk_xprt.xpt_flags))
>> > > -		set_bit(RQ_LOCAL, &rqstp->rq_flags);
>> > > -	else
>> > > -		clear_bit(RQ_LOCAL, &rqstp->rq_flags);
>> > > +	assign_bit(RQ_LOCAL, &rqstp->rq_flags,
>> > > +		   test_bit(XPT_LOCAL, &svsk->sk_xprt.xpt_flags));
>> > > 
>> > >  	/* Completing one message stops ->read_sock with whatever
>> > >  	 * follows still queued, and no path from here re-arms XPT_DATA.
>> > > -- 
>> > > 2.51.0
>> > 
>> > -- 
>> > Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)
>> > 
>> > 
>
>-- 
>Jeff Layton <jlayton@kernel.org>
>
>