Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Aug 2023 15:00:27 -0400
From:   Chuck Lever <chuck.lever@oracle.com>
To:     NeilBrown <neilb@suse.de>
Cc:     Jeff Layton <jlayton@kernel.org>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 2/6] SUNRPC: rename and refactor svc_get_next_xprt()
Message-ID: <ZMv5S7k4iCQgYXZ4@tissot.1015granger.net>
References: <20230802073443.17965-1-neilb@suse.de>
 <20230802073443.17965-3-neilb@suse.de>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230802073443.17965-3-neilb@suse.de>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ewkWpMH0+K3K/K1QhlpJ9TkelLhbAid4p1TPQAnRwHopzQLwcMoOeNt1Sbr4?=
 =?us-ascii?Q?VfP/rYmX42ZbkbjQNRwS/WXCHFjvjv3zJNzGf57mfXbPZGxA5TnOJBSsyoWM?=
 =?us-ascii?Q?qOw6ZkY4fY5dpRZLeFNw01PMNK7uIAX+MJTMBzSp7lWAGqd2FxgRGv0CuYez?=
 =?us-ascii?Q?VVa/0IxV9fT2NqBo7YxH9bOMJ7kWSSpwVmIQSv90xOQiZuHjRQvttuAqw6du?=
 =?us-ascii?Q?/r4xVnw79Gy5I1jhULp8WrdJTQFcMieQWU62SQK1UwA2yUPwOSt1UPdfuVv9?=
 =?us-ascii?Q?FxjI7O6pnxWINRgFKCeShvEF6kHwbXho88XUihl2HbEpW5JtAHNO18Vijum0?=
 =?us-ascii?Q?ld5a3/Kuis5SYnOKOkpF6Dq5whKoAdfFiREkva4k31K9/0+rfQ7J3RqOyBwp?=
 =?us-ascii?Q?9/CbIt3LNeeJAgV6FSwZSteZDAXGmNI+/sBuDT6UPB/6tUoyLZL2TzM/EE82?=
 =?us-ascii?Q?nOkT+QoKJBpBq5McM4SwlpOrtEYVAYpexHHHUHvo04dqODB8R2IjelxOiW29?=
 =?us-ascii?Q?Y6hvmPsrwjdY5aSV++NHkU92IGuwMkFCUmc5dzYKAqTXr441aegRCeEJrn8P?=
 =?us-ascii?Q?NHIzyyvKXGE9P21iazZT9uCYaAl/TiFWb4sdDeGjs6jkGlbFBbbYduDoqK6G?=
 =?us-ascii?Q?s/eDfgc9AxmqRYT4twuup0gVjIrLUjvw3WTYdTGsQ0gbH/vJZvFNtJNUX90q?=
 =?us-ascii?Q?5osvAkxf9FoOV0T5LmzRI5fvfFSJiejQN3bmqgvYahTjN5LDoSkNzGgpIJDU?=
 =?us-ascii?Q?X2gY2RboyZwZmVSg9BnfqF9QJhLfzRnzN//4cwNZGhqIxWfdV4XSCxsdTG7p?=
 =?us-ascii?Q?xZ9MFIUD8e/s1Qeoi0VI3Brv5cNH4IDZCQ/7LMwtqiQwy82sENZiZ3fNHbtj?=
 =?us-ascii?Q?KnQSgmYW0wiBVFR7aWjiug0H2lLfgT8xJ9MiKhWO4lDS+gQ5VAwsnRZ0jsXs?=
 =?us-ascii?Q?dH9lQ0lchkXH1E6KNOD+vfwTpZDfP77lUUJ2u1eHxqSA5kcoRqYUueRgqh+I?=
 =?us-ascii?Q?u3sPt4/slKlJpIlCGPAbacIvnnvPlJP7D6iQ4ctDfHKrGHZPlDpH/I2hsI4g?=
 =?us-ascii?Q?MyzWXFO09i6UH/M4rRa4xZqBv7IyGO0eYZ0ZuieA96GKES1phMQucdnvMr7Y?=
 =?us-ascii?Q?McTTJABf5G0IcJ9dqpUA/V1j50Mtt2vmquzcWKd2HZlvqQTzB5TJYJAgX2op?=
 =?us-ascii?Q?DO8RLxhonJxmcpjNh4/U7QH+f3aQbZX68KJngC/VqJwxDychbdmpGZRmJlv9?=
 =?us-ascii?Q?p08XwtslXr+rDRH/S+GDJnaC2UaurfW/854YnBUFNTmluJCYXHVUzRn545ry?=
 =?us-ascii?Q?y0X6Ig96CorXLvVmSGHF06Fu/jLBBo4sL1yh3KW6pL8rddN+e8MsRootiGLM?=
 =?us-ascii?Q?LMPPgnXOE/Z9+0ApFapHjfZGObkSzurAlTDsvERiprJspqXw4/wOmfyH3m4C?=
 =?us-ascii?Q?hnfGXtDUuBVbVtxWNXX9kBKuz5vHWggODqQGh0ImoWIGtGeg3Yn7Ovcs8efL?=
 =?us-ascii?Q?P6TjPI9qXI7bl5cJ7bEy7H78rpTgzCKm9ueBcjm2cXGszf3C3aExMLWHFkUO?=
 =?us-ascii?Q?pZ4Fr0F/VuChjzFxksBOb9VRMG+EmdnQwwrfaMXoj8vKXpo4y9Mo2HqtP/V0?=
 =?us-ascii?Q?Sw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: gdB1GNgLXolYU7NHkJlagIKsFgF/OagB6FWKZFKk9PRBmFsTsAe7bduaJjOxTyncSIsNMno58NsireHrHbebw1kwmSXbT2gzWR0Ton1OUkMTvgyqmYLvOuCzGBoUQFQb8gLFMjavDhEPeYB/JxTpX1kzKLj8cTm0LpdfolTuesT+64RM0eL5Hl0qyPM+vjrnsrkyy85xl+09POboCCzoZUsMeRYVEkovBjGx6LqxYsRb1AARZpDxEiBCL2KOf9F98df1jf/hxoRCYTFFJAcS60NLLwb0PcbQowwHAjKO+sw/dVU3shaESQjvlN5sySlsjEQjDbdLb7+lVTgK6PcrkjZ565WRQMYYXV3ZpOcUsh/jmqqjFvLU7vwt9bH0Z1A8v6hw6ZpWOkGD2uyraGVSzstW7zXkpHJQNJVBIXnrMZhyl8SOLicaBqdPY7Te1D7bps9F2QF/an7+RxNpVmnjx09ujLvwCDj/VT5S3OpMySK9hOairwsFUvfB4wzGFsnSix5F+AN/d7kR4wRxhXXY3BkVtbCuJ+hVa0Xc5WrC4pS6q0UCzIFZ8qZPPXShh0VciY0qyTPixTjM6ZDSZJUAGn8+n8mVxSuFDDoaEAzg2LD4j37nzxQB4w7xK0Ym8fMv/IwxNiwH3RaDQyX2469E/vMjdiKBWwac96OUJjkJCCRsTskIzRdZD3jf89n/S6kdRTMdrVeACB1MAUIPXlzeTMwCbuHu2N9ij4EoqFUZLRS1jF261j9F/eILGKCcXkkZroVcjBCMd2ubUWfXtvEwurBsrp+6SZTvGb3HA3x7qF7iuTfitpAEMbJafmyE29YP
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4afdef45-259f-4af0-9973-08db9453eba4
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Aug 2023 19:00:36.1151
 (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: DsfpCFY1y5YbWR3udHmtXEP+knu9v9G2Pc8SLOBbUOFv3jOMVMEN1hEJp3hh+ejAQImCRZheSGca+MrBAyY8lA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB7085
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-08-03_20,2023-08-03_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 suspectscore=0
 mlxlogscore=707 malwarescore=0 mlxscore=0 adultscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2306200000
 definitions=main-2308030171
X-Proofpoint-ORIG-GUID: PN5xsHqvtwbS6hYnOvt2Q7G4XWP_H3wY
X-Proofpoint-GUID: PN5xsHqvtwbS6hYnOvt2Q7G4XWP_H3wY
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org

On Wed, Aug 02, 2023 at 05:34:39PM +1000, NeilBrown wrote:
> svc_get_next_xprt() does a lot more than just get an xprt.  It also
> decides if it needs to sleep, depending not only on the availability of
> xprts but also on the need to exit or handle external work.
> 
> So rename it to svc_rqst_wait_for_work() and only do the testing and
> waiting.  Move all the waiting-related code out of svc_recv() into the
> new svc_rqst_wait_for_work().
> 
> Move the dequeueing code out of svc_get_next_xprt() into svc_recv().
> 
> Previously svc_xprt_dequeue() would be called twice, once before waiting
> and possibly once after.  Now instead rqst_should_sleep() is called
> twice.  Once to decide if waiting is needed, and once to check against
> after setting the task state do see if we might have missed a wakeup.
> 
> signed-off-by: NeilBrown <neilb@suse.de>

I've tested and applied this one and the previous one to the thread
scheduling branch, with a few minor fix-ups. Apologies for how long
this took, I'm wrestling with a SATA/block bug on the v6.6 test
system that is being very sassy and hard to nail down.

I need to dive into the backchannel patch next. I'm trying to think
of how I want to test that one.


> ---
>  net/sunrpc/svc_xprt.c | 91 ++++++++++++++++++++-----------------------
>  1 file changed, 43 insertions(+), 48 deletions(-)
> 
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 60759647fee4..f1d64ded89fb 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -722,51 +722,33 @@ rqst_should_sleep(struct svc_rqst *rqstp)
>  	return true;
>  }
>  
> -static struct svc_xprt *svc_get_next_xprt(struct svc_rqst *rqstp)
> +static void svc_rqst_wait_for_work(struct svc_rqst *rqstp)
>  {
> -	struct svc_pool		*pool = rqstp->rq_pool;
> -
> -	/* rq_xprt should be clear on entry */
> -	WARN_ON_ONCE(rqstp->rq_xprt);
> +	struct svc_pool *pool = rqstp->rq_pool;
>  
> -	rqstp->rq_xprt = svc_xprt_dequeue(pool);
> -	if (rqstp->rq_xprt)
> -		goto out_found;
> -
> -	set_current_state(TASK_IDLE);
> -	smp_mb__before_atomic();
> -	clear_bit(SP_CONGESTED, &pool->sp_flags);
> -	clear_bit(RQ_BUSY, &rqstp->rq_flags);
> -	smp_mb__after_atomic();
> -
> -	if (likely(rqst_should_sleep(rqstp)))
> -		schedule();
> -	else
> -		__set_current_state(TASK_RUNNING);
> +	if (rqst_should_sleep(rqstp)) {
> +		set_current_state(TASK_IDLE);
> +		smp_mb__before_atomic();
> +		clear_bit(SP_CONGESTED, &pool->sp_flags);
> +		clear_bit(RQ_BUSY, &rqstp->rq_flags);
> +		smp_mb__after_atomic();
> +
> +		/* Need to check should_sleep() again after
> +		 * setting task state in case a wakeup happened
> +		 * between testing and setting.
> +		 */
> +		if (rqst_should_sleep(rqstp)) {
> +			schedule();
> +		} else {
> +			__set_current_state(TASK_RUNNING);
> +			cond_resched();
> +		}
>  
> +		set_bit(RQ_BUSY, &rqstp->rq_flags);
> +		smp_mb__after_atomic();
> +	} else
> +		cond_resched();
>  	try_to_freeze();
> -
> -	set_bit(RQ_BUSY, &rqstp->rq_flags);
> -	smp_mb__after_atomic();
> -	clear_bit(SP_TASK_PENDING, &pool->sp_flags);
> -	rqstp->rq_xprt = svc_xprt_dequeue(pool);
> -	if (rqstp->rq_xprt)
> -		goto out_found;
> -
> -	if (kthread_should_stop())
> -		return NULL;
> -	return NULL;
> -out_found:
> -	clear_bit(SP_TASK_PENDING, &pool->sp_flags);
> -	/* Normally we will wait up to 5 seconds for any required
> -	 * cache information to be provided.
> -	 */
> -	if (!test_bit(SP_CONGESTED, &pool->sp_flags))
> -		rqstp->rq_chandle.thread_wait = 5*HZ;
> -	else
> -		rqstp->rq_chandle.thread_wait = 1*HZ;
> -	trace_svc_xprt_dequeue(rqstp);
> -	return rqstp->rq_xprt;
>  }
>  
>  static void svc_add_new_temp_xprt(struct svc_serv *serv, struct svc_xprt *newxpt)
> @@ -858,20 +840,33 @@ static void svc_handle_xprt(struct svc_rqst *rqstp, struct svc_xprt *xprt)
>   */
>  void svc_recv(struct svc_rqst *rqstp)
>  {
> -	struct svc_xprt		*xprt = NULL;
> +	struct svc_pool *pool = rqstp->rq_pool;
>  
>  	if (!svc_alloc_arg(rqstp))
>  		return;
>  
> -	try_to_freeze();
> -	cond_resched();
> +	svc_rqst_wait_for_work(rqstp);
> +
> +	clear_bit(SP_TASK_PENDING, &pool->sp_flags);
> +
>  	if (kthread_should_stop())
> -		goto out;
> +		return;
> +
> +	rqstp->rq_xprt = svc_xprt_dequeue(pool);
> +	if (rqstp->rq_xprt) {
> +		struct svc_xprt *xprt = rqstp->rq_xprt;
> +
> +		/* Normally we will wait up to 5 seconds for any required
> +		 * cache information to be provided.
> +		 */
> +		if (test_bit(SP_CONGESTED, &pool->sp_flags))
> +			rqstp->rq_chandle.thread_wait = 5 * HZ;
> +		else
> +			rqstp->rq_chandle.thread_wait = 1 * HZ;
>  
> -	xprt = svc_get_next_xprt(rqstp);
> -	if (xprt)
> +		trace_svc_xprt_dequeue(rqstp);
>  		svc_handle_xprt(rqstp, xprt);
> -out:
> +	}
>  }
>  EXPORT_SYMBOL_GPL(svc_recv);
>  
> -- 
> 2.40.1
> 

-- 
Chuck Lever