Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Alexander Duyck <alexander.duyck@gmail.com>,
        Mel Gorman <mgorman@techsingularity.net>
CC:     Andrew Morton <akpm@linux-foundation.org>,
        Jesper Dangaard Brouer <brouer@redhat.com>,
        Christoph Hellwig <hch@infradead.org>,
        Matthew Wilcox <willy@infradead.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux-Net <netdev@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 5/7] SUNRPC: Refresh rq_pages using a bulk page allocator
Thread-Topic: [PATCH 5/7] SUNRPC: Refresh rq_pages using a bulk page allocator
Thread-Index: AQHXF1tVdfZNbUNlaEOVbp5QB1YPQaqAsKcAgAAKxwA=
Date:   Fri, 12 Mar 2021 19:22:43 +0000
Message-ID: <17DF335E-9194-4A16-B18E-668AD1362697@oracle.com>
References: <20210312154331.32229-1-mgorman@techsingularity.net>
 <20210312154331.32229-6-mgorman@techsingularity.net>
 <CAKgT0Uf-4CY=wU079Y87xwzz_UDm8AqGBdt_6OuVtADR8AN0hA@mail.gmail.com>
In-Reply-To: <CAKgT0Uf-4CY=wU079Y87xwzz_UDm8AqGBdt_6OuVtADR8AN0hA@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <680E2CC1CA140141BBEF26EAF6B563E1@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 17d4aea3-6277-4c5d-6921-08d8e58c360a
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Mar 2021 19:22:43.3816
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ZZGNfIa2ZFnVA0JPfyZm0qKWHhF2WFXBTa9bq8S61+M82ePBZ+wjLVg8C2PIHq+vqR3fkMUbLPP/CM6ksnsS4w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3383
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9921 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxscore=0 bulkscore=0
 adultscore=0 phishscore=0 spamscore=0 mlxlogscore=999 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2103120143
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9921 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 lowpriorityscore=0
 impostorscore=0 mlxlogscore=999 malwarescore=0 suspectscore=0 adultscore=0
 phishscore=0 spamscore=0 priorityscore=1501 bulkscore=0 clxscore=1011
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2103120143
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org

Mel, I can send you a tidied and tested update to this patch,
or you can drop the two NFSD patches and I can submit them via
the NFSD tree when alloc_pages_bulk() is merged.

> On Mar 12, 2021, at 1:44 PM, Alexander Duyck <alexander.duyck@gmail.com> =
wrote:
>=20
> On Fri, Mar 12, 2021 at 7:43 AM Mel Gorman <mgorman@techsingularity.net> =
wrote:
>>=20
>> From: Chuck Lever <chuck.lever@oracle.com>
>>=20
>> Reduce the rate at which nfsd threads hammer on the page allocator.
>> This improves throughput scalability by enabling the threads to run
>> more independently of each other.
>>=20
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
>> ---
>> net/sunrpc/svc_xprt.c | 43 +++++++++++++++++++++++++++++++------------
>> 1 file changed, 31 insertions(+), 12 deletions(-)
>>=20
>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>> index cfa7e4776d0e..38a8d6283801 100644
>> --- a/net/sunrpc/svc_xprt.c
>> +++ b/net/sunrpc/svc_xprt.c
>> @@ -642,11 +642,12 @@ static void svc_check_conn_limits(struct svc_serv =
*serv)
>> static int svc_alloc_arg(struct svc_rqst *rqstp)
>> {
>>        struct svc_serv *serv =3D rqstp->rq_server;
>> +       unsigned long needed;
>>        struct xdr_buf *arg;
>> +       struct page *page;
>>        int pages;
>>        int i;
>>=20
>> -       /* now allocate needed pages.  If we get a failure, sleep briefl=
y */
>>        pages =3D (serv->sv_max_mesg + 2 * PAGE_SIZE) >> PAGE_SHIFT;
>>        if (pages > RPCSVC_MAXPAGES) {
>>                pr_warn_once("svc: warning: pages=3D%u > RPCSVC_MAXPAGES=
=3D%lu\n",
>> @@ -654,19 +655,28 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
>>                /* use as many pages as possible */
>>                pages =3D RPCSVC_MAXPAGES;
>>        }
>> -       for (i =3D 0; i < pages ; i++)
>> -               while (rqstp->rq_pages[i] =3D=3D NULL) {
>> -                       struct page *p =3D alloc_page(GFP_KERNEL);
>> -                       if (!p) {
>> -                               set_current_state(TASK_INTERRUPTIBLE);
>> -                               if (signalled() || kthread_should_stop()=
) {
>> -                                       set_current_state(TASK_RUNNING);
>> -                                       return -EINTR;
>> -                               }
>> -                               schedule_timeout(msecs_to_jiffies(500));
>> +
>=20
>> +       for (needed =3D 0, i =3D 0; i < pages ; i++)
>> +               if (!rqstp->rq_pages[i])
>> +                       needed++;
>=20
> I would use an opening and closing braces for the for loop since
> technically the if is a multiline statement. It will make this more
> readable.
>=20
>> +       if (needed) {
>> +               LIST_HEAD(list);
>> +
>> +retry:
>=20
> Rather than kind of open code a while loop why not just make this
> "while (needed)"? Then all you have to do is break out of the for loop
> and you will automatically return here instead of having to jump to
> two different labels.
>=20
>> +               alloc_pages_bulk(GFP_KERNEL, needed, &list);
>=20
> Rather than not using the return value would it make sense here to
> perhaps subtract it from needed? Then you would know if any of the
> allocation requests weren't fulfilled.
>=20
>> +               for (i =3D 0; i < pages; i++) {
>=20
> It is probably optimizing for the exception case, but I don't think
> you want the "i =3D 0" here. If you are having to stop because the list
> is empty it probably makes sense to resume where you left off. So you
> should probably be initializing i to 0 before we check for needed.
>=20
>> +                       if (!rqstp->rq_pages[i]) {
>=20
> It might be cleaner here to just do a "continue" if rq_pages[i] is popula=
ted.
>=20
>> +                               page =3D list_first_entry_or_null(&list,
>> +                                                               struct p=
age,
>> +                                                               lru);
>> +                               if (unlikely(!page))
>> +                                       goto empty_list;
>=20
> I think I preferred the original code that wasn't jumping away from
> the loop here. With the change I suggested above that would switch the
> if(needed) to while(needed) you could have it just break out of the
> for loop to place itself back in the while loop.
>=20
>> +                               list_del(&page->lru);
>> +                               rqstp->rq_pages[i] =3D page;
>> +                               needed--;
>>                        }
>> -                       rqstp->rq_pages[i] =3D p;
>>                }
>> +       }
>>        rqstp->rq_page_end =3D &rqstp->rq_pages[pages];
>>        rqstp->rq_pages[pages] =3D NULL; /* this might be seen in nfsd_sp=
lice_actor() */
>>=20
>> @@ -681,6 +691,15 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
>>        arg->len =3D (pages-1)*PAGE_SIZE;
>>        arg->tail[0].iov_len =3D 0;
>>        return 0;
>> +
>> +empty_list:
>> +       set_current_state(TASK_INTERRUPTIBLE);
>> +       if (signalled() || kthread_should_stop()) {
>> +               set_current_state(TASK_RUNNING);
>> +               return -EINTR;
>> +       }
>> +       schedule_timeout(msecs_to_jiffies(500));
>> +       goto retry;
>> }
>>=20
>> static bool
>> --
>> 2.26.2

--
Chuck Lever