[RFC 0/1] SUNRPC: dynamic rpc_slot allocator for TCP

[RFC 0/1] SUNRPC: dynamic rpc_slot allocator for TCP

[andros]

I would appreciate comments on this patch. I've done a small amount of testing
where I've set the /proc/sys/sunrpc/tcp_slot_table_entries to 2, and watched
the dynamic allocator increase it to 3. I have a 10G test system being
configured.

Thanks!

-->Andy

[[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[andros]

From: Andy Adamson <andros@netapp.com>

Hookup TCP congestion feedback into rpc_slot allocation so that the RPC layer
can fully utilize the negotiated TCP window.

Use a slab cache for rpc_slots. Statically allocate an rpc_xprt rpc_slot slab
cache using GFP_KERNEL to the RPC_DEF_SLOT_TABLE number of slots at
rpc_xprt allocation.

Add a dynamic rpc slot allocator to rpc_xprt_ops which is set only for TCP.
For TCP, trigger a dyamic slot allocation in response to a write_space
callback which is in turn called when the TCP layer is waiting for buffer space.

Dynamically add a slot at the beginning of the RPC call_transmit state. The slot
allocator uses GFP_NOWAIT and will return without allocating a slot if
GFP_NOWAIT allocation fails. This is OK because the write_space callback will
be called again, and the dynamic slot allocator can retry.

Signed-off-by: Andy Adamson <andros@netap.com>
---
 include/linux/sunrpc/sched.h |    2 +
 include/linux/sunrpc/xprt.h  |    6 +++-
 net/sunrpc/clnt.c            |    4 ++
 net/sunrpc/sched.c           |   39 ++++++++++++++++++++++
 net/sunrpc/xprt.c            |   75 +++++++++++++++++++++++++++++++++++++-----
 net/sunrpc/xprtsock.c        |    1 +
 6 files changed, 117 insertions(+), 10 deletions(-)

diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index d81db80..3202d09 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -242,6 +242,8 @@ int		rpc_init_mempool(void);
 void		rpc_destroy_mempool(void);
 extern struct workqueue_struct *rpciod_workqueue;
 void		rpc_prepare_task(struct rpc_task *task);
+void		rpc_free_slot(struct rpc_rqst *req);
+struct rpc_rqst *rpc_alloc_slot(gfp_t gfp);
 
 static inline int rpc_wait_for_completion_task(struct rpc_task *task)
 {
diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
index a0f998c..ae3682c 100644
--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h
@@ -118,6 +118,7 @@ struct rpc_xprt_ops {
 	void		(*connect)(struct rpc_task *task);
 	void *		(*buf_alloc)(struct rpc_task *task, size_t size);
 	void		(*buf_free)(void *buffer);
+	void		(*dynamic_slot_alloc)(struct rpc_xprt *xprt);
 	int		(*send_request)(struct rpc_task *task);
 	void		(*set_retrans_timeout)(struct rpc_task *task);
 	void		(*timer)(struct rpc_task *task);
@@ -167,7 +168,6 @@ struct rpc_xprt {
 	struct rpc_wait_queue	pending;	/* requests in flight */
 	struct rpc_wait_queue	backlog;	/* waiting for slot */
 	struct list_head	free;		/* free slots */
-	struct rpc_rqst *	slot;		/* slot table storage */
 	unsigned int		max_reqs;	/* total slots */
 	unsigned long		state;		/* transport state */
 	unsigned char		shutdown   : 1,	/* being shut down */
@@ -283,6 +283,9 @@ struct rpc_xprt *	xprt_get(struct rpc_xprt *xprt);
 void			xprt_put(struct rpc_xprt *xprt);
 struct rpc_xprt *	xprt_alloc(struct net *net, int size, int max_req);
 void			xprt_free(struct rpc_xprt *);
+int			xprt_alloc_slot_entries(struct rpc_xprt *xprt,
+						int num_req);
+void			 xprt_add_slot(struct rpc_xprt *xprt);
 
 static inline __be32 *xprt_skip_transport_header(struct rpc_xprt *xprt, __be32 *p)
 {
@@ -321,6 +324,7 @@ void			xprt_conditional_disconnect(struct rpc_xprt *xprt, unsigned int cookie);
 #define XPRT_CONNECTION_ABORT	(7)
 #define XPRT_CONNECTION_CLOSE	(8)
 #define XPRT_INITIALIZED	(9)
+#define XPRT_WRITE_SPACE	(10)
 
 static inline void xprt_set_connected(struct rpc_xprt *xprt)
 {
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index e7a96e4..8e21d27 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1276,6 +1276,10 @@ call_transmit(struct rpc_task *task)
 	task->tk_action = call_status;
 	if (task->tk_status < 0)
 		return;
+
+	if (task->tk_xprt->ops->dynamic_slot_alloc)
+		task->tk_xprt->ops->dynamic_slot_alloc(task->tk_xprt);
+
 	task->tk_status = xprt_prepare_transmit(task);
 	if (task->tk_status != 0)
 		return;
diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index 6b43ee7..bbd4018 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -33,10 +33,13 @@
 #define RPC_BUFFER_MAXSIZE	(2048)
 #define RPC_BUFFER_POOLSIZE	(8)
 #define RPC_TASK_POOLSIZE	(8)
+#define RPC_SLOT_POOLSIZE	(RPC_TASK_POOLSIZE * RPC_DEF_SLOT_TABLE)
 static struct kmem_cache	*rpc_task_slabp __read_mostly;
 static struct kmem_cache	*rpc_buffer_slabp __read_mostly;
+static struct kmem_cache	*rpc_slot_slabp __read_mostly;
 static mempool_t	*rpc_task_mempool __read_mostly;
 static mempool_t	*rpc_buffer_mempool __read_mostly;
+static mempool_t	*rpc_slot_mempool __read_mostly;
 
 static void			rpc_async_schedule(struct work_struct *);
 static void			 rpc_release_task(struct rpc_task *task);
@@ -961,9 +964,33 @@ static void rpciod_stop(void)
 }
 
 void
+rpc_free_slot(struct rpc_rqst *req)
+{
+	return mempool_free(req, rpc_slot_mempool);
+}
+
+/**
+ * rpc_alloc_slot - rpc_slot allocator
+ *
+ * Static rpc_xprt Initialization:
+ *   Called with GFP_KERNEL
+ *
+ * Dynamic allocation:
+ *   Called with GFP_NOWAIT
+ *   Triggered by write_space callback.
+ */
+struct rpc_rqst *
+rpc_alloc_slot(gfp_t gfp)
+{
+	return (struct rpc_rqst *)mempool_alloc(rpc_slot_mempool, gfp);
+}
+
+void
 rpc_destroy_mempool(void)
 {
 	rpciod_stop();
+	if (rpc_slot_mempool)
+		mempool_destroy(rpc_slot_mempool);
 	if (rpc_buffer_mempool)
 		mempool_destroy(rpc_buffer_mempool);
 	if (rpc_task_mempool)
@@ -972,6 +999,8 @@ rpc_destroy_mempool(void)
 		kmem_cache_destroy(rpc_task_slabp);
 	if (rpc_buffer_slabp)
 		kmem_cache_destroy(rpc_buffer_slabp);
+	if (rpc_slot_slabp)
+		kmem_cache_destroy(rpc_slot_slabp);
 	rpc_destroy_wait_queue(&delay_queue);
 }
 
@@ -998,6 +1027,12 @@ rpc_init_mempool(void)
 					     NULL);
 	if (!rpc_buffer_slabp)
 		goto err_nomem;
+	rpc_slot_slabp = kmem_cache_create("rpc_slots",
+					     sizeof(struct rpc_rqst),
+					     0, SLAB_HWCACHE_ALIGN,
+					     NULL);
+	if (!rpc_slot_slabp)
+		goto err_nomem;
 	rpc_task_mempool = mempool_create_slab_pool(RPC_TASK_POOLSIZE,
 						    rpc_task_slabp);
 	if (!rpc_task_mempool)
@@ -1006,6 +1041,10 @@ rpc_init_mempool(void)
 						      rpc_buffer_slabp);
 	if (!rpc_buffer_mempool)
 		goto err_nomem;
+	rpc_slot_mempool = mempool_create_slab_pool(RPC_SLOT_POOLSIZE,
+						    rpc_slot_slabp);
+	if (!rpc_slot_mempool)
+		goto err_nomem;
 	return 0;
 err_nomem:
 	rpc_destroy_mempool();
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index 9494c37..1b0aa55 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -498,6 +498,7 @@ void xprt_write_space(struct rpc_xprt *xprt)
 		dprintk("RPC:       write space: waking waiting task on "
 				"xprt %p\n", xprt);
 		rpc_wake_up_queued_task(&xprt->pending, xprt->snd_task);
+		set_bit(XPRT_WRITE_SPACE, &xprt->state);
 	}
 	spin_unlock_bh(&xprt->transport_lock);
 }
@@ -957,6 +958,66 @@ static void xprt_free_slot(struct rpc_xprt *xprt, struct rpc_rqst *req)
 	spin_unlock(&xprt->reserve_lock);
 }
 
+static void
+xprt_free_slot_entries(struct rpc_xprt *xprt)
+{
+	struct rpc_rqst	*req;
+	int i = 0;
+
+	while (!list_empty(&xprt->free)) {
+		req = list_entry(xprt->free.next, struct rpc_rqst, rq_list);
+		list_del(&req->rq_list);
+		rpc_free_slot(req);
+		i++;
+	}
+	dprintk("<-- %s mempool_free %d reqs\n", __func__, i);
+}
+
+/*
+ * Static transport rpc_slot allocation called only at rpc_xprt allocation.
+ * No need to take the xprt->reserve_lock.
+ */
+int
+xprt_alloc_slot_entries(struct rpc_xprt *xprt, int num_req)
+{
+	struct rpc_rqst *req;
+	int i;
+
+	for (i = 0; i < num_req; i++) {
+		req = rpc_alloc_slot(GFP_KERNEL);
+		if (!req)
+			return -ENOMEM;
+		memset(req, 0, sizeof(*req));
+		list_add(&req->rq_list, &xprt->free);
+	}
+	dprintk("<-- %s mempool_alloc %d reqs\n", __func__,
+		xprt->max_reqs);
+	return 0;
+}
+
+/*
+ * Dynamic rpc_slot allocator. GFP_NOWAIT will not cause rpciod to sleep.
+ * Return NULL if allocation can't be serviced immediately.
+ * Triggered by write_space callback.
+ */
+void
+xprt_add_slot(struct rpc_xprt *xprt)
+{
+	struct rpc_rqst *req;
+
+	if (!test_and_clear_bit(XPRT_WRITE_SPACE, &xprt->state))
+		return;
+	req = rpc_alloc_slot(GFP_NOWAIT);
+	if (!req)
+		return;
+	spin_lock(&xprt->reserve_lock);
+	list_add(&req->rq_list, &xprt->free);
+	xprt->max_reqs += 1;
+	spin_unlock(&xprt->reserve_lock);
+
+	dprintk("RPC	added rpc_slot to transport %p\n", xprt);
+}
+
 struct rpc_xprt *xprt_alloc(struct net *net, int size, int max_req)
 {
 	struct rpc_xprt *xprt;
@@ -967,14 +1028,16 @@ struct rpc_xprt *xprt_alloc(struct net *net, int size, int max_req)
 	atomic_set(&xprt->count, 1);
 
 	xprt->max_reqs = max_req;
-	xprt->slot = kcalloc(max_req, sizeof(struct rpc_rqst), GFP_KERNEL);
-	if (xprt->slot == NULL)
+	/* allocate slots and place on free list */
+	INIT_LIST_HEAD(&xprt->free);
+	if (xprt_alloc_slot_entries(xprt, max_req) != 0)
 		goto out_free;
 
 	xprt->xprt_net = get_net(net);
 	return xprt;
 
 out_free:
+	xprt_free_slot_entries(xprt);
 	kfree(xprt);
 out:
 	return NULL;
@@ -984,7 +1047,7 @@ EXPORT_SYMBOL_GPL(xprt_alloc);
 void xprt_free(struct rpc_xprt *xprt)
 {
 	put_net(xprt->xprt_net);
-	kfree(xprt->slot);
+	xprt_free_slot_entries(xprt);
 	kfree(xprt);
 }
 EXPORT_SYMBOL_GPL(xprt_free);
@@ -1080,7 +1143,6 @@ void xprt_release(struct rpc_task *task)
 struct rpc_xprt *xprt_create_transport(struct xprt_create *args)
 {
 	struct rpc_xprt	*xprt;
-	struct rpc_rqst	*req;
 	struct xprt_class *t;
 
 	spin_lock(&xprt_list_lock);
@@ -1108,7 +1170,6 @@ found:
 	spin_lock_init(&xprt->transport_lock);
 	spin_lock_init(&xprt->reserve_lock);
 
-	INIT_LIST_HEAD(&xprt->free);
 	INIT_LIST_HEAD(&xprt->recv);
 #if defined(CONFIG_NFS_V4_1)
 	spin_lock_init(&xprt->bc_pa_lock);
@@ -1131,10 +1192,6 @@ found:
 	rpc_init_wait_queue(&xprt->resend, "xprt_resend");
 	rpc_init_priority_wait_queue(&xprt->backlog, "xprt_backlog");
 
-	/* initialize free list */
-	for (req = &xprt->slot[xprt->max_reqs-1]; req >= &xprt->slot[0]; req--)
-		list_add(&req->rq_list, &xprt->free);
-
 	xprt_init_xid(xprt);
 
 	dprintk("RPC:       created transport %p with %u slots\n", xprt,
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index bf005d3..8ab2801 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2115,6 +2115,7 @@ static struct rpc_xprt_ops xs_tcp_ops = {
 	.connect		= xs_connect,
 	.buf_alloc		= rpc_malloc,
 	.buf_free		= rpc_free,
+	.dynamic_slot_alloc	= xprt_add_slot,
 	.send_request		= xs_tcp_send_request,
 	.set_retrans_timeout	= xprt_set_retrans_timeout_def,
 	.close			= xs_tcp_close,
-- 
1.7.3.1

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Jeff Layton]

On Mon,  2 May 2011 21:40:08 -0400
andros@netapp.com wrote:

> From: Andy Adamson <andros@netapp.com>
> 
> Hookup TCP congestion feedback into rpc_slot allocation so that the RPC layer
> can fully utilize the negotiated TCP window.
> 
> Use a slab cache for rpc_slots. Statically allocate an rpc_xprt rpc_slot slab
> cache using GFP_KERNEL to the RPC_DEF_SLOT_TABLE number of slots at
> rpc_xprt allocation.
> 
> Add a dynamic rpc slot allocator to rpc_xprt_ops which is set only for TCP.
> For TCP, trigger a dyamic slot allocation in response to a write_space
> callback which is in turn called when the TCP layer is waiting for buffer space.
> 
> Dynamically add a slot at the beginning of the RPC call_transmit state. The slot
> allocator uses GFP_NOWAIT and will return without allocating a slot if
> GFP_NOWAIT allocation fails. This is OK because the write_space callback will
> be called again, and the dynamic slot allocator can retry.
> 
> Signed-off-by: Andy Adamson <andros@netap.com>
> ---
>  include/linux/sunrpc/sched.h |    2 +
>  include/linux/sunrpc/xprt.h  |    6 +++-
>  net/sunrpc/clnt.c            |    4 ++
>  net/sunrpc/sched.c           |   39 ++++++++++++++++++++++
>  net/sunrpc/xprt.c            |   75 +++++++++++++++++++++++++++++++++++++-----
>  net/sunrpc/xprtsock.c        |    1 +
>  6 files changed, 117 insertions(+), 10 deletions(-)
> 

Nice work, comments inline below...


[...]

> +
> +/*
> + * Static transport rpc_slot allocation called only at rpc_xprt allocation.
> + * No need to take the xprt->reserve_lock.
> + */
> +int
> +xprt_alloc_slot_entries(struct rpc_xprt *xprt, int num_req)
> +{
> +	struct rpc_rqst *req;
> +	int i;
> +
> +	for (i = 0; i < num_req; i++) {
> +		req = rpc_alloc_slot(GFP_KERNEL);
> +		if (!req)
> +			return -ENOMEM;
> +		memset(req, 0, sizeof(*req));
> +		list_add(&req->rq_list, &xprt->free);
> +	}
> +	dprintk("<-- %s mempool_alloc %d reqs\n", __func__,
> +		xprt->max_reqs);
> +	return 0;
> +}
> +

So, I don't quite get this...

You declare a global mempool early on, and then allocate from that
mempool for a list of static entries. Doesn't that sort of rob you of
any benefit of using a mempool here? IOW, won't the static allocations
potentially rob the mempool of "guaranteed" entries such that the
dynamic ones eventually all turn into slab allocations anyway?

What I think would make more sense would be to have multiple mempools
-- one per xprt and simply set the mempool size to the number of
"static" entries that you want for the mempool. Then you could get rid
of the free list, and just do allocations out of the mempool directly.
You'll be guaranteed to be able to allocate up to the number in the
mempool and everything above that would just becomes a slab allocation.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Trond Myklebust]

On Tue, 2011-05-03 at 20:20 -0400, Jeff Layton wrote:
> On Mon,  2 May 2011 21:40:08 -0400
> andros@netapp.com wrote:
> 
> > From: Andy Adamson <andros@netapp.com>
> > 
> > Hookup TCP congestion feedback into rpc_slot allocation so that the RPC layer
> > can fully utilize the negotiated TCP window.
> > 
> > Use a slab cache for rpc_slots. Statically allocate an rpc_xprt rpc_slot slab
> > cache using GFP_KERNEL to the RPC_DEF_SLOT_TABLE number of slots at
> > rpc_xprt allocation.
> > 
> > Add a dynamic rpc slot allocator to rpc_xprt_ops which is set only for TCP.
> > For TCP, trigger a dyamic slot allocation in response to a write_space
> > callback which is in turn called when the TCP layer is waiting for buffer space.
> > 
> > Dynamically add a slot at the beginning of the RPC call_transmit state. The slot
> > allocator uses GFP_NOWAIT and will return without allocating a slot if
> > GFP_NOWAIT allocation fails. This is OK because the write_space callback will
> > be called again, and the dynamic slot allocator can retry.
> > 
> > Signed-off-by: Andy Adamson <andros@netap.com>
> > ---
> >  include/linux/sunrpc/sched.h |    2 +
> >  include/linux/sunrpc/xprt.h  |    6 +++-
> >  net/sunrpc/clnt.c            |    4 ++
> >  net/sunrpc/sched.c           |   39 ++++++++++++++++++++++
> >  net/sunrpc/xprt.c            |   75 +++++++++++++++++++++++++++++++++++++-----
> >  net/sunrpc/xprtsock.c        |    1 +
> >  6 files changed, 117 insertions(+), 10 deletions(-)
> > 
> 
> Nice work, comments inline below...
> 
> 
> [...]
> 
> > +
> > +/*
> > + * Static transport rpc_slot allocation called only at rpc_xprt allocation.
> > + * No need to take the xprt->reserve_lock.
> > + */
> > +int
> > +xprt_alloc_slot_entries(struct rpc_xprt *xprt, int num_req)
> > +{
> > +	struct rpc_rqst *req;
> > +	int i;
> > +
> > +	for (i = 0; i < num_req; i++) {
> > +		req = rpc_alloc_slot(GFP_KERNEL);
> > +		if (!req)
> > +			return -ENOMEM;
> > +		memset(req, 0, sizeof(*req));
> > +		list_add(&req->rq_list, &xprt->free);
> > +	}
> > +	dprintk("<-- %s mempool_alloc %d reqs\n", __func__,
> > +		xprt->max_reqs);
> > +	return 0;
> > +}
> > +
> 
> So, I don't quite get this...
> 
> You declare a global mempool early on, and then allocate from that
> mempool for a list of static entries. Doesn't that sort of rob you of
> any benefit of using a mempool here? IOW, won't the static allocations
> potentially rob the mempool of "guaranteed" entries such that the
> dynamic ones eventually all turn into slab allocations anyway?

The other thing is that we really should _never_ be using GFP_KERNEL
allocations in any paths that might be used for writebacks, since that
might recurse back into the filesystem due to direct memory reclaims. In
fact, since this code path will commonly be used by rpciod, then we
should rather be using GFP_ATOMIC and/or GFP_NOWAIT (see rpc_malloc).

> What I think would make more sense would be to have multiple mempools
> -- one per xprt and simply set the mempool size to the number of
> "static" entries that you want for the mempool. Then you could get rid
> of the free list, and just do allocations out of the mempool directly.
> You'll be guaranteed to be able to allocate up to the number in the
> mempool and everything above that would just becomes a slab allocation.

I'm not sure that we need multiple mempools: all we really require is
that memory reclaims to be able to make some form of progress.
I'd therefore rather advocate a global mempool (which matches our policy
on the rpc buffer pool), but that we allow the allocations to fail if
we're really low on memory.

The problem with multiple mempools is that if there is no activity on
one of those filesystems, then we're basically locking up memory for no
good reason.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[NeilBrown]

On Tue, 03 May 2011 20:44:50 -0400 Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:

> On Tue, 2011-05-03 at 20:20 -0400, Jeff Layton wrote:
> > On Mon,  2 May 2011 21:40:08 -0400
> > andros@netapp.com wrote:
> > 
> > > From: Andy Adamson <andros@netapp.com>
> > > 
> > > Hookup TCP congestion feedback into rpc_slot allocation so that the RPC layer
> > > can fully utilize the negotiated TCP window.
> > > 
> > > Use a slab cache for rpc_slots. Statically allocate an rpc_xprt rpc_slot slab
> > > cache using GFP_KERNEL to the RPC_DEF_SLOT_TABLE number of slots at
> > > rpc_xprt allocation.
> > > 
> > > Add a dynamic rpc slot allocator to rpc_xprt_ops which is set only for TCP.
> > > For TCP, trigger a dyamic slot allocation in response to a write_space
> > > callback which is in turn called when the TCP layer is waiting for buffer space.
> > > 
> > > Dynamically add a slot at the beginning of the RPC call_transmit state. The slot
> > > allocator uses GFP_NOWAIT and will return without allocating a slot if
> > > GFP_NOWAIT allocation fails. This is OK because the write_space callback will
> > > be called again, and the dynamic slot allocator can retry.
> > > 
> > > Signed-off-by: Andy Adamson <andros@netap.com>
> > > ---
> > >  include/linux/sunrpc/sched.h |    2 +
> > >  include/linux/sunrpc/xprt.h  |    6 +++-
> > >  net/sunrpc/clnt.c            |    4 ++
> > >  net/sunrpc/sched.c           |   39 ++++++++++++++++++++++
> > >  net/sunrpc/xprt.c            |   75 +++++++++++++++++++++++++++++++++++++-----
> > >  net/sunrpc/xprtsock.c        |    1 +
> > >  6 files changed, 117 insertions(+), 10 deletions(-)
> > > 
> > 
> > Nice work, comments inline below...
> > 
> > 
> > [...]
> > 
> > > +
> > > +/*
> > > + * Static transport rpc_slot allocation called only at rpc_xprt allocation.
> > > + * No need to take the xprt->reserve_lock.
> > > + */
> > > +int
> > > +xprt_alloc_slot_entries(struct rpc_xprt *xprt, int num_req)
> > > +{
> > > +	struct rpc_rqst *req;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < num_req; i++) {
> > > +		req = rpc_alloc_slot(GFP_KERNEL);
> > > +		if (!req)
> > > +			return -ENOMEM;
> > > +		memset(req, 0, sizeof(*req));
> > > +		list_add(&req->rq_list, &xprt->free);
> > > +	}
> > > +	dprintk("<-- %s mempool_alloc %d reqs\n", __func__,
> > > +		xprt->max_reqs);
> > > +	return 0;
> > > +}
> > > +
> > 
> > So, I don't quite get this...
> > 
> > You declare a global mempool early on, and then allocate from that
> > mempool for a list of static entries. Doesn't that sort of rob you of
> > any benefit of using a mempool here? IOW, won't the static allocations
> > potentially rob the mempool of "guaranteed" entries such that the
> > dynamic ones eventually all turn into slab allocations anyway?
> 
> The other thing is that we really should _never_ be using GFP_KERNEL
> allocations in any paths that might be used for writebacks, since that
> might recurse back into the filesystem due to direct memory reclaims. In
> fact, since this code path will commonly be used by rpciod, then we
> should rather be using GFP_ATOMIC and/or GFP_NOWAIT (see rpc_malloc).
> 
> > What I think would make more sense would be to have multiple mempools
> > -- one per xprt and simply set the mempool size to the number of
> > "static" entries that you want for the mempool. Then you could get rid
> > of the free list, and just do allocations out of the mempool directly.
> > You'll be guaranteed to be able to allocate up to the number in the
> > mempool and everything above that would just becomes a slab allocation.
> 
> I'm not sure that we need multiple mempools: all we really require is
> that memory reclaims to be able to make some form of progress.
> I'd therefore rather advocate a global mempool (which matches our policy
> on the rpc buffer pool), but that we allow the allocations to fail if
> we're really low on memory.
> 
> The problem with multiple mempools is that if there is no activity on
> one of those filesystems, then we're basically locking up memory for no
> good reason.
> 

