Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\))
Subject: Re: [PATCH RFC] SUNRPC: Avoid digging into the ATOMIC pool
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <ddaf320c5c7a4e90f7a1b034fbbabf9bba899fea.camel@hammerspace.com>
Date:   Mon, 4 Mar 2019 11:05:17 -0500
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <E8DA13E4-5164-4092-A695-B9F11E6D28F9@oracle.com>
References: <20190304153222.27638.90558.stgit@manet.1015granger.net>
 <ddaf320c5c7a4e90f7a1b034fbbabf9bba899fea.camel@hammerspace.com>
To:     Trond Myklebust <trondmy@hammerspace.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Mar 4, 2019, at 11:02 AM, Trond Myklebust <trondmy@hammerspace.com> =
wrote:
>=20
> On Mon, 2019-03-04 at 10:32 -0500, Chuck Lever wrote:
>> Page allocation requests made when the SPARSE_PAGES flag is set are
>> allowed to fail, and are not critical. No need to spend a rare
>> resource.
>>=20
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> net/sunrpc/socklib.c           |    2 +-
>> net/sunrpc/xprtrdma/rpc_rdma.c |    2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>=20
>> diff --git a/net/sunrpc/socklib.c b/net/sunrpc/socklib.c
>> index 7e55cfc..9faea12 100644
>> --- a/net/sunrpc/socklib.c
>> +++ b/net/sunrpc/socklib.c
>> @@ -106,7 +106,7 @@ static size_t xdr_skb_read_and_csum_bits(struct
>> xdr_skb_reader *desc, void *to,
>> 		/* ACL likes to be lazy in allocating pages - ACLs
>> 		 * are small by default but can get huge. */
>> 		if ((xdr->flags & XDRBUF_SPARSE_PAGES) && *ppage =3D=3D
>> NULL) {
>> -			*ppage =3D alloc_page(GFP_ATOMIC);
>> +			*ppage =3D alloc_page(GFP_NOWAIT | =
__GFP_NOWARN);
>> 			if (unlikely(*ppage =3D=3D NULL)) {
>> 				if (copied =3D=3D 0)
>> 					copied =3D -ENOMEM;
>=20
> Hmm... Can't we make this GFP_KERNEL? I can't see that we're holding
> any locks here.

We're holding the transport send lock. Since this is an order 0 =
allocation
it's unlikely to fail even with a less aggressive request like NOWAIT.


> It does look like svc_udp_recvfrom() is wrapping the
> call to csum_partial_copy_to_xdr() in a local_bh_disable()/enable()
> pair, but is that actually needed? If so, for what?
>=20
>> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c
>> b/net/sunrpc/xprtrdma/rpc_rdma.c
>> index 6c1fb27..b759b16 100644
>> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
>> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
>> @@ -238,7 +238,7 @@ static bool rpcrdma_results_inline(struct
>> rpcrdma_xprt *r_xprt,
>> 		 */
>> 		if (unlikely(xdrbuf->flags & XDRBUF_SPARSE_PAGES)) {
>> 			if (!*ppages)
>> -				*ppages =3D alloc_page(GFP_ATOMIC);
>> +				*ppages =3D alloc_page(GFP_NOWAIT |
>> __GFP_NOWARN);
>> 			if (!*ppages)
>> 				return -ENOBUFS;
>> 		}
>>=20
>=20
> Cheers
>  Trond
> --=20
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com

--
Chuck Lever