Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1472866 > unrolled thread

[PATCH v2] fs/nfsd/nfs4callback: Remove deprecated create_singlethread_workqueue

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue Bhaktipriya Shridhar <bhaktipriya96@gmail.com> - 2016-08-30 23:00 +0200
    Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue Jeff Layton <jlayton@poochiereds.net> - 2016-08-30 23:10 +0200
      Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue Tejun Heo <tj@kernel.org> - 2016-08-31 16:50 +0200
        Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue Jeff Layton <jlayton@redhat.com> - 2016-08-31 17:10 +0200

#1472866 — [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated create_singlethread_workqueue

The workqueue "callback_wq" queues a single work item &cb->cb_work per
nfsd4_callback instance and thus, it doesn't require execution ordering.
Hence, alloc_workqueue has been used to replace the
deprecated create_singlethread_workqueue instance.

The WQ_MEM_RECLAIM flag has not been set since this is an in-kernel nfs
server and isn't involved in memory reclaim operations on the local
host.

Since there are fixed number of work items, explicit concurrency
limit is unnecessary here.

Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
---
 Changes in v2:
	- No change. Made this a separate patch (categorised based on
	  directories).

 fs/nfsd/nfs4callback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 7389cb1..a6611c6 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1021,7 +1021,7 @@ static const struct rpc_call_ops nfsd4_cb_ops = {

 int nfsd4_create_callback_queue(void)
 {
-	callback_wq = create_singlethread_workqueue("nfsd4_callbacks");
+	callback_wq = alloc_workqueue("nfsd4_callbacks", 0, 0);
 	if (!callback_wq)
 		return -ENOMEM;
 	return 0;
--
2.1.4

[toc] | [next] | [standalone]

#1472877

On Wed, 2016-08-31 at 02:23 +0530, Bhaktipriya Shridhar wrote:
> The workqueue "callback_wq" queues a single work item &cb->cb_work
> per
> nfsd4_callback instance and thus, it doesn't require execution
> ordering.
> Hence, alloc_workqueue has been used to replace the
> deprecated create_singlethread_workqueue instance.
> 
> The WQ_MEM_RECLAIM flag has not been set since this is an in-kernel
> nfs
> server and isn't involved in memory reclaim operations on the local
> host.
> 
> Since there are fixed number of work items, explicit concurrency
> limit is unnecessary here.
> 
> Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
> ---
>  Changes in v2:
> 	- No change. Made this a separate patch (categorised based on
> 	  directories).
> 
>  fs/nfsd/nfs4callback.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 7389cb1..a6611c6 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -1021,7 +1021,7 @@ static const struct rpc_call_ops nfsd4_cb_ops =
> {
> 
>  int nfsd4_create_callback_queue(void)
>  {
> -	callback_wq =
> create_singlethread_workqueue("nfsd4_callbacks");
> +	callback_wq = alloc_workqueue("nfsd4_callbacks", 0, 0);
>  	if (!callback_wq)
>  		return -ENOMEM;
>  	return 0;
> --
> 2.1.4
> 

Hah! I have almost exactly the same patch in my tree. I've only not
sent it because I haven't had the chance to test it well.

The only difference in mine is that it passes in WQ_UNBOUND. ISTM that
we don't really need a bound workqueue here since we only use this to
kick off callbacks to the client. I doubt we'd get much out of strictly
maintaining cache locality here, and we're better off just sending it
the callback as quickly as possible.

Thoughts?
-- 
Jeff Layton <jlayton@poochiereds.net>

[toc] | [prev] | [next] | [standalone]

#1473564

Hello, Jeff.

On Tue, Aug 30, 2016 at 05:07:23PM -0400, Jeff Layton wrote:
> Hah! I have almost exactly the same patch in my tree. I've only not
> sent it because I haven't had the chance to test it well.
> 
> The only difference in mine is that it passes in WQ_UNBOUND. ISTM that
> we don't really need a bound workqueue here since we only use this to
> kick off callbacks to the client. I doubt we'd get much out of strictly
> maintaining cache locality here, and we're better off just sending it
> the callback as quickly as possible.

We recently broke strong locality guarantee for users which don't use
queue_work_on(), so the default locality is now only for optimization
instead of correctness anyway.  Unless there are actual benefits to
using WQ_UNBOUND, I think in general it's better to stick with as
little attributes as possible so that we have more maneuvering room
down the line.  But, yeah, if this can impact performance in subtle
ways, it could be best to just do an identity conversion at least for
now.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1473589

On Wed, 2016-08-31 at 10:39 -0400, Tejun Heo wrote:
> Hello, Jeff.
> 
> On Tue, Aug 30, 2016 at 05:07:23PM -0400, Jeff Layton wrote:
> > 
> > Hah! I have almost exactly the same patch in my tree. I've only not
> > sent it because I haven't had the chance to test it well.
> > 
> > The only difference in mine is that it passes in WQ_UNBOUND. ISTM
> > that
> > we don't really need a bound workqueue here since we only use this
> > to
> > kick off callbacks to the client. I doubt we'd get much out of
> > strictly
> > maintaining cache locality here, and we're better off just sending
> > it
> > the callback as quickly as possible.
> 
> We recently broke strong locality guarantee for users which don't use
> queue_work_on(), so the default locality is now only for optimization
> instead of correctness anyway.  Unless there are actual benefits to
> using WQ_UNBOUND, I think in general it's better to stick with as
> little attributes as possible so that we have more maneuvering room
> down the line.  But, yeah, if this can impact performance in subtle
> ways, it could be best to just do an identity conversion at least for
> now.
> 
> Thanks.
> 

Ahh ok, thanks...that's good to know. If you think we don't need
WQ_UNBOUND then this is fine with me.

Reviewed-by: Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web