It might be worth pointing out that mempools only use the reserved memory as
a last resort.   They try a normal allocation first and only when that fails
do they dip into the pool.  This is because normal allocation avoids taking
global locks on common case - they are CPU-local.  The pool is a global
resource (i.e. for all CPUs) so a spinlock is needed.

As GFP_KERNEL essentially never fails, it will never dip into the pool, so
there is not a lot of point using it in a mempool allocation (though I guess
it could make the code cleaner/neater and that would be a good thing).

In general, mempools should be sized to the minimum number of entries
necessary to make forward progress when memory pressure is high.  This is
typically 1.  Making mempools larger than this might allow throughput to be a
bit higher during that high-memory-pressure time, but wastes memory all the
rest of the time.

You only need multiple mempools when they might be used on the one request.
i.e. if two separate allocation are needed to handle the one request, then
they must come from two separate mempools.
If two different allocations will always be for two independent requests, then
they can safely share a mempool.

For rpc slots, I doubt you need mempools at all.
Mempools are only needed if failure is not an option and you would rather
wait, but you cannot wait for regular writeback because you are on the
writeback path.  So you use a mempool which waits for a previous request to
complete.  I don't think that describes rpc slots at all.
For rpc slots, you can afford to wait when setting up the transport, as you
are not on the writeout path yet, and later you can always cope with failure.
So just use kmalloc.

