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

[Jeff Layton]

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

> > 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@xxxxxxx>
> > > ---
> > >  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@xxxxxxxxxx>