Re: [PATCH 1/4] sunrpc: add a rcu_head to svc_rqst and use kfree_rcu to free it

["J. Bruce Fields" <bfields@fieldses.org>]

On Fri, Nov 21, 2014 at 02:19:28PM -0500, Jeff Layton wrote:
> ...also make the manipulation of sp_all_threads list use RCU-friendly
> functions.
> 
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> Tested-by: Chris Worley <chris.worley@primarydata.com>
> ---
>  include/linux/sunrpc/svc.h    |  2 ++
>  include/trace/events/sunrpc.h |  3 ++-
>  net/sunrpc/svc.c              | 10 ++++++----
>  3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 5f0ab39bf7c3..7f80a99c59e4 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -223,6 +223,7 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
>  struct svc_rqst {
>  	struct list_head	rq_list;	/* idle list */
>  	struct list_head	rq_all;		/* all threads list */
> +	struct rcu_head		rq_rcu_head;	/* for RCU deferred kfree */
>  	struct svc_xprt *	rq_xprt;	/* transport ptr */
>  
>  	struct sockaddr_storage	rq_addr;	/* peer address */
> @@ -262,6 +263,7 @@ struct svc_rqst {
>  #define	RQ_SPLICE_OK	(4)			/* turned off in gss privacy
>  						 * to prevent encrypting page
>  						 * cache pages */
> +#define	RQ_VICTIM	(5)			/* about to be shut down */
>  	unsigned long		rq_flags;	/* flags field */
>  
>  	void *			rq_argp;	/* decoded arguments */
> diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
> index 5848fc235869..08a5fed50f34 100644
> --- a/include/trace/events/sunrpc.h
> +++ b/include/trace/events/sunrpc.h
> @@ -418,7 +418,8 @@ TRACE_EVENT(xs_tcp_data_recv,
>  		{ (1UL << RQ_LOCAL),		"RQ_LOCAL"},		\
>  		{ (1UL << RQ_USEDEFERRAL),	"RQ_USEDEFERRAL"},	\
>  		{ (1UL << RQ_DROPME),		"RQ_DROPME"},		\
> -		{ (1UL << RQ_SPLICE_OK),	"RQ_SPLICE_OK"})
> +		{ (1UL << RQ_SPLICE_OK),	"RQ_SPLICE_OK"},	\
> +		{ (1UL << RQ_VICTIM),		"RQ_VICTIM"})
>  
>  TRACE_EVENT(svc_recv,
>  	TP_PROTO(struct svc_rqst *rqst, int status),
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 5d9a443d21f6..4edef32f3b9f 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -616,7 +616,7 @@ svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node)
>  	serv->sv_nrthreads++;
>  	spin_lock_bh(&pool->sp_lock);
>  	pool->sp_nrthreads++;
> -	list_add(&rqstp->rq_all, &pool->sp_all_threads);
> +	list_add_rcu(&rqstp->rq_all, &pool->sp_all_threads);
>  	spin_unlock_bh(&pool->sp_lock);
>  	rqstp->rq_server = serv;
>  	rqstp->rq_pool = pool;
> @@ -684,7 +684,8 @@ found_pool:
>  		 * so we don't try to kill it again.
>  		 */
>  		rqstp = list_entry(pool->sp_all_threads.next, struct svc_rqst, rq_all);
> -		list_del_init(&rqstp->rq_all);
> +		set_bit(RQ_VICTIM, &rqstp->rq_flags);
> +		list_del_rcu(&rqstp->rq_all);
>  		task = rqstp->rq_task;
>  	}
>  	spin_unlock_bh(&pool->sp_lock);
> @@ -782,10 +783,11 @@ svc_exit_thread(struct svc_rqst *rqstp)
>  
>  	spin_lock_bh(&pool->sp_lock);
>  	pool->sp_nrthreads--;
> -	list_del(&rqstp->rq_all);
> +	if (!test_and_set_bit(RQ_VICTIM, &rqstp->rq_flags))
> +		list_del_rcu(&rqstp->rq_all);

Both users of RQ_VICTIM are under the sp_lock, so we don't really need
an atomic test_and_set_bit, do we?

But I guess svc_exit_thread probably still needs to check for the case
where it's called on a thread that svc_set_num_threads has already
chosen, and this works even if it's overkill.  OK, fine.

--b.

>  	spin_unlock_bh(&pool->sp_lock);
>  
> -	kfree(rqstp);
> +	kfree_rcu(rqstp, rq_rcu_head);
>  
>  	/* Release the server */
>  	if (serv)
> -- 
> 2.1.0
>