NeilBrown

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Trond Myklebust]

On Wed, 2011-05-04 at 11:18 +1000, NeilBrown wrote:
> For rpc slots, I doubt you need mempools at all.
> Mempools are only needed if failure is not an option and you would rather
> wait, but you cannot wait for regular writeback because you are on the
> writeback path.  So you use a mempool which waits for a previous request to
> complete.  I don't think that describes rpc slots at all.
> For rpc slots, you can afford to wait when setting up the transport, as you
> are not on the writeout path yet, and later you can always cope with failure.
> So just use kmalloc.

Errr.... No. By the time you get to allocating an RPC slot, you may be
bang smack in the middle of the writeout path.

The scenario would be that something triggers a writeback (kswapd,
direct reclaim,...) which triggers an RPC call, which requires you to
allocate at least one rpc slot before you can put the write on the wire.

I agree with your assertion that we only need one successful slot
allocation in order to make overall forward progress, but we definitely
do need that one...

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[NeilBrown]

On Tue, 03 May 2011 21:46:03 -0400 Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:

> On Wed, 2011-05-04 at 11:18 +1000, NeilBrown wrote:
> > For rpc slots, I doubt you need mempools at all.
> > Mempools are only needed if failure is not an option and you would rather
> > wait, but you cannot wait for regular writeback because you are on the
> > writeback path.  So you use a mempool which waits for a previous request to
> > complete.  I don't think that describes rpc slots at all.
> > For rpc slots, you can afford to wait when setting up the transport, as you
> > are not on the writeout path yet, and later you can always cope with failure.
> > So just use kmalloc.
> 
> Errr.... No. By the time you get to allocating an RPC slot, you may be
> bang smack in the middle of the writeout path.
> 
> The scenario would be that something triggers a writeback (kswapd,
> direct reclaim,...) which triggers an RPC call, which requires you to
> allocate at least one rpc slot before you can put the write on the wire.
> 
> I agree with your assertion that we only need one successful slot
> allocation in order to make overall forward progress, but we definitely
> do need that one...
> 

Probably I misunderstood the code, but I thought that there was a base set of
slots preallocated.  Eventually one of those will become free won't it?

Looking at the code again, it only ever returns entries to the mempool when
it is about to destroy the xprt.  That makes no sense.  If you are using a
mempool, then you allocate when you need to use space, and free it as soon as
you have finished with it, so the next request can get a turn.

NeilBrown

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Jeff Layton]

On Wed, 4 May 2011 12:07:26 +1000
NeilBrown <neilb@suse.de> wrote:

> On Tue, 03 May 2011 21:46:03 -0400 Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> 
> > On Wed, 2011-05-04 at 11:18 +1000, NeilBrown wrote:
> > > For rpc slots, I doubt you need mempools at all.
> > > Mempools are only needed if failure is not an option and you would rather
> > > wait, but you cannot wait for regular writeback because you are on the
> > > writeback path.  So you use a mempool which waits for a previous request to
> > > complete.  I don't think that describes rpc slots at all.
> > > For rpc slots, you can afford to wait when setting up the transport, as you
> > > are not on the writeout path yet, and later you can always cope with failure.
> > > So just use kmalloc.
> > 
> > Errr.... No. By the time you get to allocating an RPC slot, you may be
> > bang smack in the middle of the writeout path.
> > 
> > The scenario would be that something triggers a writeback (kswapd,
> > direct reclaim,...) which triggers an RPC call, which requires you to
> > allocate at least one rpc slot before you can put the write on the wire.
> > 
> > I agree with your assertion that we only need one successful slot
> > allocation in order to make overall forward progress, but we definitely
> > do need that one...
> > 
> 
> Probably I misunderstood the code, but I thought that there was a base set of
> slots preallocated.  Eventually one of those will become free won't it?
> 
> Looking at the code again, it only ever returns entries to the mempool when
> it is about to destroy the xprt.  That makes no sense.  If you are using a
> mempool, then you allocate when you need to use space, and free it as soon as
> you have finished with it, so the next request can get a turn.
> 

That was sort of my point, though I didn't articulate it very well.

I don't think we want to keep this "static" set of rqsts and the
mempool. It seems like that's somewhat redundant and more complex than
is warranted. The question is...which should we keep?

I think it sort of depends on what the intended model is. If we want to
allocate out of the "guaranteed" pool first and only when that's
exhausted fall back to doing dynamic allocations, then we probably want
to keep the static list of entries.

If we want to allocate dynamically most of the time and only dip into
the "guaranteed" pool when that can't be satisfied, then basing all of
this around mempools might make more sense.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Andy Adamson]

On May 4, 2011, at 7:54 AM, Jeff Layton wrote:

> On Wed, 4 May 2011 12:07:26 +1000
> NeilBrown <neilb@suse.de> wrote:
> 
>> On Tue, 03 May 2011 21:46:03 -0400 Trond Myklebust
>> <Trond.Myklebust@netapp.com> wrote:
>> 
>>> On Wed, 2011-05-04 at 11:18 +1000, NeilBrown wrote:
>>>> For rpc slots, I doubt you need mempools at all.
>>>> Mempools are only needed if failure is not an option and you would rather
>>>> wait, but you cannot wait for regular writeback because you are on the
>>>> writeback path.  So you use a mempool which waits for a previous request to
>>>> complete.  I don't think that describes rpc slots at all.
>>>> For rpc slots, you can afford to wait when setting up the transport, as you
>>>> are not on the writeout path yet, and later you can always cope with failure.
>>>> So just use kmalloc.
>>> 
>>> Errr.... No. By the time you get to allocating an RPC slot, you may be
>>> bang smack in the middle of the writeout path.
>>> 
>>> The scenario would be that something triggers a writeback (kswapd,
>>> direct reclaim,...) which triggers an RPC call, which requires you to
>>> allocate at least one rpc slot before you can put the write on the wire.
>>> 
>>> I agree with your assertion that we only need one successful slot
>>> allocation in order to make overall forward progress, but we definitely
>>> do need that one...
>>> 
>> 
>> Probably I misunderstood the code, but I thought that there was a base set of
>> slots preallocated.  Eventually one of those will become free won't it?
>> 
>> Looking at the code again, it only ever returns entries to the mempool when
>> it is about to destroy the xprt.  That makes no sense.  If you are using a
>> mempool, then you allocate when you need to use space, and free it as soon as
>> you have finished with it, so the next request can get a turn.
>> 
> 
> That was sort of my point, though I didn't articulate it very well.
> 
> I don't think we want to keep this "static" set of rqsts and the
> mempool. It seems like that's somewhat redundant and more complex than
> is warranted. The question is...which should we keep?
> 
> I think it sort of depends on what the intended model is. If we want to
> allocate out of the "guaranteed" pool first and only when that's
> exhausted fall back to doing dynamic allocations, then we probably want
> to keep the static list of entries.
> 
> If we want to allocate dynamically most of the time and only dip into
> the "guaranteed" pool when that can't be satisfied, then basing all of
> this around mempools might make more sense.

Thanks for the review - I've learned a lot about mempools from this discussion.

I've noticed that a lot of the time, (at least for TCP) rpc_slots are allocated and not used.
The default slot entries value is a guess. This is why I like the idea of dynamically allocating most 
of the time, and only dipping into the guaranteed pool when the dynamic allocation
can't be satisfied. e.g. get rid of the static set of slots.
Does this design require two mempools, one for dynamic allocation and a guaranteed one being small and "in reserve"?

For UDP and RDMA, the default # of slot entries can turn into an upper bound.
For TCP, the default # of slot entries would have no meaning WRT upper bound, as the upper bound is determined
by the TCP window size.

We should actually free the slots in xprt_free_slot to return the slot to the mempool.

-->Andy
> 
> -- 
> Jeff Layton <jlayton@redhat.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Jeff Layton]

On Wed, 4 May 2011 10:54:57 -0400
Andy Adamson <andros@netapp.com> wrote:

> 
> On May 4, 2011, at 7:54 AM, Jeff Layton wrote:
> 
> > On Wed, 4 May 2011 12:07:26 +1000
> > NeilBrown <neilb@suse.de> wrote:
> > 
> >> On Tue, 03 May 2011 21:46:03 -0400 Trond Myklebust
> >> <Trond.Myklebust@netapp.com> wrote:
> >> 
> >>> On Wed, 2011-05-04 at 11:18 +1000, NeilBrown wrote:
> >>>> For rpc slots, I doubt you need mempools at all.
> >>>> Mempools are only needed if failure is not an option and you would rather
> >>>> wait, but you cannot wait for regular writeback because you are on the
> >>>> writeback path.  So you use a mempool which waits for a previous request to
> >>>> complete.  I don't think that describes rpc slots at all.
> >>>> For rpc slots, you can afford to wait when setting up the transport, as you
> >>>> are not on the writeout path yet, and later you can always cope with failure.
> >>>> So just use kmalloc.
> >>> 
> >>> Errr.... No. By the time you get to allocating an RPC slot, you may be
> >>> bang smack in the middle of the writeout path.
> >>> 
> >>> The scenario would be that something triggers a writeback (kswapd,
> >>> direct reclaim,...) which triggers an RPC call, which requires you to
> >>> allocate at least one rpc slot before you can put the write on the wire.
> >>> 
> >>> I agree with your assertion that we only need one successful slot
> >>> allocation in order to make overall forward progress, but we definitely
> >>> do need that one...
> >>> 
> >> 
> >> Probably I misunderstood the code, but I thought that there was a base set of
> >> slots preallocated.  Eventually one of those will become free won't it?
> >> 
> >> Looking at the code again, it only ever returns entries to the mempool when
> >> it is about to destroy the xprt.  That makes no sense.  If you are using a
> >> mempool, then you allocate when you need to use space, and free it as soon as
> >> you have finished with it, so the next request can get a turn.
> >> 
> > 
> > That was sort of my point, though I didn't articulate it very well.
> > 
> > I don't think we want to keep this "static" set of rqsts and the
> > mempool. It seems like that's somewhat redundant and more complex than
> > is warranted. The question is...which should we keep?
> > 
> > I think it sort of depends on what the intended model is. If we want to
> > allocate out of the "guaranteed" pool first and only when that's
> > exhausted fall back to doing dynamic allocations, then we probably want
> > to keep the static list of entries.
> > 
> > If we want to allocate dynamically most of the time and only dip into
> > the "guaranteed" pool when that can't be satisfied, then basing all of
> > this around mempools might make more sense.
> 
> Thanks for the review - I've learned a lot about mempools from this discussion.
> 
> I've noticed that a lot of the time, (at least for TCP) rpc_slots are allocated and not used.
> The default slot entries value is a guess. This is why I like the idea of dynamically allocating most 
> of the time, and only dipping into the guaranteed pool when the dynamic allocation
> can't be satisfied. e.g. get rid of the static set of slots.
> Does this design require two mempools, one for dynamic allocation and a guaranteed one being small and "in reserve"?
> 

No, a single mempool would give you that. Just declare the mempool with
the amount of "emergency" slots you want, and then allocate directly
from it and then free them back to the mempool. Most of the time (as
Neil points out) you'll get them allocated from the slabcache. It'll
it'll only dip into the reserved set when those can't be allocated.

The other thing you have to determine is whether you want this
guaranteed number of slots to be per-xprt or global. Today, it's
per-xprt. Making it global might mean less wasted memory overall, but I
could forsee situations where a hung mount could starve other mounts of
slots.

Figuring that out might help guide the overall design here...

> For UDP and RDMA, the default # of slot entries can turn into an upper bound.
> For TCP, the default # of slot entries would have no meaning WRT upper bound, as the upper bound is determined
> by the TCP window size.
> 

Maybe rather than having this dynamic_slot_alloc operation that's only
conditionally called, it would cleaner to abstract out the slot
allocator altogether.

UDP and RDMA could use the same routine and just allocate out of a
fixed set of preallocated slots (sort of like how they do today), and
TCP could use this new scheme.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Trond Myklebust]

On Wed, 2011-05-04 at 11:18 -0400, Jeff Layton wrote:
> The other thing you have to determine is whether you want this
> guaranteed number of slots to be per-xprt or global. Today, it's
> per-xprt. Making it global might mean less wasted memory overall, but I
> could forsee situations where a hung mount could starve other mounts of
> slots.

The hung mount scenario is one that we're not going to deal too well
with anyway: both rpc_tasks and rpc buffers are allocated from global
mempools (as are the struct nfs_read_data/nfs_write_data)

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Andy Adamson]

On May 4, 2011, at 11:18 AM, Jeff Layton wrote:

> On Wed, 4 May 2011 10:54:57 -0400
> Andy Adamson <andros@netapp.com> wrote:
> 
>> 
>> On May 4, 2011, at 7:54 AM, Jeff Layton wrote:
>> 
>>> On Wed, 4 May 2011 12:07:26 +1000
>>> NeilBrown <neilb@suse.de> wrote:
>>> 
>>>> On Tue, 03 May 2011 21:46:03 -0400 Trond Myklebust
>>>> <Trond.Myklebust@netapp.com> wrote:
>>>> 
>>>>> On Wed, 2011-05-04 at 11:18 +1000, NeilBrown wrote:
>>>>>> For rpc slots, I doubt you need mempools at all.
>>>>>> Mempools are only needed if failure is not an option and you would rather
>>>>>> wait, but you cannot wait for regular writeback because you are on the
>>>>>> writeback path.  So you use a mempool which waits for a previous request to
>>>>>> complete.  I don't think that describes rpc slots at all.
>>>>>> For rpc slots, you can afford to wait when setting up the transport, as you
>>>>>> are not on the writeout path yet, and later you can always cope with failure.
>>>>>> So just use kmalloc.
>>>>> 
>>>>> Errr.... No. By the time you get to allocating an RPC slot, you may be
>>>>> bang smack in the middle of the writeout path.
>>>>> 
>>>>> The scenario would be that something triggers a writeback (kswapd,
>>>>> direct reclaim,...) which triggers an RPC call, which requires you to
>>>>> allocate at least one rpc slot before you can put the write on the wire.
>>>>> 
>>>>> I agree with your assertion that we only need one successful slot
>>>>> allocation in order to make overall forward progress, but we definitely
>>>>> do need that one...
>>>>> 
>>>> 
>>>> Probably I misunderstood the code, but I thought that there was a base set of
>>>> slots preallocated.  Eventually one of those will become free won't it?
>>>> 
>>>> Looking at the code again, it only ever returns entries to the mempool when
>>>> it is about to destroy the xprt.  That makes no sense.  If you are using a
>>>> mempool, then you allocate when you need to use space, and free it as soon as
>>>> you have finished with it, so the next request can get a turn.
>>>> 
>>> 
>>> That was sort of my point, though I didn't articulate it very well.
>>> 
>>> I don't think we want to keep this "static" set of rqsts and the
>>> mempool. It seems like that's somewhat redundant and more complex than
>>> is warranted. The question is...which should we keep?
>>> 
>>> I think it sort of depends on what the intended model is. If we want to
>>> allocate out of the "guaranteed" pool first and only when that's
>>> exhausted fall back to doing dynamic allocations, then we probably want
>>> to keep the static list of entries.
>>> 
>>> If we want to allocate dynamically most of the time and only dip into
>>> the "guaranteed" pool when that can't be satisfied, then basing all of
>>> this around mempools might make more sense.
>> 
>> Thanks for the review - I've learned a lot about mempools from this discussion.
>> 
>> I've noticed that a lot of the time, (at least for TCP) rpc_slots are allocated and not used.
>> The default slot entries value is a guess. This is why I like the idea of dynamically allocating most 
>> of the time, and only dipping into the guaranteed pool when the dynamic allocation
>> can't be satisfied. e.g. get rid of the static set of slots.
>> Does this design require two mempools, one for dynamic allocation and a guaranteed one being small and "in reserve"?
>> 
> 
> No, a single mempool would give you that. Just declare the mempool with
> the amount of "emergency" slots you want, and then allocate directly
> from it and then free them back to the mempool. Most of the time (as
> Neil points out) you'll get them allocated from the slabcache. It'll
> it'll only dip into the reserved set when those can't be allocated.

Cool.

> 
> The other thing you have to determine is whether you want this
> guaranteed number of slots to be per-xprt or global. Today, it's
> per-xprt. Making it global might mean less wasted memory overall, but I
> could forsee situations where a hung mount could starve other mounts of
> slots.
> 
> Figuring that out might help guide the overall design here...
> 
>> For UDP and RDMA, the default # of slot entries can turn into an upper bound.
>> For TCP, the default # of slot entries would have no meaning WRT upper bound, as the upper bound is determined
>> by the TCP window size.
>> 
> 
> Maybe rather than having this dynamic_slot_alloc operation that's only
> conditionally called, it would cleaner to abstract out the slot
> allocator altogether.
> 
> UDP and RDMA could use the same routine and just allocate out of a
> fixed set of preallocated slots (sort of like how they do today), and
> TCP could use this new scheme.

We need rpc_slots (UDP/RDMA/TCP/....) to be freed back to the mempool in xprt_free_slot.
If we allocate the set # of rpc_slots, place them on the xprt->free list, and they don't get used,
then they won't be freed until the xprt is destroyed.

I was thinking of using the same allocator for UDP/RDMA/TCP. With all transports, start off with a 
minimal # of pre-allocated slots (6? 4? 2?) and for UDP/RDMA trigger a dynamic allocation in xprt_alloc_slot
when there are no slots available and the # slots allocated is less than the default # slot entries for that transport.

-->Andy

> -- 
> Jeff Layton <jlayton@redhat.com>

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Chuck Lever]

On May 4, 2011, at 11:52 AM, Andy Adamson wrote:

> 
> On May 4, 2011, at 11:18 AM, Jeff Layton wrote:
> 
>> On Wed, 4 May 2011 10:54:57 -0400
>> Andy Adamson <andros@netapp.com> wrote:
>> 
>>> 
>>> On May 4, 2011, at 7:54 AM, Jeff Layton wrote:
>>> 
>>>> On Wed, 4 May 2011 12:07:26 +1000
>>>> NeilBrown <neilb@suse.de> wrote:
>>>> 
>>>>> On Tue, 03 May 2011 21:46:03 -0400 Trond Myklebust
>>>>> <Trond.Myklebust@netapp.com> wrote:
>>>>> 
>>>>>> On Wed, 2011-05-04 at 11:18 +1000, NeilBrown wrote:
>>>>>>> For rpc slots, I doubt you need mempools at all.
>>>>>>> Mempools are only needed if failure is not an option and you would rather
>>>>>>> wait, but you cannot wait for regular writeback because you are on the
>>>>>>> writeback path.  So you use a mempool which waits for a previous request to
>>>>>>> complete.  I don't think that describes rpc slots at all.
>>>>>>> For rpc slots, you can afford to wait when setting up the transport, as you
>>>>>>> are not on the writeout path yet, and later you can always cope with failure.
>>>>>>> So just use kmalloc.
>>>>>> 
>>>>>> Errr.... No. By the time you get to allocating an RPC slot, you may be
>>>>>> bang smack in the middle of the writeout path.
>>>>>> 
>>>>>> The scenario would be that something triggers a writeback (kswapd,
>>>>>> direct reclaim,...) which triggers an RPC call, which requires you to
>>>>>> allocate at least one rpc slot before you can put the write on the wire.
>>>>>> 
>>>>>> I agree with your assertion that we only need one successful slot
>>>>>> allocation in order to make overall forward progress, but we definitely
>>>>>> do need that one...
>>>>>> 
>>>>> 
>>>>> Probably I misunderstood the code, but I thought that there was a base set of
>>>>> slots preallocated.  Eventually one of those will become free won't it?
>>>>> 
>>>>> Looking at the code again, it only ever returns entries to the mempool when
>>>>> it is about to destroy the xprt.  That makes no sense.  If you are using a
>>>>> mempool, then you allocate when you need to use space, and free it as soon as
>>>>> you have finished with it, so the next request can get a turn.
>>>>> 
>>>> 
>>>> That was sort of my point, though I didn't articulate it very well.
>>>> 
>>>> I don't think we want to keep this "static" set of rqsts and the
>>>> mempool. It seems like that's somewhat redundant and more complex than
>>>> is warranted. The question is...which should we keep?
>>>> 
>>>> I think it sort of depends on what the intended model is. If we want to
>>>> allocate out of the "guaranteed" pool first and only when that's
>>>> exhausted fall back to doing dynamic allocations, then we probably want
>>>> to keep the static list of entries.
>>>> 
>>>> If we want to allocate dynamically most of the time and only dip into
>>>> the "guaranteed" pool when that can't be satisfied, then basing all of
>>>> this around mempools might make more sense.
>>> 
>>> Thanks for the review - I've learned a lot about mempools from this discussion.
>>> 
>>> I've noticed that a lot of the time, (at least for TCP) rpc_slots are allocated and not used.
>>> The default slot entries value is a guess. This is why I like the idea of dynamically allocating most 
>>> of the time, and only dipping into the guaranteed pool when the dynamic allocation
>>> can't be satisfied. e.g. get rid of the static set of slots.
>>> Does this design require two mempools, one for dynamic allocation and a guaranteed one being small and "in reserve"?
>>> 
>> 
>> No, a single mempool would give you that. Just declare the mempool with
>> the amount of "emergency" slots you want, and then allocate directly
>> from it and then free them back to the mempool. Most of the time (as
>> Neil points out) you'll get them allocated from the slabcache. It'll
>> it'll only dip into the reserved set when those can't be allocated.
> 
> Cool.
> 
>> 
>> The other thing you have to determine is whether you want this
>> guaranteed number of slots to be per-xprt or global. Today, it's
>> per-xprt. Making it global might mean less wasted memory overall, but I
>> could forsee situations where a hung mount could starve other mounts of
>> slots.
>> 
>> Figuring that out might help guide the overall design here...
>> 
>>> For UDP and RDMA, the default # of slot entries can turn into an upper bound.
>>> For TCP, the default # of slot entries would have no meaning WRT upper bound, as the upper bound is determined
>>> by the TCP window size.
>>> 
>> 
>> Maybe rather than having this dynamic_slot_alloc operation that's only
>> conditionally called, it would cleaner to abstract out the slot
>> allocator altogether.
>> 
>> UDP and RDMA could use the same routine and just allocate out of a
>> fixed set of preallocated slots (sort of like how they do today), and
>> TCP could use this new scheme.
> 
> We need rpc_slots (UDP/RDMA/TCP/....) to be freed back to the mempool in xprt_free_slot.
> If we allocate the set # of rpc_slots, place them on the xprt->free list, and they don't get used,
> then they won't be freed until the xprt is destroyed.

The reason to keep slots around and on a local free list is that full-out memory allocation is heavy weight.  I keep thinking of the talk Willy gave at LSF about how much scalability was affected in the SCSI I/O path by simple memory allocation operations.  Is using a SLAB or SLUB as fast as what we have today?

> I was thinking of using the same allocator for UDP/RDMA/TCP. With all transports, start off with a 
> minimal # of pre-allocated slots (6? 4? 2?) and for UDP/RDMA trigger a dynamic allocation in xprt_alloc_slot
> when there are no slots available and the # slots allocated is less than the default # slot entries for that transport.
> 
> -->Andy
> 
>> -- 
>> Jeff Layton <jlayton@redhat.com>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Andy Adamson]

On May 4, 2011, at 12:01 PM, Chuck Lever wrote:

> 
> On May 4, 2011, at 11:52 AM, Andy Adamson wrote:
> 
>> 
>> On May 4, 2011, at 11:18 AM, Jeff Layton wrote:
>> 
>>> On Wed, 4 May 2011 10:54:57 -0400
>>> Andy Adamson <andros@netapp.com> wrote:
>>> 
>>>> 
>>>> On May 4, 2011, at 7:54 AM, Jeff Layton wrote:
>>>> 
>>>>> On Wed, 4 May 2011 12:07:26 +1000
>>>>> NeilBrown <neilb@suse.de> wrote:
>>>>> 
>>>>>> On Tue, 03 May 2011 21:46:03 -0400 Trond Myklebust
>>>>>> <Trond.Myklebust@netapp.com> wrote:
>>>>>> 
>>>>>>> On Wed, 2011-05-04 at 11:18 +1000, NeilBrown wrote:
>>>>>>>> For rpc slots, I doubt you need mempools at all.
>>>>>>>> Mempools are only needed if failure is not an option and you would rather
>>>>>>>> wait, but you cannot wait for regular writeback because you are on the
>>>>>>>> writeback path.  So you use a mempool which waits for a previous request to
>>>>>>>> complete.  I don't think that describes rpc slots at all.
>>>>>>>> For rpc slots, you can afford to wait when setting up the transport, as you
>>>>>>>> are not on the writeout path yet, and later you can always cope with failure.
>>>>>>>> So just use kmalloc.
>>>>>>> 
>>>>>>> Errr.... No. By the time you get to allocating an RPC slot, you may be
>>>>>>> bang smack in the middle of the writeout path.
>>>>>>> 
>>>>>>> The scenario would be that something triggers a writeback (kswapd,
>>>>>>> direct reclaim,...) which triggers an RPC call, which requires you to
>>>>>>> allocate at least one rpc slot before you can put the write on the wire.
>>>>>>> 
>>>>>>> I agree with your assertion that we only need one successful slot
>>>>>>> allocation in order to make overall forward progress, but we definitely
>>>>>>> do need that one...
>>>>>>> 
>>>>>> 
>>>>>> Probably I misunderstood the code, but I thought that there was a base set of
>>>>>> slots preallocated.  Eventually one of those will become free won't it?
>>>>>> 
>>>>>> Looking at the code again, it only ever returns entries to the mempool when
>>>>>> it is about to destroy the xprt.  That makes no sense.  If you are using a
>>>>>> mempool, then you allocate when you need to use space, and free it as soon as
>>>>>> you have finished with it, so the next request can get a turn.
>>>>>> 
>>>>> 
>>>>> That was sort of my point, though I didn't articulate it very well.
>>>>> 
>>>>> I don't think we want to keep this "static" set of rqsts and the
>>>>> mempool. It seems like that's somewhat redundant and more complex than
>>>>> is warranted. The question is...which should we keep?
>>>>> 
>>>>> I think it sort of depends on what the intended model is. If we want to
>>>>> allocate out of the "guaranteed" pool first and only when that's
>>>>> exhausted fall back to doing dynamic allocations, then we probably want
>>>>> to keep the static list of entries.
>>>>> 
>>>>> If we want to allocate dynamically most of the time and only dip into
>>>>> the "guaranteed" pool when that can't be satisfied, then basing all of
>>>>> this around mempools might make more sense.
>>>> 
>>>> Thanks for the review - I've learned a lot about mempools from this discussion.
>>>> 
>>>> I've noticed that a lot of the time, (at least for TCP) rpc_slots are allocated and not used.
>>>> The default slot entries value is a guess. This is why I like the idea of dynamically allocating most 
>>>> of the time, and only dipping into the guaranteed pool when the dynamic allocation
>>>> can't be satisfied. e.g. get rid of the static set of slots.
>>>> Does this design require two mempools, one for dynamic allocation and a guaranteed one being small and "in reserve"?
>>>> 
>>> 
>>> No, a single mempool would give you that. Just declare the mempool with
>>> the amount of "emergency" slots you want, and then allocate directly
>>> from it and then free them back to the mempool. Most of the time (as
>>> Neil points out) you'll get them allocated from the slabcache. It'll
>>> it'll only dip into the reserved set when those can't be allocated.
>> 
>> Cool.
>> 
>>> 
>>> The other thing you have to determine is whether you want this
>>> guaranteed number of slots to be per-xprt or global. Today, it's
>>> per-xprt. Making it global might mean less wasted memory overall, but I
>>> could forsee situations where a hung mount could starve other mounts of
>>> slots.
>>> 
>>> Figuring that out might help guide the overall design here...
>>> 
>>>> For UDP and RDMA, the default # of slot entries can turn into an upper bound.
>>>> For TCP, the default # of slot entries would have no meaning WRT upper bound, as the upper bound is determined
>>>> by the TCP window size.
>>>> 
>>> 
>>> Maybe rather than having this dynamic_slot_alloc operation that's only
>>> conditionally called, it would cleaner to abstract out the slot
>>> allocator altogether.
>>> 
>>> UDP and RDMA could use the same routine and just allocate out of a
>>> fixed set of preallocated slots (sort of like how they do today), and
>>> TCP could use this new scheme.
>> 
>> We need rpc_slots (UDP/RDMA/TCP/....) to be freed back to the mempool in xprt_free_slot.
>> If we allocate the set # of rpc_slots, place them on the xprt->free list, and they don't get used,
>> then they won't be freed until the xprt is destroyed.
> 
> The reason to keep slots around and on a local free list is that full-out memory allocation is heavy weight.  I keep thinking of the talk Willy gave at LSF about how much scalability was affected in the SCSI I/O path by simple memory allocation operations.  Is using a SLAB or SLUB as fast as what we have today?

That is a good question. We could implement this same slot allocation scheme without a SLAB/SLUB - just use kmalloc and leave xprt_free_slot to just move the allocated slots to the free list as is done today. I don't have any information on a performance hit. 

Is this worth testing, or should we just stick with kmalloc and friends?

-->Andy
> 
>> I was thinking of using the same allocator for UDP/RDMA/TCP. With all transports, start off with a 
>> minimal # of pre-allocated slots (6? 4? 2?) and for UDP/RDMA trigger a dynamic allocation in xprt_alloc_slot
>> when there are no slots available and the # slots allocated is less than the default # slot entries for that transport.
>> 
>> -->Andy
>> 
>>> -- 
>>> Jeff Layton <jlayton@redhat.com>
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Jeff Layton]

On Wed, 4 May 2011 13:22:01 -0400
Andy Adamson <andros@netapp.com> wrote:

> 
> On May 4, 2011, at 12:01 PM, Chuck Lever wrote:
> 
> > 
> > On May 4, 2011, at 11:52 AM, Andy Adamson wrote:
> > 
> >> 
> >> On May 4, 2011, at 11:18 AM, Jeff Layton wrote:
> >> 
> >>> On Wed, 4 May 2011 10:54:57 -0400
> >>> Andy Adamson <andros@netapp.com> wrote:
> >>> 
> >>>> 
> >>>> On May 4, 2011, at 7:54 AM, Jeff Layton wrote:
> >>>> 
> >>>>> On Wed, 4 May 2011 12:07:26 +1000
> >>>>> NeilBrown <neilb@suse.de> wrote:
> >>>>> 
> >>>>>> On Tue, 03 May 2011 21:46:03 -0400 Trond Myklebust
> >>>>>> <Trond.Myklebust@netapp.com> wrote:
> >>>>>> 
> >>>>>>> On Wed, 2011-05-04 at 11:18 +1000, NeilBrown wrote:
> >>>>>>>> For rpc slots, I doubt you need mempools at all.
> >>>>>>>> Mempools are only needed if failure is not an option and you would rather
> >>>>>>>> wait, but you cannot wait for regular writeback because you are on the
> >>>>>>>> writeback path.  So you use a mempool which waits for a previous request to
> >>>>>>>> complete.  I don't think that describes rpc slots at all.
> >>>>>>>> For rpc slots, you can afford to wait when setting up the transport, as you
> >>>>>>>> are not on the writeout path yet, and later you can always cope with failure.
> >>>>>>>> So just use kmalloc.
> >>>>>>> 
> >>>>>>> Errr.... No. By the time you get to allocating an RPC slot, you may be
> >>>>>>> bang smack in the middle of the writeout path.
> >>>>>>> 
> >>>>>>> The scenario would be that something triggers a writeback (kswapd,
> >>>>>>> direct reclaim,...) which triggers an RPC call, which requires you to
> >>>>>>> allocate at least one rpc slot before you can put the write on the wire.
> >>>>>>> 
> >>>>>>> I agree with your assertion that we only need one successful slot
> >>>>>>> allocation in order to make overall forward progress, but we definitely
> >>>>>>> do need that one...
> >>>>>>> 
> >>>>>> 
> >>>>>> Probably I misunderstood the code, but I thought that there was a base set of
> >>>>>> slots preallocated.  Eventually one of those will become free won't it?
> >>>>>> 
> >>>>>> Looking at the code again, it only ever returns entries to the mempool when
> >>>>>> it is about to destroy the xprt.  That makes no sense.  If you are using a
> >>>>>> mempool, then you allocate when you need to use space, and free it as soon as
> >>>>>> you have finished with it, so the next request can get a turn.
> >>>>>> 
> >>>>> 
> >>>>> That was sort of my point, though I didn't articulate it very well.
> >>>>> 
> >>>>> I don't think we want to keep this "static" set of rqsts and the
> >>>>> mempool. It seems like that's somewhat redundant and more complex than
> >>>>> is warranted. The question is...which should we keep?
> >>>>> 
> >>>>> I think it sort of depends on what the intended model is. If we want to
> >>>>> allocate out of the "guaranteed" pool first and only when that's
> >>>>> exhausted fall back to doing dynamic allocations, then we probably want
> >>>>> to keep the static list of entries.
> >>>>> 
> >>>>> If we want to allocate dynamically most of the time and only dip into
> >>>>> the "guaranteed" pool when that can't be satisfied, then basing all of
> >>>>> this around mempools might make more sense.
> >>>> 
> >>>> Thanks for the review - I've learned a lot about mempools from this discussion.
> >>>> 
> >>>> I've noticed that a lot of the time, (at least for TCP) rpc_slots are allocated and not used.
> >>>> The default slot entries value is a guess. This is why I like the idea of dynamically allocating most 
> >>>> of the time, and only dipping into the guaranteed pool when the dynamic allocation
> >>>> can't be satisfied. e.g. get rid of the static set of slots.
> >>>> Does this design require two mempools, one for dynamic allocation and a guaranteed one being small and "in reserve"?
> >>>> 
> >>> 
> >>> No, a single mempool would give you that. Just declare the mempool with
> >>> the amount of "emergency" slots you want, and then allocate directly
> >>> from it and then free them back to the mempool. Most of the time (as
> >>> Neil points out) you'll get them allocated from the slabcache. It'll
> >>> it'll only dip into the reserved set when those can't be allocated.
> >> 
> >> Cool.
> >> 
> >>> 
> >>> The other thing you have to determine is whether you want this
> >>> guaranteed number of slots to be per-xprt or global. Today, it's
> >>> per-xprt. Making it global might mean less wasted memory overall, but I
> >>> could forsee situations where a hung mount could starve other mounts of
> >>> slots.
> >>> 
> >>> Figuring that out might help guide the overall design here...
> >>> 
> >>>> For UDP and RDMA, the default # of slot entries can turn into an upper bound.
> >>>> For TCP, the default # of slot entries would have no meaning WRT upper bound, as the upper bound is determined
> >>>> by the TCP window size.
> >>>> 
> >>> 
> >>> Maybe rather than having this dynamic_slot_alloc operation that's only
> >>> conditionally called, it would cleaner to abstract out the slot
> >>> allocator altogether.
> >>> 
> >>> UDP and RDMA could use the same routine and just allocate out of a
> >>> fixed set of preallocated slots (sort of like how they do today), and
> >>> TCP could use this new scheme.
> >> 
> >> We need rpc_slots (UDP/RDMA/TCP/....) to be freed back to the mempool in xprt_free_slot.
> >> If we allocate the set # of rpc_slots, place them on the xprt->free list, and they don't get used,
> >> then they won't be freed until the xprt is destroyed.
> > 
> > The reason to keep slots around and on a local free list is that full-out memory allocation is heavy weight.  I keep thinking of the talk Willy gave at LSF about how much scalability was affected in the SCSI I/O path by simple memory allocation operations.  Is using a SLAB or SLUB as fast as what we have today?
> 
> That is a good question. We could implement this same slot allocation scheme without a SLAB/SLUB - just use kmalloc and leave xprt_free_slot to just move the allocated slots to the free list as is done today. I don't have any information on a performance hit. 
> 
> Is this worth testing, or should we just stick with kmalloc and friends?
> 

I think Chuck's point here is that there is an argument for doing it
the other way around -- keeping a static set of objects and only
dynamically allocating when those are used up.

Of course, pulling them off the free list is not cost-free either (you
do need to take a spinlock there), but it may have less contention.

It's hard for me to imagine that this will make a big difference
overall, but I guess you never know. Personally, I wouldn't worry about
it. :)

-- 
Jeff Layton <jlayton@redhat.com>

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Jeff Layton]

On Tue, 03 May 2011 20:44:50 -0400
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Tue, 2011-05-03 at 20:20 -0400, Jeff Layton wrote:
> > On Mon,  2 May 2011 21:40:08 -0400
> > andros@netapp.com wrote:
> > 
> > > From: Andy Adamson <andros@netapp.com>
> > > 
> > > Hookup TCP congestion feedback into rpc_slot allocation so that the RPC layer
> > > can fully utilize the negotiated TCP window.
> > > 
> > > Use a slab cache for rpc_slots. Statically allocate an rpc_xprt rpc_slot slab
> > > cache using GFP_KERNEL to the RPC_DEF_SLOT_TABLE number of slots at
> > > rpc_xprt allocation.
> > > 
> > > Add a dynamic rpc slot allocator to rpc_xprt_ops which is set only for TCP.
> > > For TCP, trigger a dyamic slot allocation in response to a write_space
> > > callback which is in turn called when the TCP layer is waiting for buffer space.
> > > 
> > > Dynamically add a slot at the beginning of the RPC call_transmit state. The slot
> > > allocator uses GFP_NOWAIT and will return without allocating a slot if
> > > GFP_NOWAIT allocation fails. This is OK because the write_space callback will
> > > be called again, and the dynamic slot allocator can retry.
> > > 
> > > Signed-off-by: Andy Adamson <andros@netap.com>
> > > ---
> > >  include/linux/sunrpc/sched.h |    2 +
> > >  include/linux/sunrpc/xprt.h  |    6 +++-
> > >  net/sunrpc/clnt.c            |    4 ++
> > >  net/sunrpc/sched.c           |   39 ++++++++++++++++++++++
> > >  net/sunrpc/xprt.c            |   75 +++++++++++++++++++++++++++++++++++++-----
> > >  net/sunrpc/xprtsock.c        |    1 +
> > >  6 files changed, 117 insertions(+), 10 deletions(-)
> > > 
> > 
> > Nice work, comments inline below...
> > 
> > 
> > [...]
> > 
> > > +
> > > +/*
> > > + * Static transport rpc_slot allocation called only at rpc_xprt allocation.
> > > + * No need to take the xprt->reserve_lock.
> > > + */
> > > +int
> > > +xprt_alloc_slot_entries(struct rpc_xprt *xprt, int num_req)
> > > +{
> > > +	struct rpc_rqst *req;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < num_req; i++) {
> > > +		req = rpc_alloc_slot(GFP_KERNEL);
> > > +		if (!req)
> > > +			return -ENOMEM;
> > > +		memset(req, 0, sizeof(*req));
> > > +		list_add(&req->rq_list, &xprt->free);
> > > +	}
> > > +	dprintk("<-- %s mempool_alloc %d reqs\n", __func__,
> > > +		xprt->max_reqs);
> > > +	return 0;
> > > +}
> > > +
> > 
> > So, I don't quite get this...
> > 
> > You declare a global mempool early on, and then allocate from that
> > mempool for a list of static entries. Doesn't that sort of rob you of
> > any benefit of using a mempool here? IOW, won't the static allocations
> > potentially rob the mempool of "guaranteed" entries such that the
> > dynamic ones eventually all turn into slab allocations anyway?
> 
> The other thing is that we really should _never_ be using GFP_KERNEL
> allocations in any paths that might be used for writebacks, since that
> might recurse back into the filesystem due to direct memory reclaims. In
> fact, since this code path will commonly be used by rpciod, then we
> should rather be using GFP_ATOMIC and/or GFP_NOWAIT (see rpc_malloc).
> 

I think GFP_NOWAIT is probably appropriate, and that seems to be what
the patch uses for the dynamic allocations. It only uses GFP_KERNEL for
the "static" ones (created at xprt_alloc time).

> > What I think would make more sense would be to have multiple mempools
> > -- one per xprt and simply set the mempool size to the number of
> > "static" entries that you want for the mempool. Then you could get rid
> > of the free list, and just do allocations out of the mempool directly.
> > You'll be guaranteed to be able to allocate up to the number in the
> > mempool and everything above that would just becomes a slab allocation.
> 
> I'm not sure that we need multiple mempools: all we really require is
> that memory reclaims to be able to make some form of progress.
> I'd therefore rather advocate a global mempool (which matches our policy
> on the rpc buffer pool), but that we allow the allocations to fail if
> we're really low on memory.
> 
> The problem with multiple mempools is that if there is no activity on
> one of those filesystems, then we're basically locking up memory for no
> good reason.
> 

But this patch does that too. It allocates a list of static entries at
xprt_alloc time that won't shrink when memory is tight. If that's
something we want to keep, then why bother with a mempool for the
"dynamic" entries? We'll already have a set of guaranteed entries (the
static ones).

The whole point of a mempool is to give you a pool of objects that has
a guaranteed minimum number. I think we just need to understand whether
that minimum should be a global one or per-xprt.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Jeff Layton]

On Mon,  2 May 2011 21:40:08 -0400
andros@netapp.com wrote:

> From: Andy Adamson <andros@netapp.com>
> 
> Hookup TCP congestion feedback into rpc_slot allocation so that the RPC layer
> can fully utilize the negotiated TCP window.
> 
> Use a slab cache for rpc_slots. Statically allocate an rpc_xprt rpc_slot slab
> cache using GFP_KERNEL to the RPC_DEF_SLOT_TABLE number of slots at
> rpc_xprt allocation.
> 
> Add a dynamic rpc slot allocator to rpc_xprt_ops which is set only for TCP.
> For TCP, trigger a dyamic slot allocation in response to a write_space
> callback which is in turn called when the TCP layer is waiting for buffer space.
> 
> Dynamically add a slot at the beginning of the RPC call_transmit state. The slot
> allocator uses GFP_NOWAIT and will return without allocating a slot if
> GFP_NOWAIT allocation fails. This is OK because the write_space callback will
> be called again, and the dynamic slot allocator can retry.
> 
> Signed-off-by: Andy Adamson <andros@netap.com>
> ---
>  include/linux/sunrpc/sched.h |    2 +
>  include/linux/sunrpc/xprt.h  |    6 +++-
>  net/sunrpc/clnt.c            |    4 ++
>  net/sunrpc/sched.c           |   39 ++++++++++++++++++++++
>  net/sunrpc/xprt.c            |   75 +++++++++++++++++++++++++++++++++++++-----
>  net/sunrpc/xprtsock.c        |    1 +
>  6 files changed, 117 insertions(+), 10 deletions(-)
> 

[...]

> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index 9494c37..1b0aa55 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -498,6 +498,7 @@ void xprt_write_space(struct rpc_xprt *xprt)
>  		dprintk("RPC:       write space: waking waiting task on "
>  				"xprt %p\n", xprt);
>  		rpc_wake_up_queued_task(&xprt->pending, xprt->snd_task);
> +		set_bit(XPRT_WRITE_SPACE, &xprt->state);
>  	}
>  	spin_unlock_bh(&xprt->transport_lock);
>  }
> @@ -957,6 +958,66 @@ static void xprt_free_slot(struct rpc_xprt *xprt, struct rpc_rqst *req)
>  	spin_unlock(&xprt->reserve_lock);
>  }
>  

Is the above a potential race as well? I looks possible (if unlikely)
that we'd wake up the task, and it find that XPRT_WRITE_SPACE is still
unset. Would it make more sense to do the set_bit call before waking up
the task?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Andy Adamson]

On May 4, 2011, at 10:59 AM, Jeff Layton wrote:

> On Mon,  2 May 2011 21:40:08 -0400
> andros@netapp.com wrote:
> 
>> From: Andy Adamson <andros@netapp.com>
>> 
>> Hookup TCP congestion feedback into rpc_slot allocation so that the RPC layer
>> can fully utilize the negotiated TCP window.
>> 
>> Use a slab cache for rpc_slots. Statically allocate an rpc_xprt rpc_slot slab
>> cache using GFP_KERNEL to the RPC_DEF_SLOT_TABLE number of slots at
>> rpc_xprt allocation.
>> 
>> Add a dynamic rpc slot allocator to rpc_xprt_ops which is set only for TCP.
>> For TCP, trigger a dyamic slot allocation in response to a write_space
>> callback which is in turn called when the TCP layer is waiting for buffer space.
>> 
>> Dynamically add a slot at the beginning of the RPC call_transmit state. The slot
>> allocator uses GFP_NOWAIT and will return without allocating a slot if
>> GFP_NOWAIT allocation fails. This is OK because the write_space callback will
>> be called again, and the dynamic slot allocator can retry.
>> 
>> Signed-off-by: Andy Adamson <andros-tvW9BSKw3QMAvxtiuMwx3w@public.gmane.org>
>> ---
>> include/linux/sunrpc/sched.h |    2 +
>> include/linux/sunrpc/xprt.h  |    6 +++-
>> net/sunrpc/clnt.c            |    4 ++
>> net/sunrpc/sched.c           |   39 ++++++++++++++++++++++
>> net/sunrpc/xprt.c            |   75 +++++++++++++++++++++++++++++++++++++-----
>> net/sunrpc/xprtsock.c        |    1 +
>> 6 files changed, 117 insertions(+), 10 deletions(-)
>> 
> 
> [...]
> 
>> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
>> index 9494c37..1b0aa55 100644
>> --- a/net/sunrpc/xprt.c
>> +++ b/net/sunrpc/xprt.c
>> @@ -498,6 +498,7 @@ void xprt_write_space(struct rpc_xprt *xprt)
>> 		dprintk("RPC:       write space: waking waiting task on "
>> 				"xprt %p\n", xprt);
>> 		rpc_wake_up_queued_task(&xprt->pending, xprt->snd_task);
>> +		set_bit(XPRT_WRITE_SPACE, &xprt->state);
>> 	}
>> 	spin_unlock_bh(&xprt->transport_lock);
>> }
>> @@ -957,6 +958,66 @@ static void xprt_free_slot(struct rpc_xprt *xprt, struct rpc_rqst *req)
>> 	spin_unlock(&xprt->reserve_lock);
>> }
>> 
> 
> Is the above a potential race as well? I looks possible (if unlikely)
> that we'd wake up the task, and it find that XPRT_WRITE_SPACE is still
> unset. Would it make more sense to do the set_bit call before waking up
> the task?

Yes - good catch.

-->Andy
> 
> -- 
> Jeff Layton <jlayton@redhat.com>

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Jeff Layton]

On Mon,  2 May 2011 21:40:08 -0400
andros@netapp.com wrote:

> +	if (!test_and_clear_bit(XPRT_WRITE_SPACE, &xprt->state))
> +		return;

Also, I'm not sure that a single bit really conveys enough information
for this.

IIUC, sk_write_space gets called when a packet is TCP ACK'ed. It seems
possible that we would sometimes have buffer space available to queue
the packet without sk_write_space being called. With this, we'll
basically be serializing all dynamic slot allocations behind the
sk_write_space callbacks.

Consider the case of many small TCP frames being sent after a large one
just got ACK'ed. Only one would be allowed to be sent, even though
there might be enough send buffer space to allow for more.

Would it instead make more sense to base this on the amount of space
available in the actual socket rather than this bit?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Andy Adamson]

On May 4, 2011, at 11:08 AM, Jeff Layton wrote:

> On Mon,  2 May 2011 21:40:08 -0400
> andros@netapp.com wrote:
> 
>> +	if (!test_and_clear_bit(XPRT_WRITE_SPACE, &xprt->state))
>> +		return;
> 
> Also, I'm not sure that a single bit really conveys enough information
> for this.
> 
> IIUC, sk_write_space gets called when a packet is TCP ACK'ed. It seems
> possible that we would sometimes have buffer space available to queue
> the packet without sk_write_space being called. With this, we'll
> basically be serializing all dynamic slot allocations behind the
> sk_write_space callbacks.

Which I thought was OK given that the TCP window takes a while to stabilize.

> 
> Consider the case of many small TCP frames being sent after a large one
> just got ACK'ed. Only one would be allowed to be sent, even though
> there might be enough send buffer space to allow for more.
> 
> Would it instead make more sense to base this on the amount of space
> available in the actual socket rather than this bit?

So at each write_space, potentially allocate more than one rpc_slot as opposed
to allocating one rpc_slot and waiting for the next write_space? I could look at this
with the 10G testiing.

-->Andy

> 
> -- 
> Jeff Layton <jlayton@redhat.com>

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Jeff Layton]

On Wed, 4 May 2011 11:20:59 -0400
Andy Adamson <andros@netapp.com> wrote:

> 
> On May 4, 2011, at 11:08 AM, Jeff Layton wrote:
> 
> > On Mon,  2 May 2011 21:40:08 -0400
> > andros@netapp.com wrote:
> > 
> >> +	if (!test_and_clear_bit(XPRT_WRITE_SPACE, &xprt->state))
> >> +		return;
> > 
> > Also, I'm not sure that a single bit really conveys enough information
> > for this.
> > 
> > IIUC, sk_write_space gets called when a packet is TCP ACK'ed. It seems
> > possible that we would sometimes have buffer space available to queue
> > the packet without sk_write_space being called. With this, we'll
> > basically be serializing all dynamic slot allocations behind the
> > sk_write_space callbacks.
> 
> Which I thought was OK given that the TCP window takes a while to stabilize.
> 

Hmm, ok. I hadn't considered that, but you may be correct.

> > 
> > Consider the case of many small TCP frames being sent after a large one
> > just got ACK'ed. Only one would be allowed to be sent, even though
> > there might be enough send buffer space to allow for more.
> > 
> > Would it instead make more sense to base this on the amount of space
> > available in the actual socket rather than this bit?
> 
> So at each write_space, potentially allocate more than one rpc_slot as opposed
> to allocating one rpc_slot and waiting for the next write_space? I could look at this
> with the 10G testiing.
> 

xs_tcp_write_space has this:

        /* from net/core/stream.c:sk_stream_write_space */
        if (sk_stream_wspace(sk) >= sk_stream_min_wspace(sk))
                xs_write_space(sk);

...my (hand-wavy) thinking was that it might make sense to check
against this value again in the slot allocation routine.

IOW, allow allocation of a slot if there's reasonably enough buffer
space to do a send instead of allowing just one per xs_write_space
callback.

Of course, that gets complicated if there are a bunch of slot
allocations in parallel. You'd probably need to offset the amount of
available space by a certain amount for each slot that has been
allocated but not yet sent.

Perhaps though that's too complicated to be really useful...

-- 
Jeff Layton <jlayton@redhat.com>

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Trond Myklebust]

On Wed, 2011-05-04 at 11:20 -0400, Andy Adamson wrote:
> On May 4, 2011, at 11:08 AM, Jeff Layton wrote:
> 
> > On Mon,  2 May 2011 21:40:08 -0400
> > andros@netapp.com wrote:
> > 
> >> +	if (!test_and_clear_bit(XPRT_WRITE_SPACE, &xprt->state))
> >> +		return;
> > 
> > Also, I'm not sure that a single bit really conveys enough information
> > for this.
> > 
> > IIUC, sk_write_space gets called when a packet is TCP ACK'ed. It seems
> > possible that we would sometimes have buffer space available to queue
> > the packet without sk_write_space being called. With this, we'll
> > basically be serializing all dynamic slot allocations behind the
> > sk_write_space callbacks.
> 
> Which I thought was OK given that the TCP window takes a while to stabilize.
> 
> > 
> > Consider the case of many small TCP frames being sent after a large one
> > just got ACK'ed. Only one would be allowed to be sent, even though
> > there might be enough send buffer space to allow for more.
> > 
> > Would it instead make more sense to base this on the amount of space
> > available in the actual socket rather than this bit?
> 
> So at each write_space, potentially allocate more than one rpc_slot as opposed
> to allocating one rpc_slot and waiting for the next write_space? I could look at this
> with the 10G testiing.

Why? You can't send that data. Once you hit the write space limit, then
the socket remains blocked until you get the callback. It doesn't matter
how small the frame, you will not be allowed to send more data.

On the other hand, we do set the SOCK_NOSPACE bit, which means that the
socket layer will attempt to grow the TCP window even though we're not
actually putting more data into the socket.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Jeff Layton]

On Wed, 04 May 2011 11:35:34 -0400
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Wed, 2011-05-04 at 11:20 -0400, Andy Adamson wrote:
> > On May 4, 2011, at 11:08 AM, Jeff Layton wrote:
> > 
> > > On Mon,  2 May 2011 21:40:08 -0400
> > > andros@netapp.com wrote:
> > > 
> > >> +	if (!test_and_clear_bit(XPRT_WRITE_SPACE, &xprt->state))
> > >> +		return;
> > > 
> > > Also, I'm not sure that a single bit really conveys enough information
> > > for this.
> > > 
> > > IIUC, sk_write_space gets called when a packet is TCP ACK'ed. It seems
> > > possible that we would sometimes have buffer space available to queue
> > > the packet without sk_write_space being called. With this, we'll
> > > basically be serializing all dynamic slot allocations behind the
> > > sk_write_space callbacks.
> > 
> > Which I thought was OK given that the TCP window takes a while to stabilize.
> > 
> > > 
> > > Consider the case of many small TCP frames being sent after a large one
> > > just got ACK'ed. Only one would be allowed to be sent, even though
> > > there might be enough send buffer space to allow for more.
> > > 
> > > Would it instead make more sense to base this on the amount of space
> > > available in the actual socket rather than this bit?
> > 
> > So at each write_space, potentially allocate more than one rpc_slot as opposed
> > to allocating one rpc_slot and waiting for the next write_space? I could look at this
> > with the 10G testiing.
> 
> Why? You can't send that data. Once you hit the write space limit, then
> the socket remains blocked until you get the callback. It doesn't matter
> how small the frame, you will not be allowed to send more data.
> 
> On the other hand, we do set the SOCK_NOSPACE bit, which means that the
> socket layer will attempt to grow the TCP window even though we're not
> actually putting more data into the socket.
> 

I'm not sure I understand what you're suggesting here.

I guess my main point is that a single bit that we flip on in
write_space and flip off when a slot is allocated doesn't carry enough
info. That scheme will also be subject to subtle differences in timing.
For instance...

Suppose a large number of TCP ACKs come in all at around the same time.
write_space gets called a bunch of times in succession, so the bit gets
"set" several times. Several queued tasks get woken up but only one can
clear the bit so only one gets a slot.

However, if those acks come in with enough of a delay between them, then
you can potentially get one slot allocated per write_space callback.

I think we ought to consider a heuristic that doesn't rely on the
frequency and timing of write_space callbacks.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [[RFC] 1/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Trond Myklebust]

On Thu, 2011-05-05 at 07:47 -0400, Jeff Layton wrote:
> On Wed, 04 May 2011 11:35:34 -0400
> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> 
> > On Wed, 2011-05-04 at 11:20 -0400, Andy Adamson wrote:
> > > On May 4, 2011, at 11:08 AM, Jeff Layton wrote:
> > > 
> > > > On Mon,  2 May 2011 21:40:08 -0400
> > > > andros@netapp.com wrote:
> > > > 
> > > >> +	if (!test_and_clear_bit(XPRT_WRITE_SPACE, &xprt->state))
> > > >> +		return;
> > > > 
> > > > Also, I'm not sure that a single bit really conveys enough information
> > > > for this.
> > > > 
> > > > IIUC, sk_write_space gets called when a packet is TCP ACK'ed. It seems
> > > > possible that we would sometimes have buffer space available to queue
> > > > the packet without sk_write_space being called. With this, we'll
> > > > basically be serializing all dynamic slot allocations behind the
> > > > sk_write_space callbacks.
> > > 
> > > Which I thought was OK given that the TCP window takes a while to stabilize.
> > > 
> > > > 
> > > > Consider the case of many small TCP frames being sent after a large one
> > > > just got ACK'ed. Only one would be allowed to be sent, even though
> > > > there might be enough send buffer space to allow for more.
> > > > 
> > > > Would it instead make more sense to base this on the amount of space
> > > > available in the actual socket rather than this bit?
> > > 
> > > So at each write_space, potentially allocate more than one rpc_slot as opposed
> > > to allocating one rpc_slot and waiting for the next write_space? I could look at this
> > > with the 10G testiing.
> > 
> > Why? You can't send that data. Once you hit the write space limit, then
> > the socket remains blocked until you get the callback. It doesn't matter
> > how small the frame, you will not be allowed to send more data.
> > 
> > On the other hand, we do set the SOCK_NOSPACE bit, which means that the
> > socket layer will attempt to grow the TCP window even though we're not
> > actually putting more data into the socket.
> > 
> 
> I'm not sure I understand what you're suggesting here.
> 
> I guess my main point is that a single bit that we flip on in
> write_space and flip off when a slot is allocated doesn't carry enough
> info. That scheme will also be subject to subtle differences in timing.
> For instance...
> 
> Suppose a large number of TCP ACKs come in all at around the same time.
> write_space gets called a bunch of times in succession, so the bit gets
> "set" several times. Several queued tasks get woken up but only one can
> clear the bit so only one gets a slot.
> 
> However, if those acks come in with enough of a delay between them, then
> you can potentially get one slot allocated per write_space callback.

The write space callback doesn't wake anyone up until 1/2 the total
socket buffer is free: that's what the sock_writeable() test does.

> I think we ought to consider a heuristic that doesn't rely on the
> frequency and timing of write_space callbacks.

What we're doing now is basically what is being done by the socket layer
when a user process tries to write too much data to the socket: we tell
the TCP layer to grow the window, and we wait for the write space
callback to tell us that we have enough free socket buffer space to be
able to make progress. We're not waking up and retrying on every ACK as
you suggest.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [RFC 0/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Chuck Lever]

Hi-

On May 2, 2011, at 9:40 PM, andros@netapp.com wrote:

> I would appreciate comments on this patch. I've done a small amount of testing
> where I've set the /proc/sys/sunrpc/tcp_slot_table_entries to 2, and watched
> the dynamic allocator increase it to 3. I have a 10G test system being
> configured.

For TCP transports, then, the effective slot table size varies between the tcp_slot_table_entries setting and the estimated number of RPCs that can fit in the TCP window?  What happens when the TCP window shrinks such that fewer RPCs can fit than are allowed by the tcp_slot_table_entries setting?

Have you done performance testing with loopback mounts (ie server and client on the same machine, mounting over the "lo" interface)?  As I recall this was the test case that Trond found worked poorly when we boosted the tcp_slot_table_entries setting to default to a 64 entry slot table.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [RFC 0/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Andy Adamson]

On May 3, 2011, at 4:06 PM, Chuck Lever wrote:

> Hi-
> 
> On May 2, 2011, at 9:40 PM, andros@netapp.com wrote:
> 
>> I would appreciate comments on this patch. I've done a small amount of testing
>> where I've set the /proc/sys/sunrpc/tcp_slot_table_entries to 2, and watched
>> the dynamic allocator increase it to 3. I have a 10G test system being
>> configured.
> 
> For TCP transports, then, the effective slot table size varies between the tcp_slot_table_entries setting and the estimated number of RPCs that can fit in the TCP window?  

Yes.

> What happens when the TCP window shrinks such that fewer RPCs can fit than are allowed by the tcp_slot_table_entries setting?

We don't shrink the rpc_slot table size. The idea being that if the TCP hits size X then shrinks to size X-n, there is a good chance that it will get back to size X. In the worst case, this is still better than a hard configured 'to large' slot table size.

> 
> Have you done performance testing with loopback mounts (ie server and client on the same machine, mounting over the "lo" interface)?  As I recall this was the test case that Trond found worked poorly when we boosted the tcp_slot_table_entries setting to default to a 64 entry slot table.

No, not yet. I'll add this to the planned testing.  So having way too many default rpc_slots is bad? I could test by starting with a default of 2 slots and seeing how many are needed.

-->Andy

> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
> 

Re: [RFC 0/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Chuck Lever]

On May 3, 2011, at 4:13 PM, Andy Adamson wrote:

> 
> On May 3, 2011, at 4:06 PM, Chuck Lever wrote:
> 
>> Hi-
>> 
>> On May 2, 2011, at 9:40 PM, andros@netapp.com wrote:
>> 
>>> I would appreciate comments on this patch. I've done a small amount of testing
>>> where I've set the /proc/sys/sunrpc/tcp_slot_table_entries to 2, and watched
>>> the dynamic allocator increase it to 3. I have a 10G test system being
>>> configured.
>> 
>> For TCP transports, then, the effective slot table size varies between the tcp_slot_table_entries setting and the estimated number of RPCs that can fit in the TCP window?  
> 
> Yes.
> 
>> What happens when the TCP window shrinks such that fewer RPCs can fit than are allowed by the tcp_slot_table_entries setting?
> 
> We don't shrink the rpc_slot table size. The idea being that if the TCP hits size X then shrinks to size X-n, there is a good chance that it will get back to size X. In the worst case, this is still better than a hard configured 'to large' slot table size.

Seems to me that we wouldn't need the tcp_slot_table_entries setting at all, then.  Always start at 2 and it will adjust itself as needed.  Unless we find we need a hard upper bound in some cases.

>> Have you done performance testing with loopback mounts (ie server and client on the same machine, mounting over the "lo" interface)?  As I recall this was the test case that Trond found worked poorly when we boosted the tcp_slot_table_entries setting to default to a 64 entry slot table.
> 
> No, not yet. I'll add this to the planned testing.  So having way too many default rpc_slots is bad? I could test by starting with a default of 2 slots and seeing how many are needed.

Apparently there was a problem with having too many slots, but I was never clear what that problem might be.  The report was only poor performance on loopback NFS mounts.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [RFC 0/1] SUNRPC: dynamic rpc_slot allocator for TCP

[Andy Adamson]

On May 3, 2011, at 4:20 PM, Chuck Lever wrote:

> 
> On May 3, 2011, at 4:13 PM, Andy Adamson wrote:
> 
>> 
>> On May 3, 2011, at 4:06 PM, Chuck Lever wrote:
>> 
>>> Hi-
>>> 
>>> On May 2, 2011, at 9:40 PM, andros@netapp.com wrote:
>>> 
>>>> I would appreciate comments on this patch. I've done a small amount of testing
>>>> where I've set the /proc/sys/sunrpc/tcp_slot_table_entries to 2, and watched
>>>> the dynamic allocator increase it to 3. I have a 10G test system being
>>>> configured.
>>> 
>>> For TCP transports, then, the effective slot table size varies between the tcp_slot_table_entries setting and the estimated number of RPCs that can fit in the TCP window?  
>> 
>> Yes.
>> 
>>> What happens when the TCP window shrinks such that fewer RPCs can fit than are allowed by the tcp_slot_table_entries setting?
>> 
>> We don't shrink the rpc_slot table size. The idea being that if the TCP hits size X then shrinks to size X-n, there is a good chance that it will get back to size X. In the worst case, this is still better than a hard configured 'to large' slot table size.
> 
> Seems to me that we wouldn't need the tcp_slot_table_entries setting at all, then.  Always start at 2 and it will adjust itself as needed.  

Yep - for TCP. UDP and RDMA will still use just the default.  I hope to determine a new reasonable  tcp default with testing.

> Unless we find we need a hard upper bound in some cases.

This is also a goal for testing - adding more and more network delay in a 10G test environment to increase the TCP window.  We may upper bound it by looking at available memory.

> 
>>> Have you done performance testing with loopback mounts (ie server and client on the same machine, mounting over the "lo" interface)?  As I recall this was the test case that Trond found worked poorly when we boosted the tcp_slot_table_entries setting to default to a 64 entry slot table.
>> 
>> No, not yet. I'll add this to the planned testing.  So having way too many default rpc_slots is bad? I could test by starting with a default of 2 slots and seeing how many are needed.
> 
> Apparently there was a problem with having too many slots, but I was never clear what that problem might be.  The report was only poor performance on loopback NFS mounts.

OK. I'll check it out.

-->Andy

> 
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
>