LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH v2 0/6] 9p: Use IDRs more effectively
@ 2018-07-11 21:02 Matthew Wilcox
  2018-07-11 21:02 ` [PATCH v2 1/6] 9p: Fix comment on smp_wmb Matthew Wilcox
                   ` (6 more replies)
  0 siblings, 7 replies; 23+ messages in thread
From: Matthew Wilcox @ 2018-07-11 21:02 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Matthew Wilcox, v9fs-developer, Latchesar Ionkov,
	Eric Van Hensbergen, Ron Minnich, linux-kernel, linux-fsdevel

The 9p code doesn't take advantage of the IDR's ability to store
a pointer.  We can actually get rid of the p9_idpool abstraction
and the multi-dimensional array of requests.

v2: Address feedback from Dominique.

Matthew Wilcox (6):
  9p: Fix comment on smp_wmb
  9p: Change p9_fid_create calling convention
  9p: Replace the fidlist with an IDR
  9p: Embed wait_queue_head into p9_req_t
  9p: Use a slab for allocating requests
  9p: Remove p9_idpool

 include/net/9p/9p.h     |   8 -
 include/net/9p/client.h |  62 ++------
 net/9p/Makefile         |   1 -
 net/9p/client.c         | 319 ++++++++++++++--------------------------
 net/9p/mod.c            |   7 +-
 net/9p/trans_virtio.c   |   2 +-
 net/9p/util.c           | 141 ------------------
 7 files changed, 133 insertions(+), 407 deletions(-)
 delete mode 100644 net/9p/util.c

-- 
2.18.0


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 1/6] 9p: Fix comment on smp_wmb
  2018-07-11 21:02 [PATCH v2 0/6] 9p: Use IDRs more effectively Matthew Wilcox
@ 2018-07-11 21:02 ` Matthew Wilcox
  2018-07-12 11:55   ` [V9fs-developer] " Greg Kurz
  2018-07-11 21:02 ` [PATCH v2 2/6] 9p: Change p9_fid_create calling convention Matthew Wilcox
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 23+ messages in thread
From: Matthew Wilcox @ 2018-07-11 21:02 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Matthew Wilcox, v9fs-developer, Latchesar Ionkov,
	Eric Van Hensbergen, Ron Minnich, linux-kernel, linux-fsdevel

The previous comment misled me into thinking the barrier wasn't needed
at all.

Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 net/9p/client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index 18c5271910dc..999eceb8af98 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -447,7 +447,7 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status)
 
 	/*
 	 * This barrier is needed to make sure any change made to req before
-	 * the other thread wakes up will indeed be seen by the waiting side.
+	 * the status change is visible to another thread
 	 */
 	smp_wmb();
 	req->status = status;
-- 
2.18.0


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 2/6] 9p: Change p9_fid_create calling convention
  2018-07-11 21:02 [PATCH v2 0/6] 9p: Use IDRs more effectively Matthew Wilcox
  2018-07-11 21:02 ` [PATCH v2 1/6] 9p: Fix comment on smp_wmb Matthew Wilcox
@ 2018-07-11 21:02 ` Matthew Wilcox
  2018-07-12  2:15   ` [V9fs-developer] " piaojun
                     ` (2 more replies)
  2018-07-11 21:02 ` [PATCH v2 3/6] 9p: Replace the fidlist with an IDR Matthew Wilcox
                   ` (4 subsequent siblings)
  6 siblings, 3 replies; 23+ messages in thread
From: Matthew Wilcox @ 2018-07-11 21:02 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Matthew Wilcox, v9fs-developer, Latchesar Ionkov,
	Eric Van Hensbergen, Ron Minnich, linux-kernel, linux-fsdevel

Return NULL instead of ERR_PTR when we can't allocate a FID.  The ENOSPC
return value was getting all the way back to userspace, and that's
confusing for a userspace program which isn't expecting read() to tell it
there's no space left on the filesystem.  The best error we can return to
indicate a temporary failure caused by lack of client resources is ENOMEM.

Maybe it would be better to sleep until a FID is available, but that's
not a change I'm comfortable making.

Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 net/9p/client.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index 999eceb8af98..389a2904b7b3 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -913,13 +913,11 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
 	p9_debug(P9_DEBUG_FID, "clnt %p\n", clnt);
 	fid = kmalloc(sizeof(struct p9_fid), GFP_KERNEL);
 	if (!fid)
-		return ERR_PTR(-ENOMEM);
+		return NULL;
 
 	ret = p9_idpool_get(clnt->fidpool);
-	if (ret < 0) {
-		ret = -ENOSPC;
+	if (ret < 0)
 		goto error;
-	}
 	fid->fid = ret;
 
 	memset(&fid->qid, 0, sizeof(struct p9_qid));
@@ -935,7 +933,7 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
 
 error:
 	kfree(fid);
-	return ERR_PTR(ret);
+	return NULL;
 }
 
 static void p9_fid_destroy(struct p9_fid *fid)
@@ -1137,9 +1135,8 @@ struct p9_fid *p9_client_attach(struct p9_client *clnt, struct p9_fid *afid,
 	p9_debug(P9_DEBUG_9P, ">>> TATTACH afid %d uname %s aname %s\n",
 		 afid ? afid->fid : -1, uname, aname);
 	fid = p9_fid_create(clnt);
-	if (IS_ERR(fid)) {
-		err = PTR_ERR(fid);
-		fid = NULL;
+	if (!fid) {
+		err = -ENOMEM;
 		goto error;
 	}
 	fid->uid = n_uname;
@@ -1188,9 +1185,8 @@ struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname,
 	clnt = oldfid->clnt;
 	if (clone) {
 		fid = p9_fid_create(clnt);
-		if (IS_ERR(fid)) {
-			err = PTR_ERR(fid);
-			fid = NULL;
+		if (!fid) {
+			err = -ENOMEM;
 			goto error;
 		}
 
@@ -2018,9 +2014,8 @@ struct p9_fid *p9_client_xattrwalk(struct p9_fid *file_fid,
 	err = 0;
 	clnt = file_fid->clnt;
 	attr_fid = p9_fid_create(clnt);
-	if (IS_ERR(attr_fid)) {
-		err = PTR_ERR(attr_fid);
-		attr_fid = NULL;
+	if (!attr_fid) {
+		err = -ENOMEM;
 		goto error;
 	}
 	p9_debug(P9_DEBUG_9P,
-- 
2.18.0


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 3/6] 9p: Replace the fidlist with an IDR
  2018-07-11 21:02 [PATCH v2 0/6] 9p: Use IDRs more effectively Matthew Wilcox
  2018-07-11 21:02 ` [PATCH v2 1/6] 9p: Fix comment on smp_wmb Matthew Wilcox
  2018-07-11 21:02 ` [PATCH v2 2/6] 9p: Change p9_fid_create calling convention Matthew Wilcox
@ 2018-07-11 21:02 ` Matthew Wilcox
  2018-07-12 11:17   ` Dominique Martinet
  2018-07-13  2:05   ` [V9fs-developer] " jiangyiwen
  2018-07-11 21:02 ` [PATCH v2 4/6] 9p: Embed wait_queue_head into p9_req_t Matthew Wilcox
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 23+ messages in thread
From: Matthew Wilcox @ 2018-07-11 21:02 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Matthew Wilcox, v9fs-developer, Latchesar Ionkov,
	Eric Van Hensbergen, Ron Minnich, linux-kernel, linux-fsdevel

The p9_idpool being used to allocate the IDs uses an IDR to allocate
the IDs ... which we then keep in a doubly-linked list, rather than in
the IDR which allocated them.  We can use an IDR directly which saves
two pointers per p9_fid, and a tiny memory allocation per p9_client.

Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 include/net/9p/client.h |  9 +++------
 net/9p/client.c         | 44 +++++++++++++++--------------------------
 2 files changed, 19 insertions(+), 34 deletions(-)

diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index 7af9d769b97d..e405729cd1c7 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -27,6 +27,7 @@
 #define NET_9P_CLIENT_H
 
 #include <linux/utsname.h>
+#include <linux/idr.h>
 
 /* Number of requests per row */
 #define P9_ROW_MAXTAG 255
@@ -128,8 +129,7 @@ struct p9_req_t {
  * @proto_version: 9P protocol version to use
  * @trans_mod: module API instantiated with this client
  * @trans: tranport instance state and API
- * @fidpool: fid handle accounting for session
- * @fidlist: List of active fid handles
+ * @fids: All active FID handles
  * @tagpool - transaction id accounting for session
  * @reqs - 2D array of requests
  * @max_tag - current maximum tag id allocated
@@ -169,8 +169,7 @@ struct p9_client {
 		} tcp;
 	} trans_opts;
 
-	struct p9_idpool *fidpool;
-	struct list_head fidlist;
+	struct idr fids;
 
 	struct p9_idpool *tagpool;
 	struct p9_req_t *reqs[P9_ROW_MAXTAG];
@@ -188,7 +187,6 @@ struct p9_client {
  * @iounit: the server reported maximum transaction size for this file
  * @uid: the numeric uid of the local user who owns this handle
  * @rdir: readdir accounting structure (allocated on demand)
- * @flist: per-client-instance fid tracking
  * @dlist: per-dentry fid tracking
  *
  * TODO: This needs lots of explanation.
@@ -204,7 +202,6 @@ struct p9_fid {
 
 	void *rdir;
 
-	struct list_head flist;
 	struct hlist_node dlist;	/* list of all fids attached to a dentry */
 };
 
diff --git a/net/9p/client.c b/net/9p/client.c
index 389a2904b7b3..b89c7298267c 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -908,30 +908,29 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
 {
 	int ret;
 	struct p9_fid *fid;
-	unsigned long flags;
 
 	p9_debug(P9_DEBUG_FID, "clnt %p\n", clnt);
 	fid = kmalloc(sizeof(struct p9_fid), GFP_KERNEL);
 	if (!fid)
 		return NULL;
 
-	ret = p9_idpool_get(clnt->fidpool);
-	if (ret < 0)
-		goto error;
-	fid->fid = ret;
-
 	memset(&fid->qid, 0, sizeof(struct p9_qid));
 	fid->mode = -1;
 	fid->uid = current_fsuid();
 	fid->clnt = clnt;
 	fid->rdir = NULL;
-	spin_lock_irqsave(&clnt->lock, flags);
-	list_add(&fid->flist, &clnt->fidlist);
-	spin_unlock_irqrestore(&clnt->lock, flags);
+	fid->fid = 0;
 
-	return fid;
+	idr_preload(GFP_KERNEL);
+	spin_lock_irq(&clnt->lock);
+	ret = idr_alloc_u32(&clnt->fids, fid, &fid->fid, P9_NOFID - 1,
+			GFP_NOWAIT);
+	spin_unlock_irq(&clnt->lock);
+	idr_preload_end();
+
+	if (!ret)
+		return fid;
 
-error:
 	kfree(fid);
 	return NULL;
 }
@@ -943,9 +942,8 @@ static void p9_fid_destroy(struct p9_fid *fid)
 
 	p9_debug(P9_DEBUG_FID, "fid %d\n", fid->fid);
 	clnt = fid->clnt;
-	p9_idpool_put(fid->fid, clnt->fidpool);
 	spin_lock_irqsave(&clnt->lock, flags);
-	list_del(&fid->flist);
+	idr_remove(&clnt->fids, fid->fid);
 	spin_unlock_irqrestore(&clnt->lock, flags);
 	kfree(fid->rdir);
 	kfree(fid);
@@ -1028,7 +1026,7 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
 	memcpy(clnt->name, client_id, strlen(client_id) + 1);
 
 	spin_lock_init(&clnt->lock);
-	INIT_LIST_HEAD(&clnt->fidlist);
+	idr_init(&clnt->fids);
 
 	err = p9_tag_init(clnt);
 	if (err < 0)
@@ -1048,18 +1046,12 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
 		goto destroy_tagpool;
 	}
 
-	clnt->fidpool = p9_idpool_create();
-	if (IS_ERR(clnt->fidpool)) {
-		err = PTR_ERR(clnt->fidpool);
-		goto put_trans;
-	}
-
 	p9_debug(P9_DEBUG_MUX, "clnt %p trans %p msize %d protocol %d\n",
 		 clnt, clnt->trans_mod, clnt->msize, clnt->proto_version);
 
 	err = clnt->trans_mod->create(clnt, dev_name, options);
 	if (err)
-		goto destroy_fidpool;
+		goto put_trans;
 
 	if (clnt->msize > clnt->trans_mod->maxsize)
 		clnt->msize = clnt->trans_mod->maxsize;
@@ -1072,8 +1064,6 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
 
 close_trans:
 	clnt->trans_mod->close(clnt);
-destroy_fidpool:
-	p9_idpool_destroy(clnt->fidpool);
 put_trans:
 	v9fs_put_trans(clnt->trans_mod);
 destroy_tagpool:
@@ -1086,7 +1076,8 @@ EXPORT_SYMBOL(p9_client_create);
 
 void p9_client_destroy(struct p9_client *clnt)
 {
-	struct p9_fid *fid, *fidptr;
+	struct p9_fid *fid;
+	int id;
 
 	p9_debug(P9_DEBUG_MUX, "clnt %p\n", clnt);
 
@@ -1095,14 +1086,11 @@ void p9_client_destroy(struct p9_client *clnt)
 
 	v9fs_put_trans(clnt->trans_mod);
 
-	list_for_each_entry_safe(fid, fidptr, &clnt->fidlist, flist) {
+	idr_for_each_entry(&clnt->fids, fid, id) {
 		pr_info("Found fid %d not clunked\n", fid->fid);
 		p9_fid_destroy(fid);
 	}
 
-	if (clnt->fidpool)
-		p9_idpool_destroy(clnt->fidpool);
-
 	p9_tag_cleanup(clnt);
 
 	kfree(clnt);
-- 
2.18.0


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 4/6] 9p: Embed wait_queue_head into p9_req_t
  2018-07-11 21:02 [PATCH v2 0/6] 9p: Use IDRs more effectively Matthew Wilcox
                   ` (2 preceding siblings ...)
  2018-07-11 21:02 ` [PATCH v2 3/6] 9p: Replace the fidlist with an IDR Matthew Wilcox
@ 2018-07-11 21:02 ` Matthew Wilcox
  2018-07-12 14:36   ` [V9fs-developer] " Greg Kurz
  2018-07-11 21:02 ` [PATCH v2 5/6] 9p: Use a slab for allocating requests Matthew Wilcox
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 23+ messages in thread
From: Matthew Wilcox @ 2018-07-11 21:02 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Matthew Wilcox, v9fs-developer, Latchesar Ionkov,
	Eric Van Hensbergen, Ron Minnich, linux-kernel, linux-fsdevel

On a 64-bit system, the wait_queue_head_t is 24 bytes while the pointer
to it is 8 bytes.  Growing the p9_req_t by 16 bytes is better than
performing a 24-byte memory allocation.

Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 include/net/9p/client.h |  2 +-
 net/9p/client.c         | 19 +++++--------------
 net/9p/trans_virtio.c   |  2 +-
 3 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index e405729cd1c7..0fa0fbab33b0 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -113,7 +113,7 @@ enum p9_req_status_t {
 struct p9_req_t {
 	int status;
 	int t_err;
-	wait_queue_head_t *wq;
+	wait_queue_head_t wq;
 	struct p9_fcall *tc;
 	struct p9_fcall *rc;
 	void *aux;
diff --git a/net/9p/client.c b/net/9p/client.c
index b89c7298267c..bc8aba6b5ce0 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -282,8 +282,9 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
 				return ERR_PTR(-ENOMEM);
 			}
 			for (col = 0; col < P9_ROW_MAXTAG; col++) {
-				c->reqs[row][col].status = REQ_STATUS_IDLE;
-				c->reqs[row][col].tc = NULL;
+				req = &c->reqs[row][col];
+				req->status = REQ_STATUS_IDLE;
+				init_waitqueue_head(&req->wq);
 			}
 			c->max_tag += P9_ROW_MAXTAG;
 		}
@@ -293,13 +294,6 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
 	col = tag % P9_ROW_MAXTAG;
 
 	req = &c->reqs[row][col];
-	if (!req->wq) {
-		req->wq = kmalloc(sizeof(wait_queue_head_t), GFP_NOFS);
-		if (!req->wq)
-			goto grow_failed;
-		init_waitqueue_head(req->wq);
-	}
-
 	if (!req->tc)
 		req->tc = p9_fcall_alloc(alloc_msize);
 	if (!req->rc)
@@ -319,9 +313,7 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
 	pr_err("Couldn't grow tag array\n");
 	kfree(req->tc);
 	kfree(req->rc);
-	kfree(req->wq);
 	req->tc = req->rc = NULL;
-	req->wq = NULL;
 	return ERR_PTR(-ENOMEM);
 }
 
@@ -409,7 +401,6 @@ static void p9_tag_cleanup(struct p9_client *c)
 	/* free requests associated with tags */
 	for (row = 0; row < (c->max_tag/P9_ROW_MAXTAG); row++) {
 		for (col = 0; col < P9_ROW_MAXTAG; col++) {
-			kfree(c->reqs[row][col].wq);
 			kfree(c->reqs[row][col].tc);
 			kfree(c->reqs[row][col].rc);
 		}
@@ -452,7 +443,7 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status)
 	smp_wmb();
 	req->status = status;
 
-	wake_up(req->wq);
+	wake_up(&req->wq);
 	p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc->tag);
 }
 EXPORT_SYMBOL(p9_client_cb);
@@ -773,7 +764,7 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
 	}
 again:
 	/* Wait for the response */
-	err = wait_event_killable(*req->wq, req->status >= REQ_STATUS_RCVD);
+	err = wait_event_killable(req->wq, req->status >= REQ_STATUS_RCVD);
 
 	/*
 	 * Make sure our req is coherent with regard to updates in other
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 05006cbb3361..3e096c98313c 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -490,7 +490,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 	virtqueue_kick(chan->vq);
 	spin_unlock_irqrestore(&chan->lock, flags);
 	p9_debug(P9_DEBUG_TRANS, "virtio request kicked\n");
-	err = wait_event_killable(*req->wq, req->status >= REQ_STATUS_RCVD);
+	err = wait_event_killable(req->wq, req->status >= REQ_STATUS_RCVD);
 	/*
 	 * Non kernel buffers are pinned, unpin them
 	 */
-- 
2.18.0


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 5/6] 9p: Use a slab for allocating requests
  2018-07-11 21:02 [PATCH v2 0/6] 9p: Use IDRs more effectively Matthew Wilcox
                   ` (3 preceding siblings ...)
  2018-07-11 21:02 ` [PATCH v2 4/6] 9p: Embed wait_queue_head into p9_req_t Matthew Wilcox
@ 2018-07-11 21:02 ` Matthew Wilcox
  2018-07-18 10:05   ` Dominique Martinet
  2018-07-11 21:02 ` [PATCH v2 6/6] 9p: Remove p9_idpool Matthew Wilcox
  2018-07-11 23:37 ` [PATCH v2 0/6] 9p: Use IDRs more effectively Dominique Martinet
  6 siblings, 1 reply; 23+ messages in thread
From: Matthew Wilcox @ 2018-07-11 21:02 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Matthew Wilcox, v9fs-developer, Latchesar Ionkov,
	Eric Van Hensbergen, Ron Minnich, linux-kernel, linux-fsdevel

Replace the custom batch allocation with a slab.  Use an IDR to store
pointers to the active requests instead of an array.  We don't try to
handle P9_NOTAG specially; the IDR will happily shrink all the way back
once the TVERSION call has completed.

Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 include/net/9p/client.h |  51 ++-------
 net/9p/client.c         | 239 +++++++++++++++-------------------------
 net/9p/mod.c            |   7 +-
 3 files changed, 101 insertions(+), 196 deletions(-)

diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index 0fa0fbab33b0..a4dc42c53d18 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -64,22 +64,15 @@ enum p9_trans_status {
 
 /**
  * enum p9_req_status_t - status of a request
- * @REQ_STATUS_IDLE: request slot unused
  * @REQ_STATUS_ALLOC: request has been allocated but not sent
  * @REQ_STATUS_UNSENT: request waiting to be sent
  * @REQ_STATUS_SENT: request sent to server
  * @REQ_STATUS_RCVD: response received from server
  * @REQ_STATUS_FLSHD: request has been flushed
  * @REQ_STATUS_ERROR: request encountered an error on the client side
- *
- * The @REQ_STATUS_IDLE state is used to mark a request slot as unused
- * but use is actually tracked by the idpool structure which handles tag
- * id allocation.
- *
  */
 
 enum p9_req_status_t {
-	REQ_STATUS_IDLE,
 	REQ_STATUS_ALLOC,
 	REQ_STATUS_UNSENT,
 	REQ_STATUS_SENT,
@@ -92,24 +85,12 @@ enum p9_req_status_t {
  * struct p9_req_t - request slots
  * @status: status of this request slot
  * @t_err: transport error
- * @flush_tag: tag of request being flushed (for flush requests)
  * @wq: wait_queue for the client to block on for this request
  * @tc: the request fcall structure
  * @rc: the response fcall structure
  * @aux: transport specific data (provided for trans_fd migration)
  * @req_list: link for higher level objects to chain requests
- *
- * Transport use an array to track outstanding requests
- * instead of a list.  While this may incurr overhead during initial
- * allocation or expansion, it makes request lookup much easier as the
- * tag id is a index into an array.  (We use tag+1 so that we can accommodate
- * the -1 tag for the T_VERSION request).
- * This also has the nice effect of only having to allocate wait_queues
- * once, instead of constantly allocating and freeing them.  Its possible
- * other resources could benefit from this scheme as well.
- *
  */
-
 struct p9_req_t {
 	int status;
 	int t_err;
@@ -117,40 +98,26 @@ struct p9_req_t {
 	struct p9_fcall *tc;
 	struct p9_fcall *rc;
 	void *aux;
-
 	struct list_head req_list;
 };
 
 /**
  * struct p9_client - per client instance state
- * @lock: protect @fidlist
+ * @lock: protect @fids and @reqs
  * @msize: maximum data size negotiated by protocol
- * @dotu: extension flags negotiated by protocol
  * @proto_version: 9P protocol version to use
  * @trans_mod: module API instantiated with this client
+ * @status: connection state
  * @trans: tranport instance state and API
  * @fids: All active FID handles
- * @tagpool - transaction id accounting for session
- * @reqs - 2D array of requests
- * @max_tag - current maximum tag id allocated
- * @name - node name used as client id
+ * @reqs: All active requests.
+ * @name: node name used as client id
  *
  * The client structure is used to keep track of various per-client
  * state that has been instantiated.
- * In order to minimize per-transaction overhead we use a
- * simple array to lookup requests instead of a hash table
- * or linked list.  In order to support larger number of
- * transactions, we make this a 2D array, allocating new rows
- * when we need to grow the total number of the transactions.
- *
- * Each row is 256 requests and we'll support up to 256 rows for
- * a total of 64k concurrent requests per session.
- *
- * Bugs: duplicated data and potentially unnecessary elements.
  */
-
 struct p9_client {
-	spinlock_t lock; /* protect client structure */
+	spinlock_t lock;
 	unsigned int msize;
 	unsigned char proto_version;
 	struct p9_trans_module *trans_mod;
@@ -170,10 +137,7 @@ struct p9_client {
 	} trans_opts;
 
 	struct idr fids;
-
-	struct p9_idpool *tagpool;
-	struct p9_req_t *reqs[P9_ROW_MAXTAG];
-	int max_tag;
+	struct idr reqs;
 
 	char name[__NEW_UTS_LEN + 1];
 };
@@ -279,4 +243,7 @@ struct p9_fid *p9_client_xattrwalk(struct p9_fid *, const char *, u64 *);
 int p9_client_xattrcreate(struct p9_fid *, const char *, u64, int);
 int p9_client_readlink(struct p9_fid *fid, char **target);
 
+int p9_client_init(void);
+void p9_client_exit(void);
+
 #endif /* NET_9P_CLIENT_H */
diff --git a/net/9p/client.c b/net/9p/client.c
index bc8aba6b5ce0..ecf5dd269f4c 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -241,132 +241,103 @@ static struct p9_fcall *p9_fcall_alloc(int alloc_msize)
 	return fc;
 }
 
+static struct kmem_cache *p9_req_cache;
+
 /**
- * p9_tag_alloc - lookup/allocate a request by tag
- * @c: client session to lookup tag within
- * @tag: numeric id for transaction
- *
- * this is a simple array lookup, but will grow the
- * request_slots as necessary to accommodate transaction
- * ids which did not previously have a slot.
- *
- * this code relies on the client spinlock to manage locks, its
- * possible we should switch to something else, but I'd rather
- * stick with something low-overhead for the common case.
+ * p9_req_alloc - Allocate a new request.
+ * @c: Client session.
+ * @type: Transaction type.
+ * @max_size: Maximum packet size for this request.
  *
+ * Context: Process context.
+ * Return: Pointer to new request.
  */
-
 static struct p9_req_t *
-p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
+p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
 {
-	unsigned long flags;
-	int row, col;
-	struct p9_req_t *req;
+	struct p9_req_t *req = kmem_cache_alloc(p9_req_cache, GFP_NOFS);
 	int alloc_msize = min(c->msize, max_size);
+	int tag;
 
-	/* This looks up the original request by tag so we know which
-	 * buffer to read the data into */
-	tag++;
-
-	if (tag >= c->max_tag) {
-		spin_lock_irqsave(&c->lock, flags);
-		/* check again since original check was outside of lock */
-		while (tag >= c->max_tag) {
-			row = (tag / P9_ROW_MAXTAG);
-			c->reqs[row] = kcalloc(P9_ROW_MAXTAG,
-					sizeof(struct p9_req_t), GFP_ATOMIC);
-
-			if (!c->reqs[row]) {
-				pr_err("Couldn't grow tag array\n");
-				spin_unlock_irqrestore(&c->lock, flags);
-				return ERR_PTR(-ENOMEM);
-			}
-			for (col = 0; col < P9_ROW_MAXTAG; col++) {
-				req = &c->reqs[row][col];
-				req->status = REQ_STATUS_IDLE;
-				init_waitqueue_head(&req->wq);
-			}
-			c->max_tag += P9_ROW_MAXTAG;
-		}
-		spin_unlock_irqrestore(&c->lock, flags);
-	}
-	row = tag / P9_ROW_MAXTAG;
-	col = tag % P9_ROW_MAXTAG;
+	if (!req)
+		return NULL;
 
-	req = &c->reqs[row][col];
-	if (!req->tc)
-		req->tc = p9_fcall_alloc(alloc_msize);
-	if (!req->rc)
-		req->rc = p9_fcall_alloc(alloc_msize);
+	req->tc = p9_fcall_alloc(alloc_msize);
+	req->rc = p9_fcall_alloc(alloc_msize);
 	if (!req->tc || !req->rc)
-		goto grow_failed;
+		goto free;
 
 	p9pdu_reset(req->tc);
 	p9pdu_reset(req->rc);
-
-	req->tc->tag = tag-1;
 	req->status = REQ_STATUS_ALLOC;
+	init_waitqueue_head(&req->wq);
+	INIT_LIST_HEAD(&req->req_list);
+
+	idr_preload(GFP_NOFS);
+	spin_lock_irq(&c->lock);
+	if (type == P9_TVERSION)
+		tag = idr_alloc(&c->reqs, req, P9_NOTAG, P9_NOTAG + 1,
+				GFP_NOWAIT);
+	else
+		tag = idr_alloc(&c->reqs, req, 0, P9_NOTAG, GFP_NOWAIT);
+	req->tc->tag = tag;
+	spin_unlock_irq(&c->lock);
+	idr_preload_end();
+	if (tag < 0)
+		goto free;
 
 	return req;
 
-grow_failed:
-	pr_err("Couldn't grow tag array\n");
+free:
 	kfree(req->tc);
 	kfree(req->rc);
-	req->tc = req->rc = NULL;
+	kmem_cache_free(p9_req_cache, req);
 	return ERR_PTR(-ENOMEM);
 }
 
 /**
- * p9_tag_lookup - lookup a request by tag
- * @c: client session to lookup tag within
- * @tag: numeric id for transaction
+ * p9_tag_lookup - Look up a request by tag.
+ * @c: Client session.
+ * @tag: Transaction ID.
  *
+ * Context: Any context.
+ * Return: A request, or %NULL if there is no request with that tag.
  */
-
 struct p9_req_t *p9_tag_lookup(struct p9_client *c, u16 tag)
 {
-	int row, col;
-
-	/* This looks up the original request by tag so we know which
-	 * buffer to read the data into */
-	tag++;
-
-	if(tag >= c->max_tag) 
-		return NULL;
+	struct p9_req_t *req;
 
-	row = tag / P9_ROW_MAXTAG;
-	col = tag % P9_ROW_MAXTAG;
+	rcu_read_lock();
+	req = idr_find(&c->reqs, tag);
+	/*
+	 * There's no refcount on the req; a malicious server could cause
+	 * us to dereference a NULL pointer
+	 */
+	rcu_read_unlock();
 
-	return &c->reqs[row][col];
+	return req;
 }
 EXPORT_SYMBOL(p9_tag_lookup);
 
 /**
- * p9_tag_init - setup tags structure and contents
- * @c:  v9fs client struct
- *
- * This initializes the tags structure for each client instance.
+ * p9_free_req - Free a request.
+ * @c: Client session.
+ * @r: Request to free.
  *
+ * Context: Any context.
  */
-
-static int p9_tag_init(struct p9_client *c)
+static void p9_free_req(struct p9_client *c, struct p9_req_t *r)
 {
-	int err = 0;
+	unsigned long flags;
+	u16 tag = r->tc->tag;
 
-	c->tagpool = p9_idpool_create();
-	if (IS_ERR(c->tagpool)) {
-		err = PTR_ERR(c->tagpool);
-		goto error;
-	}
-	err = p9_idpool_get(c->tagpool); /* reserve tag 0 */
-	if (err < 0) {
-		p9_idpool_destroy(c->tagpool);
-		goto error;
-	}
-	c->max_tag = 0;
-error:
-	return err;
+	p9_debug(P9_DEBUG_MUX, "clnt %p req %p tag: %d\n", c, r, tag);
+	spin_lock_irqsave(&c->lock, flags);
+	idr_remove(&c->reqs, tag);
+	spin_unlock_irqrestore(&c->lock, flags);
+	kfree(r->tc);
+	kfree(r->rc);
+	kmem_cache_free(p9_req_cache, r);
 }
 
 /**
@@ -378,52 +349,15 @@ static int p9_tag_init(struct p9_client *c)
  */
 static void p9_tag_cleanup(struct p9_client *c)
 {
-	int row, col;
-
-	/* check to insure all requests are idle */
-	for (row = 0; row < (c->max_tag/P9_ROW_MAXTAG); row++) {
-		for (col = 0; col < P9_ROW_MAXTAG; col++) {
-			if (c->reqs[row][col].status != REQ_STATUS_IDLE) {
-				p9_debug(P9_DEBUG_MUX,
-					 "Attempting to cleanup non-free tag %d,%d\n",
-					 row, col);
-				/* TODO: delay execution of cleanup */
-				return;
-			}
-		}
-	}
-
-	if (c->tagpool) {
-		p9_idpool_put(0, c->tagpool); /* free reserved tag 0 */
-		p9_idpool_destroy(c->tagpool);
-	}
+	struct p9_req_t *req;
+	int id;
 
-	/* free requests associated with tags */
-	for (row = 0; row < (c->max_tag/P9_ROW_MAXTAG); row++) {
-		for (col = 0; col < P9_ROW_MAXTAG; col++) {
-			kfree(c->reqs[row][col].tc);
-			kfree(c->reqs[row][col].rc);
-		}
-		kfree(c->reqs[row]);
+	rcu_read_lock();
+	idr_for_each_entry(&c->reqs, req, id) {
+		pr_info("Tag %d still in use\n", id);
+		p9_free_req(c, req);
 	}
-	c->max_tag = 0;
-}
-
-/**
- * p9_free_req - free a request and clean-up as necessary
- * c: client state
- * r: request to release
- *
- */
-
-static void p9_free_req(struct p9_client *c, struct p9_req_t *r)
-{
-	int tag = r->tc->tag;
-	p9_debug(P9_DEBUG_MUX, "clnt %p req %p tag: %d\n", c, r, tag);
-
-	r->status = REQ_STATUS_IDLE;
-	if (tag != P9_NOTAG && p9_idpool_check(tag, c->tagpool))
-		p9_idpool_put(tag, c->tagpool);
+	rcu_read_unlock();
 }
 
 /**
@@ -690,7 +624,7 @@ static struct p9_req_t *p9_client_prepare_req(struct p9_client *c,
 					      int8_t type, int req_size,
 					      const char *fmt, va_list ap)
 {
-	int tag, err;
+	int err;
 	struct p9_req_t *req;
 
 	p9_debug(P9_DEBUG_MUX, "client %p op %d\n", c, type);
@@ -703,24 +637,17 @@ static struct p9_req_t *p9_client_prepare_req(struct p9_client *c,
 	if ((c->status == BeginDisconnect) && (type != P9_TCLUNK))
 		return ERR_PTR(-EIO);
 
-	tag = P9_NOTAG;
-	if (type != P9_TVERSION) {
-		tag = p9_idpool_get(c->tagpool);
-		if (tag < 0)
-			return ERR_PTR(-ENOMEM);
-	}
-
-	req = p9_tag_alloc(c, tag, req_size);
+	req = p9_tag_alloc(c, type, req_size);
 	if (IS_ERR(req))
 		return req;
 
 	/* marshall the data */
-	p9pdu_prepare(req->tc, tag, type);
+	p9pdu_prepare(req->tc, req->tc->tag, type);
 	err = p9pdu_vwritef(req->tc, c->proto_version, fmt, ap);
 	if (err)
 		goto reterr;
 	p9pdu_finalize(c, req->tc);
-	trace_9p_client_req(c, type, tag);
+	trace_9p_client_req(c, type, req->tc->tag);
 	return req;
 reterr:
 	p9_free_req(c, req);
@@ -1018,14 +945,11 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
 
 	spin_lock_init(&clnt->lock);
 	idr_init(&clnt->fids);
-
-	err = p9_tag_init(clnt);
-	if (err < 0)
-		goto free_client;
+	idr_init(&clnt->reqs);
 
 	err = parse_opts(options, clnt);
 	if (err < 0)
-		goto destroy_tagpool;
+		goto free_client;
 
 	if (!clnt->trans_mod)
 		clnt->trans_mod = v9fs_get_default_trans();
@@ -1034,7 +958,7 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
 		err = -EPROTONOSUPPORT;
 		p9_debug(P9_DEBUG_ERROR,
 			 "No transport defined or default transport\n");
-		goto destroy_tagpool;
+		goto free_client;
 	}
 
 	p9_debug(P9_DEBUG_MUX, "clnt %p trans %p msize %d protocol %d\n",
@@ -1057,8 +981,6 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
 	clnt->trans_mod->close(clnt);
 put_trans:
 	v9fs_put_trans(clnt->trans_mod);
-destroy_tagpool:
-	p9_idpool_destroy(clnt->tagpool);
 free_client:
 	kfree(clnt);
 	return ERR_PTR(err);
@@ -2274,3 +2196,14 @@ int p9_client_readlink(struct p9_fid *fid, char **target)
 	return err;
 }
 EXPORT_SYMBOL(p9_client_readlink);
+
+int __init p9_client_init(void)
+{
+	p9_req_cache = KMEM_CACHE(p9_req_t, 0);
+	return p9_req_cache ? 0 : -ENOMEM;
+}
+
+void __exit p9_client_exit(void)
+{
+	kmem_cache_destroy(p9_req_cache);
+}
diff --git a/net/9p/mod.c b/net/9p/mod.c
index eb9777f05755..0da56d6af73b 100644
--- a/net/9p/mod.c
+++ b/net/9p/mod.c
@@ -171,7 +171,11 @@ void v9fs_put_trans(struct p9_trans_module *m)
  */
 static int __init init_p9(void)
 {
-	int ret = 0;
+	int ret;
+
+	ret = p9_client_init();
+	if (ret)
+		return ret;
 
 	p9_error_init();
 	pr_info("Installing 9P2000 support\n");
@@ -190,6 +194,7 @@ static void __exit exit_p9(void)
 	pr_info("Unloading 9P2000 support\n");
 
 	p9_trans_fd_exit();
+	p9_client_exit();
 }
 
 module_init(init_p9)
-- 
2.18.0


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 6/6] 9p: Remove p9_idpool
  2018-07-11 21:02 [PATCH v2 0/6] 9p: Use IDRs more effectively Matthew Wilcox
                   ` (4 preceding siblings ...)
  2018-07-11 21:02 ` [PATCH v2 5/6] 9p: Use a slab for allocating requests Matthew Wilcox
@ 2018-07-11 21:02 ` Matthew Wilcox
  2018-07-11 23:37 ` [PATCH v2 0/6] 9p: Use IDRs more effectively Dominique Martinet
  6 siblings, 0 replies; 23+ messages in thread
From: Matthew Wilcox @ 2018-07-11 21:02 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Matthew Wilcox, v9fs-developer, Latchesar Ionkov,
	Eric Van Hensbergen, Ron Minnich, linux-kernel, linux-fsdevel

There are no more users left of the p9_idpool; delete it.

Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 include/net/9p/9p.h |   8 ---
 net/9p/Makefile     |   1 -
 net/9p/util.c       | 141 --------------------------------------------
 3 files changed, 150 deletions(-)
 delete mode 100644 net/9p/util.c

diff --git a/include/net/9p/9p.h b/include/net/9p/9p.h
index b8eb51a661e5..e23896116d9a 100644
--- a/include/net/9p/9p.h
+++ b/include/net/9p/9p.h
@@ -561,16 +561,8 @@ struct p9_fcall {
 	u8 *sdata;
 };
 
-struct p9_idpool;
-
 int p9_errstr2errno(char *errstr, int len);
 
-struct p9_idpool *p9_idpool_create(void);
-void p9_idpool_destroy(struct p9_idpool *);
-int p9_idpool_get(struct p9_idpool *p);
-void p9_idpool_put(int id, struct p9_idpool *p);
-int p9_idpool_check(int id, struct p9_idpool *p);
-
 int p9_error_init(void);
 int p9_trans_fd_init(void);
 void p9_trans_fd_exit(void);
diff --git a/net/9p/Makefile b/net/9p/Makefile
index c0486cfc85d9..aa0a5641e5d0 100644
--- a/net/9p/Makefile
+++ b/net/9p/Makefile
@@ -8,7 +8,6 @@ obj-$(CONFIG_NET_9P_RDMA) += 9pnet_rdma.o
 	mod.o \
 	client.o \
 	error.o \
-	util.o \
 	protocol.o \
 	trans_fd.o \
 	trans_common.o \
diff --git a/net/9p/util.c b/net/9p/util.c
deleted file mode 100644
index 59f278e64f58..000000000000
--- a/net/9p/util.c
+++ /dev/null
@@ -1,141 +0,0 @@
-/*
- *  net/9p/util.c
- *
- *  This file contains some helper functions
- *
- *  Copyright (C) 2007 by Latchesar Ionkov <lucho@ionkov.net>
- *  Copyright (C) 2004 by Eric Van Hensbergen <ericvh@gmail.com>
- *  Copyright (C) 2002 by Ron Minnich <rminnich@lanl.gov>
- *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License version 2
- *  as published by the Free Software Foundation.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to:
- *  Free Software Foundation
- *  51 Franklin Street, Fifth Floor
- *  Boston, MA  02111-1301  USA
- *
- */
-
-#include <linux/module.h>
-#include <linux/errno.h>
-#include <linux/fs.h>
-#include <linux/sched.h>
-#include <linux/parser.h>
-#include <linux/idr.h>
-#include <linux/slab.h>
-#include <net/9p/9p.h>
-
-/**
- * struct p9_idpool - per-connection accounting for tag idpool
- * @lock: protects the pool
- * @pool: idr to allocate tag id from
- *
- */
-
-struct p9_idpool {
-	spinlock_t lock;
-	struct idr pool;
-};
-
-/**
- * p9_idpool_create - create a new per-connection id pool
- *
- */
-
-struct p9_idpool *p9_idpool_create(void)
-{
-	struct p9_idpool *p;
-
-	p = kmalloc(sizeof(struct p9_idpool), GFP_KERNEL);
-	if (!p)
-		return ERR_PTR(-ENOMEM);
-
-	spin_lock_init(&p->lock);
-	idr_init(&p->pool);
-
-	return p;
-}
-EXPORT_SYMBOL(p9_idpool_create);
-
-/**
- * p9_idpool_destroy - create a new per-connection id pool
- * @p: idpool to destroy
- */
-
-void p9_idpool_destroy(struct p9_idpool *p)
-{
-	idr_destroy(&p->pool);
-	kfree(p);
-}
-EXPORT_SYMBOL(p9_idpool_destroy);
-
-/**
- * p9_idpool_get - allocate numeric id from pool
- * @p: pool to allocate from
- *
- * Bugs: This seems to be an awful generic function, should it be in idr.c with
- *            the lock included in struct idr?
- */
-
-int p9_idpool_get(struct p9_idpool *p)
-{
-	int i;
-	unsigned long flags;
-
-	idr_preload(GFP_NOFS);
-	spin_lock_irqsave(&p->lock, flags);
-
-	/* no need to store exactly p, we just need something non-null */
-	i = idr_alloc(&p->pool, p, 0, 0, GFP_NOWAIT);
-
-	spin_unlock_irqrestore(&p->lock, flags);
-	idr_preload_end();
-	if (i < 0)
-		return -1;
-
-	p9_debug(P9_DEBUG_MUX, " id %d pool %p\n", i, p);
-	return i;
-}
-EXPORT_SYMBOL(p9_idpool_get);
-
-/**
- * p9_idpool_put - release numeric id from pool
- * @id: numeric id which is being released
- * @p: pool to release id into
- *
- * Bugs: This seems to be an awful generic function, should it be in idr.c with
- *            the lock included in struct idr?
- */
-
-void p9_idpool_put(int id, struct p9_idpool *p)
-{
-	unsigned long flags;
-
-	p9_debug(P9_DEBUG_MUX, " id %d pool %p\n", id, p);
-
-	spin_lock_irqsave(&p->lock, flags);
-	idr_remove(&p->pool, id);
-	spin_unlock_irqrestore(&p->lock, flags);
-}
-EXPORT_SYMBOL(p9_idpool_put);
-
-/**
- * p9_idpool_check - check if the specified id is available
- * @id: id to check
- * @p: pool to check
- */
-
-int p9_idpool_check(int id, struct p9_idpool *p)
-{
-	return idr_find(&p->pool, id) != NULL;
-}
-EXPORT_SYMBOL(p9_idpool_check);
-
-- 
2.18.0


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 0/6] 9p: Use IDRs more effectively
  2018-07-11 21:02 [PATCH v2 0/6] 9p: Use IDRs more effectively Matthew Wilcox
                   ` (5 preceding siblings ...)
  2018-07-11 21:02 ` [PATCH v2 6/6] 9p: Remove p9_idpool Matthew Wilcox
@ 2018-07-11 23:37 ` Dominique Martinet
  6 siblings, 0 replies; 23+ messages in thread
From: Dominique Martinet @ 2018-07-11 23:37 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: v9fs-developer, Latchesar Ionkov, Eric Van Hensbergen,
	Ron Minnich, linux-kernel, linux-fsdevel

Matthew Wilcox wrote on Wed, Jul 11, 2018:
> The 9p code doesn't take advantage of the IDR's ability to store
> a pointer.  We can actually get rid of the p9_idpool abstraction
> and the multi-dimensional array of requests.
> 
> v2: Address feedback from Dominique.

Thanks, I've picked them up for 4.19
 ( git://github.com/martinetd/linux 9p-next as per mail on
 v9fs-developer list, "Current 9P patches - test branch")

This shouldn't stop anyone else from doing more reviews/test, I'm doing
this 9p-patch-gathering on no autority and I've only checked very basic
things for now.

-- 
Dominique Martinet

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [V9fs-developer] [PATCH v2 2/6] 9p: Change p9_fid_create calling convention
  2018-07-11 21:02 ` [PATCH v2 2/6] 9p: Change p9_fid_create calling convention Matthew Wilcox
@ 2018-07-12  2:15   ` " piaojun
  2018-07-12 11:56   ` Greg Kurz
  2018-07-13  1:18   ` jiangyiwen
  2 siblings, 0 replies; 23+ messages in thread
From: piaojun @ 2018-07-12  2:15 UTC (permalink / raw)
  To: Matthew Wilcox, Dominique Martinet
  Cc: Latchesar Ionkov, Eric Van Hensbergen, linux-kernel, Ron Minnich,
	linux-fsdevel, v9fs-developer

LGTM

On 2018/7/12 5:02, Matthew Wilcox wrote:
> Return NULL instead of ERR_PTR when we can't allocate a FID.  The ENOSPC
> return value was getting all the way back to userspace, and that's
> confusing for a userspace program which isn't expecting read() to tell it
> there's no space left on the filesystem.  The best error we can return to
> indicate a temporary failure caused by lack of client resources is ENOMEM.
> 
> Maybe it would be better to sleep until a FID is available, but that's
> not a change I'm comfortable making.
> 
> Signed-off-by: Matthew Wilcox <willy@infradead.org>
Reviewed-by: Jun Piao <piaojun@huawei.com>
> ---
>  net/9p/client.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 999eceb8af98..389a2904b7b3 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -913,13 +913,11 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
>  	p9_debug(P9_DEBUG_FID, "clnt %p\n", clnt);
>  	fid = kmalloc(sizeof(struct p9_fid), GFP_KERNEL);
>  	if (!fid)
> -		return ERR_PTR(-ENOMEM);
> +		return NULL;
>  
>  	ret = p9_idpool_get(clnt->fidpool);
> -	if (ret < 0) {
> -		ret = -ENOSPC;
> +	if (ret < 0)
>  		goto error;
> -	}
>  	fid->fid = ret;
>  
>  	memset(&fid->qid, 0, sizeof(struct p9_qid));
> @@ -935,7 +933,7 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
>  
>  error:
>  	kfree(fid);
> -	return ERR_PTR(ret);
> +	return NULL;
>  }
>  
>  static void p9_fid_destroy(struct p9_fid *fid)
> @@ -1137,9 +1135,8 @@ struct p9_fid *p9_client_attach(struct p9_client *clnt, struct p9_fid *afid,
>  	p9_debug(P9_DEBUG_9P, ">>> TATTACH afid %d uname %s aname %s\n",
>  		 afid ? afid->fid : -1, uname, aname);
>  	fid = p9_fid_create(clnt);
> -	if (IS_ERR(fid)) {
> -		err = PTR_ERR(fid);
> -		fid = NULL;
> +	if (!fid) {
> +		err = -ENOMEM;
>  		goto error;
>  	}
>  	fid->uid = n_uname;
> @@ -1188,9 +1185,8 @@ struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname,
>  	clnt = oldfid->clnt;
>  	if (clone) {
>  		fid = p9_fid_create(clnt);
> -		if (IS_ERR(fid)) {
> -			err = PTR_ERR(fid);
> -			fid = NULL;
> +		if (!fid) {
> +			err = -ENOMEM;
>  			goto error;
>  		}
>  
> @@ -2018,9 +2014,8 @@ struct p9_fid *p9_client_xattrwalk(struct p9_fid *file_fid,
>  	err = 0;
>  	clnt = file_fid->clnt;
>  	attr_fid = p9_fid_create(clnt);
> -	if (IS_ERR(attr_fid)) {
> -		err = PTR_ERR(attr_fid);
> -		attr_fid = NULL;
> +	if (!attr_fid) {
> +		err = -ENOMEM;
>  		goto error;
>  	}
>  	p9_debug(P9_DEBUG_9P,
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 3/6] 9p: Replace the fidlist with an IDR
  2018-07-11 21:02 ` [PATCH v2 3/6] 9p: Replace the fidlist with an IDR Matthew Wilcox
@ 2018-07-12 11:17   ` Dominique Martinet
  2018-07-12 11:23     ` Matthew Wilcox
  2018-07-13  2:05   ` [V9fs-developer] " jiangyiwen
  1 sibling, 1 reply; 23+ messages in thread
From: Dominique Martinet @ 2018-07-12 11:17 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: v9fs-developer, Latchesar Ionkov, Eric Van Hensbergen,
	Ron Minnich, linux-kernel, linux-fsdevel

Matthew Wilcox wrote on Wed, Jul 11, 2018:
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 389a2904b7b3..b89c7298267c 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -908,30 +908,29 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
>  {
>  	int ret;
>  	struct p9_fid *fid;
> -	unsigned long flags;
>  
>  	p9_debug(P9_DEBUG_FID, "clnt %p\n", clnt);
>  	fid = kmalloc(sizeof(struct p9_fid), GFP_KERNEL);
>  	if (!fid)
>  		return NULL;
>  
> -	ret = p9_idpool_get(clnt->fidpool);
> -	if (ret < 0)
> -		goto error;
> -	fid->fid = ret;
> -
>  	memset(&fid->qid, 0, sizeof(struct p9_qid));

Ah, I had missed that you didn't update this memset as you said in reply
to comment on v1.

Could you resend just this patch and either initialize fid->fid or use
kzalloc for the fid allocation?

>  	fid->mode = -1;
>  	fid->uid = current_fsuid();
>  	fid->clnt = clnt;
>  	fid->rdir = NULL;
> -	spin_lock_irqsave(&clnt->lock, flags);
> -	list_add(&fid->flist, &clnt->fidlist);
> -	spin_unlock_irqrestore(&clnt->lock, flags);
> +	fid->fid = 0;
>  
> -	return fid;
> +	idr_preload(GFP_KERNEL);
> +	spin_lock_irq(&clnt->lock);
> +	ret = idr_alloc_u32(&clnt->fids, fid, &fid->fid, P9_NOFID - 1,
> +			GFP_NOWAIT);

If you do resend, alignment here was wrong.


Thanks,
-- 
Dominique Martinet

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 3/6] 9p: Replace the fidlist with an IDR
  2018-07-12 11:17   ` Dominique Martinet
@ 2018-07-12 11:23     ` Matthew Wilcox
  2018-07-12 11:30       ` Dominique Martinet
  0 siblings, 1 reply; 23+ messages in thread
From: Matthew Wilcox @ 2018-07-12 11:23 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: v9fs-developer, Latchesar Ionkov, Eric Van Hensbergen,
	Ron Minnich, linux-kernel, linux-fsdevel

On Thu, Jul 12, 2018 at 01:17:26PM +0200, Dominique Martinet wrote:
> Matthew Wilcox wrote on Wed, Jul 11, 2018:
> > diff --git a/net/9p/client.c b/net/9p/client.c
> > index 389a2904b7b3..b89c7298267c 100644
> > --- a/net/9p/client.c
> > +++ b/net/9p/client.c
> >  	memset(&fid->qid, 0, sizeof(struct p9_qid));
> 
> Ah, I had missed that you didn't update this memset as you said in reply
> to comment on v1.

Rather than update the memset, I ... 

> Could you resend just this patch and either initialize fid->fid or use
> kzalloc for the fid allocation?
> 
> > +	fid->fid = 0;

Did that instead ;-)

If I were going to initialise the entire structure to 0, I'd replace
the kmalloc with kzalloc and drop the memset altogether.

> If you do resend, alignment here was wrong.

I think this warning from checkpatch is complete bullshit.  It's
really none of checkpatch's business how I choose to align function
arguments.

That said, if you want it to be aligned some particular way, feel free
to adjust the whitespace.  I don't care.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 3/6] 9p: Replace the fidlist with an IDR
  2018-07-12 11:23     ` Matthew Wilcox
@ 2018-07-12 11:30       ` Dominique Martinet
  0 siblings, 0 replies; 23+ messages in thread
From: Dominique Martinet @ 2018-07-12 11:30 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: v9fs-developer, Latchesar Ionkov, Eric Van Hensbergen,
	Ron Minnich, linux-kernel, linux-fsdevel

Matthew Wilcox wrote on Thu, Jul 12, 2018:
> > Ah, I had missed that you didn't update this memset as you said in reply
> > to comment on v1.
> 
> Rather than update the memset, I ... 
> 
> > Could you resend just this patch and either initialize fid->fid or use
> > kzalloc for the fid allocation?
> > 
> > > +	fid->fid = 0;
> 
> Did that instead ;-)
> 
> If I were going to initialise the entire structure to 0, I'd replace
> the kmalloc with kzalloc and drop the memset altogether.

Oh, I'm blind, sorry! :)

> > If you do resend, alignment here was wrong.
> 
> I think this warning from checkpatch is complete bullshit.  It's
> really none of checkpatch's business how I choose to align function
> arguments.
> 
> That said, if you want it to be aligned some particular way, feel free
> to adjust the whitespace.  I don't care.

I would tend to agree that sometimes checkpatch is overzealous, but
having worked on projects where code style is all over the place it
really feels much more comfortable to have a consistent style
everywhere.

Thanks for the permission, I'll adjust it (assuming this does end up
getting pulled from my branch, but nobody yelled so far)

-- 
Dominique Martinet

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [V9fs-developer] [PATCH v2 1/6] 9p: Fix comment on smp_wmb
  2018-07-11 21:02 ` [PATCH v2 1/6] 9p: Fix comment on smp_wmb Matthew Wilcox
@ 2018-07-12 11:55   ` " Greg Kurz
  0 siblings, 0 replies; 23+ messages in thread
From: Greg Kurz @ 2018-07-12 11:55 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Dominique Martinet, Latchesar Ionkov, Eric Van Hensbergen,
	linux-kernel, Ron Minnich, linux-fsdevel, v9fs-developer

On Wed, 11 Jul 2018 14:02:20 -0700
Matthew Wilcox <willy@infradead.org> wrote:

> The previous comment misled me into thinking the barrier wasn't needed
> at all.
> 
> Signed-off-by: Matthew Wilcox <willy@infradead.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  net/9p/client.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 18c5271910dc..999eceb8af98 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -447,7 +447,7 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status)
>  
>  	/*
>  	 * This barrier is needed to make sure any change made to req before
> -	 * the other thread wakes up will indeed be seen by the waiting side.
> +	 * the status change is visible to another thread
>  	 */
>  	smp_wmb();
>  	req->status = status;


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [V9fs-developer] [PATCH v2 2/6] 9p: Change p9_fid_create calling convention
  2018-07-11 21:02 ` [PATCH v2 2/6] 9p: Change p9_fid_create calling convention Matthew Wilcox
  2018-07-12  2:15   ` [V9fs-developer] " piaojun
@ 2018-07-12 11:56   ` Greg Kurz
  2018-07-13  1:18   ` jiangyiwen
  2 siblings, 0 replies; 23+ messages in thread
From: Greg Kurz @ 2018-07-12 11:56 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Dominique Martinet, Latchesar Ionkov, Eric Van Hensbergen,
	linux-kernel, Ron Minnich, linux-fsdevel, v9fs-developer

On Wed, 11 Jul 2018 14:02:21 -0700
Matthew Wilcox <willy@infradead.org> wrote:

> Return NULL instead of ERR_PTR when we can't allocate a FID.  The ENOSPC
> return value was getting all the way back to userspace, and that's
> confusing for a userspace program which isn't expecting read() to tell it
> there's no space left on the filesystem.  The best error we can return to
> indicate a temporary failure caused by lack of client resources is ENOMEM.
> 
> Maybe it would be better to sleep until a FID is available, but that's
> not a change I'm comfortable making.
> 
> Signed-off-by: Matthew Wilcox <willy@infradead.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  net/9p/client.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 999eceb8af98..389a2904b7b3 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -913,13 +913,11 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
>  	p9_debug(P9_DEBUG_FID, "clnt %p\n", clnt);
>  	fid = kmalloc(sizeof(struct p9_fid), GFP_KERNEL);
>  	if (!fid)
> -		return ERR_PTR(-ENOMEM);
> +		return NULL;
>  
>  	ret = p9_idpool_get(clnt->fidpool);
> -	if (ret < 0) {
> -		ret = -ENOSPC;
> +	if (ret < 0)
>  		goto error;
> -	}
>  	fid->fid = ret;
>  
>  	memset(&fid->qid, 0, sizeof(struct p9_qid));
> @@ -935,7 +933,7 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
>  
>  error:
>  	kfree(fid);
> -	return ERR_PTR(ret);
> +	return NULL;
>  }
>  
>  static void p9_fid_destroy(struct p9_fid *fid)
> @@ -1137,9 +1135,8 @@ struct p9_fid *p9_client_attach(struct p9_client *clnt, struct p9_fid *afid,
>  	p9_debug(P9_DEBUG_9P, ">>> TATTACH afid %d uname %s aname %s\n",
>  		 afid ? afid->fid : -1, uname, aname);
>  	fid = p9_fid_create(clnt);
> -	if (IS_ERR(fid)) {
> -		err = PTR_ERR(fid);
> -		fid = NULL;
> +	if (!fid) {
> +		err = -ENOMEM;
>  		goto error;
>  	}
>  	fid->uid = n_uname;
> @@ -1188,9 +1185,8 @@ struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname,
>  	clnt = oldfid->clnt;
>  	if (clone) {
>  		fid = p9_fid_create(clnt);
> -		if (IS_ERR(fid)) {
> -			err = PTR_ERR(fid);
> -			fid = NULL;
> +		if (!fid) {
> +			err = -ENOMEM;
>  			goto error;
>  		}
>  
> @@ -2018,9 +2014,8 @@ struct p9_fid *p9_client_xattrwalk(struct p9_fid *file_fid,
>  	err = 0;
>  	clnt = file_fid->clnt;
>  	attr_fid = p9_fid_create(clnt);
> -	if (IS_ERR(attr_fid)) {
> -		err = PTR_ERR(attr_fid);
> -		attr_fid = NULL;
> +	if (!attr_fid) {
> +		err = -ENOMEM;
>  		goto error;
>  	}
>  	p9_debug(P9_DEBUG_9P,


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [V9fs-developer] [PATCH v2 4/6] 9p: Embed wait_queue_head into p9_req_t
  2018-07-11 21:02 ` [PATCH v2 4/6] 9p: Embed wait_queue_head into p9_req_t Matthew Wilcox
@ 2018-07-12 14:36   ` " Greg Kurz
  2018-07-12 14:40     ` Dominique Martinet
  0 siblings, 1 reply; 23+ messages in thread
From: Greg Kurz @ 2018-07-12 14:36 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Dominique Martinet, Latchesar Ionkov, Eric Van Hensbergen,
	linux-kernel, Ron Minnich, linux-fsdevel, v9fs-developer

On Wed, 11 Jul 2018 14:02:23 -0700
Matthew Wilcox <willy@infradead.org> wrote:

> On a 64-bit system, the wait_queue_head_t is 24 bytes while the pointer
> to it is 8 bytes.  Growing the p9_req_t by 16 bytes is better than
> performing a 24-byte memory allocation.
> 

This is true when all tags have been used at least once. But the current code
lazily allocates the wait_queue_head_t, ie, only when a tag is used for the
first time. Your patch causes a full row of wait_quest_head_t to be pre-allocated.

ie, P9_ROW_MAXTAG * 24 = 255 * 24 = 6120

instead of (P9_ROW_MAXTAG * 8) + 24 = 255 * 8 + 24 = 2064

This is nearly a page of allocated memory that might be never used.

Not sure if this is a problem though...

> Signed-off-by: Matthew Wilcox <willy@infradead.org>
> ---
>  include/net/9p/client.h |  2 +-
>  net/9p/client.c         | 19 +++++--------------
>  net/9p/trans_virtio.c   |  2 +-
>  3 files changed, 7 insertions(+), 16 deletions(-)
> 

... and the diffstat is nice :) so

Reviewed-by: Greg Kurz <groug@kaod.org>

> diff --git a/include/net/9p/client.h b/include/net/9p/client.h
> index e405729cd1c7..0fa0fbab33b0 100644
> --- a/include/net/9p/client.h
> +++ b/include/net/9p/client.h
> @@ -113,7 +113,7 @@ enum p9_req_status_t {
>  struct p9_req_t {
>  	int status;
>  	int t_err;
> -	wait_queue_head_t *wq;
> +	wait_queue_head_t wq;
>  	struct p9_fcall *tc;
>  	struct p9_fcall *rc;
>  	void *aux;
> diff --git a/net/9p/client.c b/net/9p/client.c
> index b89c7298267c..bc8aba6b5ce0 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -282,8 +282,9 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
>  				return ERR_PTR(-ENOMEM);
>  			}
>  			for (col = 0; col < P9_ROW_MAXTAG; col++) {
> -				c->reqs[row][col].status = REQ_STATUS_IDLE;
> -				c->reqs[row][col].tc = NULL;
> +				req = &c->reqs[row][col];
> +				req->status = REQ_STATUS_IDLE;
> +				init_waitqueue_head(&req->wq);
>  			}
>  			c->max_tag += P9_ROW_MAXTAG;
>  		}
> @@ -293,13 +294,6 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
>  	col = tag % P9_ROW_MAXTAG;
>  
>  	req = &c->reqs[row][col];
> -	if (!req->wq) {
> -		req->wq = kmalloc(sizeof(wait_queue_head_t), GFP_NOFS);
> -		if (!req->wq)
> -			goto grow_failed;
> -		init_waitqueue_head(req->wq);
> -	}
> -
>  	if (!req->tc)
>  		req->tc = p9_fcall_alloc(alloc_msize);
>  	if (!req->rc)
> @@ -319,9 +313,7 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
>  	pr_err("Couldn't grow tag array\n");
>  	kfree(req->tc);
>  	kfree(req->rc);
> -	kfree(req->wq);
>  	req->tc = req->rc = NULL;
> -	req->wq = NULL;
>  	return ERR_PTR(-ENOMEM);
>  }
>  
> @@ -409,7 +401,6 @@ static void p9_tag_cleanup(struct p9_client *c)
>  	/* free requests associated with tags */
>  	for (row = 0; row < (c->max_tag/P9_ROW_MAXTAG); row++) {
>  		for (col = 0; col < P9_ROW_MAXTAG; col++) {
> -			kfree(c->reqs[row][col].wq);
>  			kfree(c->reqs[row][col].tc);
>  			kfree(c->reqs[row][col].rc);
>  		}
> @@ -452,7 +443,7 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status)
>  	smp_wmb();
>  	req->status = status;
>  
> -	wake_up(req->wq);
> +	wake_up(&req->wq);
>  	p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc->tag);
>  }
>  EXPORT_SYMBOL(p9_client_cb);
> @@ -773,7 +764,7 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
>  	}
>  again:
>  	/* Wait for the response */
> -	err = wait_event_killable(*req->wq, req->status >= REQ_STATUS_RCVD);
> +	err = wait_event_killable(req->wq, req->status >= REQ_STATUS_RCVD);
>  
>  	/*
>  	 * Make sure our req is coherent with regard to updates in other
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 05006cbb3361..3e096c98313c 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -490,7 +490,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
>  	virtqueue_kick(chan->vq);
>  	spin_unlock_irqrestore(&chan->lock, flags);
>  	p9_debug(P9_DEBUG_TRANS, "virtio request kicked\n");
> -	err = wait_event_killable(*req->wq, req->status >= REQ_STATUS_RCVD);
> +	err = wait_event_killable(req->wq, req->status >= REQ_STATUS_RCVD);
>  	/*
>  	 * Non kernel buffers are pinned, unpin them
>  	 */


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [V9fs-developer] [PATCH v2 4/6] 9p: Embed wait_queue_head into p9_req_t
  2018-07-12 14:36   ` [V9fs-developer] " Greg Kurz
@ 2018-07-12 14:40     ` Dominique Martinet
  2018-07-12 14:59       ` Greg Kurz
  0 siblings, 1 reply; 23+ messages in thread
From: Dominique Martinet @ 2018-07-12 14:40 UTC (permalink / raw)
  To: Greg Kurz
  Cc: Matthew Wilcox, Latchesar Ionkov, Eric Van Hensbergen,
	linux-kernel, Ron Minnich, linux-fsdevel, v9fs-developer

Greg Kurz wrote on Thu, Jul 12, 2018:
> This is true when all tags have been used at least once. But the current code
> lazily allocates the wait_queue_head_t, ie, only when a tag is used for the
> first time. Your patch causes a full row of wait_quest_head_t to be pre-allocated.
> 
> ie, P9_ROW_MAXTAG * 24 = 255 * 24 = 6120
> 
> instead of (P9_ROW_MAXTAG * 8) + 24 = 255 * 8 + 24 = 2064
> 
> This is nearly a page of allocated memory that might be never used.
> 
> Not sure if this is a problem though...

I thought the exact same, but the next patch in the serie removes that
array and allocates even more lazily with a slab, so this was a
no-brainer :)

-- 
Dominique Martinet

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [V9fs-developer] [PATCH v2 4/6] 9p: Embed wait_queue_head into p9_req_t
  2018-07-12 14:40     ` Dominique Martinet
@ 2018-07-12 14:59       ` Greg Kurz
  0 siblings, 0 replies; 23+ messages in thread
From: Greg Kurz @ 2018-07-12 14:59 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Matthew Wilcox, Latchesar Ionkov, Eric Van Hensbergen,
	linux-kernel, Ron Minnich, linux-fsdevel, v9fs-developer

On Thu, 12 Jul 2018 16:40:12 +0200
Dominique Martinet <asmadeus@codewreck.org> wrote:

> Greg Kurz wrote on Thu, Jul 12, 2018:
> > This is true when all tags have been used at least once. But the current code
> > lazily allocates the wait_queue_head_t, ie, only when a tag is used for the
> > first time. Your patch causes a full row of wait_quest_head_t to be pre-allocated.
> > 
> > ie, P9_ROW_MAXTAG * 24 = 255 * 24 = 6120
> > 
> > instead of (P9_ROW_MAXTAG * 8) + 24 = 255 * 8 + 24 = 2064
> > 
> > This is nearly a page of allocated memory that might be never used.
> > 
> > Not sure if this is a problem though...  
> 
> I thought the exact same, but the next patch in the serie removes that
> array and allocates even more lazily with a slab, so this was a
> no-brainer :)
> 

Ah ? I haven't started looking at that *big* other patch yet... :)

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [V9fs-developer] [PATCH v2 2/6] 9p: Change p9_fid_create calling convention
  2018-07-11 21:02 ` [PATCH v2 2/6] 9p: Change p9_fid_create calling convention Matthew Wilcox
  2018-07-12  2:15   ` [V9fs-developer] " piaojun
  2018-07-12 11:56   ` Greg Kurz
@ 2018-07-13  1:18   ` jiangyiwen
  2 siblings, 0 replies; 23+ messages in thread
From: jiangyiwen @ 2018-07-13  1:18 UTC (permalink / raw)
  To: Matthew Wilcox, Dominique Martinet
  Cc: Latchesar Ionkov, Eric Van Hensbergen, linux-kernel, Ron Minnich,
	linux-fsdevel, v9fs-developer

On 2018/7/12 5:02, Matthew Wilcox wrote:
> Return NULL instead of ERR_PTR when we can't allocate a FID.  The ENOSPC
> return value was getting all the way back to userspace, and that's
> confusing for a userspace program which isn't expecting read() to tell it
> there's no space left on the filesystem.  The best error we can return to
> indicate a temporary failure caused by lack of client resources is ENOMEM.
> 
> Maybe it would be better to sleep until a FID is available, but that's
> not a change I'm comfortable making.
> 
> Signed-off-by: Matthew Wilcox <willy@infradead.org>

Reviewed-by: Yiwen Jiang <jiangyiwen@huwei.com>

> ---
>  net/9p/client.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 999eceb8af98..389a2904b7b3 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -913,13 +913,11 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
>  	p9_debug(P9_DEBUG_FID, "clnt %p\n", clnt);
>  	fid = kmalloc(sizeof(struct p9_fid), GFP_KERNEL);
>  	if (!fid)
> -		return ERR_PTR(-ENOMEM);
> +		return NULL;
>  
>  	ret = p9_idpool_get(clnt->fidpool);
> -	if (ret < 0) {
> -		ret = -ENOSPC;
> +	if (ret < 0)
>  		goto error;
> -	}
>  	fid->fid = ret;
>  
>  	memset(&fid->qid, 0, sizeof(struct p9_qid));
> @@ -935,7 +933,7 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
>  
>  error:
>  	kfree(fid);
> -	return ERR_PTR(ret);
> +	return NULL;
>  }
>  
>  static void p9_fid_destroy(struct p9_fid *fid)
> @@ -1137,9 +1135,8 @@ struct p9_fid *p9_client_attach(struct p9_client *clnt, struct p9_fid *afid,
>  	p9_debug(P9_DEBUG_9P, ">>> TATTACH afid %d uname %s aname %s\n",
>  		 afid ? afid->fid : -1, uname, aname);
>  	fid = p9_fid_create(clnt);
> -	if (IS_ERR(fid)) {
> -		err = PTR_ERR(fid);
> -		fid = NULL;
> +	if (!fid) {
> +		err = -ENOMEM;
>  		goto error;
>  	}
>  	fid->uid = n_uname;
> @@ -1188,9 +1185,8 @@ struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname,
>  	clnt = oldfid->clnt;
>  	if (clone) {
>  		fid = p9_fid_create(clnt);
> -		if (IS_ERR(fid)) {
> -			err = PTR_ERR(fid);
> -			fid = NULL;
> +		if (!fid) {
> +			err = -ENOMEM;
>  			goto error;
>  		}
>  
> @@ -2018,9 +2014,8 @@ struct p9_fid *p9_client_xattrwalk(struct p9_fid *file_fid,
>  	err = 0;
>  	clnt = file_fid->clnt;
>  	attr_fid = p9_fid_create(clnt);
> -	if (IS_ERR(attr_fid)) {
> -		err = PTR_ERR(attr_fid);
> -		attr_fid = NULL;
> +	if (!attr_fid) {
> +		err = -ENOMEM;
>  		goto error;
>  	}
>  	p9_debug(P9_DEBUG_9P,
> 


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [V9fs-developer] [PATCH v2 3/6] 9p: Replace the fidlist with an IDR
  2018-07-11 21:02 ` [PATCH v2 3/6] 9p: Replace the fidlist with an IDR Matthew Wilcox
  2018-07-12 11:17   ` Dominique Martinet
@ 2018-07-13  2:05   ` " jiangyiwen
  2018-07-13  2:48     ` Matthew Wilcox
  1 sibling, 1 reply; 23+ messages in thread
From: jiangyiwen @ 2018-07-13  2:05 UTC (permalink / raw)
  To: Matthew Wilcox, Dominique Martinet
  Cc: Latchesar Ionkov, Eric Van Hensbergen, linux-kernel, Ron Minnich,
	linux-fsdevel, v9fs-developer

On 2018/7/12 5:02, Matthew Wilcox wrote:
> The p9_idpool being used to allocate the IDs uses an IDR to allocate
> the IDs ... which we then keep in a doubly-linked list, rather than in
> the IDR which allocated them.  We can use an IDR directly which saves
> two pointers per p9_fid, and a tiny memory allocation per p9_client.
> 
> Signed-off-by: Matthew Wilcox <willy@infradead.org>
> ---
>  include/net/9p/client.h |  9 +++------
>  net/9p/client.c         | 44 +++++++++++++++--------------------------
>  2 files changed, 19 insertions(+), 34 deletions(-)
> 
> diff --git a/include/net/9p/client.h b/include/net/9p/client.h
> index 7af9d769b97d..e405729cd1c7 100644
> --- a/include/net/9p/client.h
> +++ b/include/net/9p/client.h
> @@ -27,6 +27,7 @@
>  #define NET_9P_CLIENT_H
>  
>  #include <linux/utsname.h>
> +#include <linux/idr.h>
>  
>  /* Number of requests per row */
>  #define P9_ROW_MAXTAG 255
> @@ -128,8 +129,7 @@ struct p9_req_t {
>   * @proto_version: 9P protocol version to use
>   * @trans_mod: module API instantiated with this client
>   * @trans: tranport instance state and API
> - * @fidpool: fid handle accounting for session
> - * @fidlist: List of active fid handles
> + * @fids: All active FID handles
>   * @tagpool - transaction id accounting for session
>   * @reqs - 2D array of requests
>   * @max_tag - current maximum tag id allocated
> @@ -169,8 +169,7 @@ struct p9_client {
>  		} tcp;
>  	} trans_opts;
>  
> -	struct p9_idpool *fidpool;
> -	struct list_head fidlist;
> +	struct idr fids;
>  
>  	struct p9_idpool *tagpool;
>  	struct p9_req_t *reqs[P9_ROW_MAXTAG];
> @@ -188,7 +187,6 @@ struct p9_client {
>   * @iounit: the server reported maximum transaction size for this file
>   * @uid: the numeric uid of the local user who owns this handle
>   * @rdir: readdir accounting structure (allocated on demand)
> - * @flist: per-client-instance fid tracking
>   * @dlist: per-dentry fid tracking
>   *
>   * TODO: This needs lots of explanation.
> @@ -204,7 +202,6 @@ struct p9_fid {
>  
>  	void *rdir;
>  
> -	struct list_head flist;
>  	struct hlist_node dlist;	/* list of all fids attached to a dentry */
>  };
>  
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 389a2904b7b3..b89c7298267c 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -908,30 +908,29 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
>  {
>  	int ret;
>  	struct p9_fid *fid;
> -	unsigned long flags;
>  
>  	p9_debug(P9_DEBUG_FID, "clnt %p\n", clnt);
>  	fid = kmalloc(sizeof(struct p9_fid), GFP_KERNEL);
>  	if (!fid)
>  		return NULL;
>  
> -	ret = p9_idpool_get(clnt->fidpool);
> -	if (ret < 0)
> -		goto error;
> -	fid->fid = ret;
> -
>  	memset(&fid->qid, 0, sizeof(struct p9_qid));
>  	fid->mode = -1;
>  	fid->uid = current_fsuid();
>  	fid->clnt = clnt;
>  	fid->rdir = NULL;
> -	spin_lock_irqsave(&clnt->lock, flags);
> -	list_add(&fid->flist, &clnt->fidlist);
> -	spin_unlock_irqrestore(&clnt->lock, flags);
> +	fid->fid = 0;
>  
> -	return fid;
> +	idr_preload(GFP_KERNEL);

It is best to use GFP_NOFS instead, or else it may cause some
unpredictable problem, because when out of memory it will
reclaim memory from v9fs.

> +	spin_lock_irq(&clnt->lock);
> +	ret = idr_alloc_u32(&clnt->fids, fid, &fid->fid, P9_NOFID - 1,
> +			GFP_NOWAIT);
> +	spin_unlock_irq(&clnt->lock);

use spin_lock instead, clnt->lock is not used in irq context.

> +	idr_preload_end();
> +
> +	if (!ret)
> +		return fid;
>  
> -error:
>  	kfree(fid);
>  	return NULL;
>  }
> @@ -943,9 +942,8 @@ static void p9_fid_destroy(struct p9_fid *fid)
>  
>  	p9_debug(P9_DEBUG_FID, "fid %d\n", fid->fid);
>  	clnt = fid->clnt;
> -	p9_idpool_put(fid->fid, clnt->fidpool);
>  	spin_lock_irqsave(&clnt->lock, flags);
> -	list_del(&fid->flist);
> +	idr_remove(&clnt->fids, fid->fid);
>  	spin_unlock_irqrestore(&clnt->lock, flags);
>  	kfree(fid->rdir);
>  	kfree(fid);
> @@ -1028,7 +1026,7 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
>  	memcpy(clnt->name, client_id, strlen(client_id) + 1);
>  
>  	spin_lock_init(&clnt->lock);
> -	INIT_LIST_HEAD(&clnt->fidlist);
> +	idr_init(&clnt->fids);
>  
>  	err = p9_tag_init(clnt);
>  	if (err < 0)
> @@ -1048,18 +1046,12 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
>  		goto destroy_tagpool;
>  	}
>  
> -	clnt->fidpool = p9_idpool_create();
> -	if (IS_ERR(clnt->fidpool)) {
> -		err = PTR_ERR(clnt->fidpool);
> -		goto put_trans;
> -	}
> -
>  	p9_debug(P9_DEBUG_MUX, "clnt %p trans %p msize %d protocol %d\n",
>  		 clnt, clnt->trans_mod, clnt->msize, clnt->proto_version);
>  
>  	err = clnt->trans_mod->create(clnt, dev_name, options);
>  	if (err)
> -		goto destroy_fidpool;
> +		goto put_trans;
>  
>  	if (clnt->msize > clnt->trans_mod->maxsize)
>  		clnt->msize = clnt->trans_mod->maxsize;
> @@ -1072,8 +1064,6 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
>  
>  close_trans:
>  	clnt->trans_mod->close(clnt);
> -destroy_fidpool:
> -	p9_idpool_destroy(clnt->fidpool);
>  put_trans:
>  	v9fs_put_trans(clnt->trans_mod);
>  destroy_tagpool:
> @@ -1086,7 +1076,8 @@ EXPORT_SYMBOL(p9_client_create);
>  
>  void p9_client_destroy(struct p9_client *clnt)
>  {
> -	struct p9_fid *fid, *fidptr;
> +	struct p9_fid *fid;
> +	int id;
>  
>  	p9_debug(P9_DEBUG_MUX, "clnt %p\n", clnt);
>  
> @@ -1095,14 +1086,11 @@ void p9_client_destroy(struct p9_client *clnt)
>  
>  	v9fs_put_trans(clnt->trans_mod);
>  
> -	list_for_each_entry_safe(fid, fidptr, &clnt->fidlist, flist) {
> +	idr_for_each_entry(&clnt->fids, fid, id) {
>  		pr_info("Found fid %d not clunked\n", fid->fid);
>  		p9_fid_destroy(fid);
>  	}
>  
> -	if (clnt->fidpool)
> -		p9_idpool_destroy(clnt->fidpool);
> -

I suggest add idr_destroy in the end.

>  	p9_tag_cleanup(clnt);
>  
>  	kfree(clnt);
> 



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [V9fs-developer] [PATCH v2 3/6] 9p: Replace the fidlist with an IDR
  2018-07-13  2:05   ` [V9fs-developer] " jiangyiwen
@ 2018-07-13  2:48     ` Matthew Wilcox
  0 siblings, 0 replies; 23+ messages in thread
From: Matthew Wilcox @ 2018-07-13  2:48 UTC (permalink / raw)
  To: jiangyiwen
  Cc: Dominique Martinet, Latchesar Ionkov, Eric Van Hensbergen,
	linux-kernel, Ron Minnich, linux-fsdevel, v9fs-developer

On Fri, Jul 13, 2018 at 10:05:50AM +0800, jiangyiwen wrote:
> > @@ -908,30 +908,29 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
> >  {
> >  	int ret;
> >  	struct p9_fid *fid;
> > -	unsigned long flags;
> >  
> >  	p9_debug(P9_DEBUG_FID, "clnt %p\n", clnt);
> >  	fid = kmalloc(sizeof(struct p9_fid), GFP_KERNEL);
> >  	if (!fid)
> >  		return NULL;
> >  
> > -	ret = p9_idpool_get(clnt->fidpool);
> > -	if (ret < 0)
> > -		goto error;
> > -	fid->fid = ret;
> > -
> >  	memset(&fid->qid, 0, sizeof(struct p9_qid));
> >  	fid->mode = -1;
> >  	fid->uid = current_fsuid();
> >  	fid->clnt = clnt;
> >  	fid->rdir = NULL;
> > -	spin_lock_irqsave(&clnt->lock, flags);
> > -	list_add(&fid->flist, &clnt->fidlist);
> > -	spin_unlock_irqrestore(&clnt->lock, flags);
> > +	fid->fid = 0;
> >  
> > -	return fid;
> > +	idr_preload(GFP_KERNEL);
> 
> It is best to use GFP_NOFS instead, or else it may cause some
> unpredictable problem, because when out of memory it will
> reclaim memory from v9fs.

Earlier in this function, fid was allocated with GFP_KERNEL:

> >  	fid = kmalloc(sizeof(struct p9_fid), GFP_KERNEL);


> > +	spin_lock_irq(&clnt->lock);
> > +	ret = idr_alloc_u32(&clnt->fids, fid, &fid->fid, P9_NOFID - 1,
> > +			GFP_NOWAIT);
> > +	spin_unlock_irq(&clnt->lock);
> 
> use spin_lock instead, clnt->lock is not used in irq context.

I don't think that's right.  What about p9_fid_destroy?  It was already
using spin_lock_irqsave(), so I just assumed that whoever wrote that
code at least considered that it might be called from interrupt context.

Also consider p9_free_req() which shares the same lock.  We could get
rid of clnt->lock altogether as there's a lock embedded in each IDR,
but that'll introduce an unwanted dependence on the RDMA tree in this
merge window.

> > @@ -1095,14 +1086,11 @@ void p9_client_destroy(struct p9_client *clnt)
> >  
> >  	v9fs_put_trans(clnt->trans_mod);
> >  
> > -	list_for_each_entry_safe(fid, fidptr, &clnt->fidlist, flist) {
> > +	idr_for_each_entry(&clnt->fids, fid, id) {
> >  		pr_info("Found fid %d not clunked\n", fid->fid);
> >  		p9_fid_destroy(fid);
> >  	}
> >  
> > -	if (clnt->fidpool)
> > -		p9_idpool_destroy(clnt->fidpool);
> > -
> 
> I suggest add idr_destroy in the end.

Why?  p9_fid_destroy calls idr_remove() for each fid, so it'll already
be empty.

Thanks for all the review, to everyone who's submitted review.  This is
a really healthy community.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 5/6] 9p: Use a slab for allocating requests
  2018-07-11 21:02 ` [PATCH v2 5/6] 9p: Use a slab for allocating requests Matthew Wilcox
@ 2018-07-18 10:05   ` Dominique Martinet
  2018-07-18 11:49     ` Matthew Wilcox
  0 siblings, 1 reply; 23+ messages in thread
From: Dominique Martinet @ 2018-07-18 10:05 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Greg Kurz, v9fs-developer, Latchesar Ionkov, Eric Van Hensbergen,
	Ron Minnich, linux-kernel, linux-fsdevel

+Cc Greg, I could use your opinion on this if you have a moment.

Matthew Wilcox wrote on Wed, Jul 11, 2018:
> Replace the custom batch allocation with a slab.  Use an IDR to store
> pointers to the active requests instead of an array.  We don't try to
> handle P9_NOTAG specially; the IDR will happily shrink all the way back
> once the TVERSION call has completed.

Sorry for coming back to this patch now, I just noticed something that's
actually probably a fairly big hit on performance...

While the slab is just as good as the array for the request itself, this
makes every single request allocate "fcalls" everytime instead of
reusing a cached allocation.
The default msize is 8k and these allocs probably are fairly efficient,
but some transports like RDMA allow to increase this to up to 1MB... And
doing this kind of allocation twice for every packet is going to be very
slow.
(not that hogging megabytes of memory was a great practice either!)


One thing is that the buffers are all going to be the same size for a
given client (.... except virtio zc buffers, I wonder what I'm missing
or why that didn't blow up before?)
Err, that aside I was going to ask if we couldn't find a way to keep a
pool of these somehow.
Ideally putting them in another slab so they could be reclaimed if
necessary, but the size could vary from one client to another, can we
create a kmem_cache object per client? the KMEM_CACHE macro is not very
flexible so I don't think that is encouraged... :)


It's a shame because I really like that patch, I'll try to find time to
run some light benchmark with varying msizes eventually but I'm not sure
when I'll find time for that... Hopefully before the 4.19 merge window!


>  /**
> - * p9_tag_alloc - lookup/allocate a request by tag
> - * @c: client session to lookup tag within
> - * @tag: numeric id for transaction
> - *
> - * this is a simple array lookup, but will grow the
> - * request_slots as necessary to accommodate transaction
> - * ids which did not previously have a slot.
> - *
> - * this code relies on the client spinlock to manage locks, its
> - * possible we should switch to something else, but I'd rather
> - * stick with something low-overhead for the common case.
> + * p9_req_alloc - Allocate a new request.
> + * @c: Client session.
> + * @type: Transaction type.
> + * @max_size: Maximum packet size for this request.
>   *
> + * Context: Process context.
> + * Return: Pointer to new request.
>   */
> -
>  static struct p9_req_t *
> -p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
> +p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
>  {
> -	unsigned long flags;
> -	int row, col;
> -	struct p9_req_t *req;
> +	struct p9_req_t *req = kmem_cache_alloc(p9_req_cache, GFP_NOFS);
>  	int alloc_msize = min(c->msize, max_size);
> +	int tag;
>  
> -	/* This looks up the original request by tag so we know which
> -	 * buffer to read the data into */
> -	tag++;
> -
> -	if (tag >= c->max_tag) {
> -		spin_lock_irqsave(&c->lock, flags);
> -		/* check again since original check was outside of lock */
> -		while (tag >= c->max_tag) {
> -			row = (tag / P9_ROW_MAXTAG);
> -			c->reqs[row] = kcalloc(P9_ROW_MAXTAG,
> -					sizeof(struct p9_req_t), GFP_ATOMIC);
> -
> -			if (!c->reqs[row]) {
> -				pr_err("Couldn't grow tag array\n");
> -				spin_unlock_irqrestore(&c->lock, flags);
> -				return ERR_PTR(-ENOMEM);
> -			}
> -			for (col = 0; col < P9_ROW_MAXTAG; col++) {
> -				req = &c->reqs[row][col];
> -				req->status = REQ_STATUS_IDLE;
> -				init_waitqueue_head(&req->wq);
> -			}
> -			c->max_tag += P9_ROW_MAXTAG;
> -		}
> -		spin_unlock_irqrestore(&c->lock, flags);
> -	}
> -	row = tag / P9_ROW_MAXTAG;
> -	col = tag % P9_ROW_MAXTAG;
> +	if (!req)
> +		return NULL;
>  
> -	req = &c->reqs[row][col];
> -	if (!req->tc)
> -		req->tc = p9_fcall_alloc(alloc_msize);
> -	if (!req->rc)
> -		req->rc = p9_fcall_alloc(alloc_msize);
> +	req->tc = p9_fcall_alloc(alloc_msize);
> +	req->rc = p9_fcall_alloc(alloc_msize);
>  	if (!req->tc || !req->rc)
> -		goto grow_failed;
> +		goto free;
>  
>  	p9pdu_reset(req->tc);
>  	p9pdu_reset(req->rc);
> -
> -	req->tc->tag = tag-1;
>  	req->status = REQ_STATUS_ALLOC;
> +	init_waitqueue_head(&req->wq);
> +	INIT_LIST_HEAD(&req->req_list);
> +
> +	idr_preload(GFP_NOFS);
> +	spin_lock_irq(&c->lock);
> +	if (type == P9_TVERSION)
> +		tag = idr_alloc(&c->reqs, req, P9_NOTAG, P9_NOTAG + 1,
> +				GFP_NOWAIT);
> +	else
> +		tag = idr_alloc(&c->reqs, req, 0, P9_NOTAG, GFP_NOWAIT);
> +	req->tc->tag = tag;
> +	spin_unlock_irq(&c->lock);
> +	idr_preload_end();
> +	if (tag < 0)
> +		goto free;

-- 
Dominique

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 5/6] 9p: Use a slab for allocating requests
  2018-07-18 10:05   ` Dominique Martinet
@ 2018-07-18 11:49     ` Matthew Wilcox
  2018-07-18 12:46       ` Dominique Martinet
  0 siblings, 1 reply; 23+ messages in thread
From: Matthew Wilcox @ 2018-07-18 11:49 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Greg Kurz, v9fs-developer, Latchesar Ionkov, Eric Van Hensbergen,
	Ron Minnich, linux-kernel, linux-fsdevel

On Wed, Jul 18, 2018 at 12:05:54PM +0200, Dominique Martinet wrote:
> +Cc Greg, I could use your opinion on this if you have a moment.
> 
> Matthew Wilcox wrote on Wed, Jul 11, 2018:
> > Replace the custom batch allocation with a slab.  Use an IDR to store
> > pointers to the active requests instead of an array.  We don't try to
> > handle P9_NOTAG specially; the IDR will happily shrink all the way back
> > once the TVERSION call has completed.
> 
> Sorry for coming back to this patch now, I just noticed something that's
> actually probably a fairly big hit on performance...
> 
> While the slab is just as good as the array for the request itself, this
> makes every single request allocate "fcalls" everytime instead of
> reusing a cached allocation.
> The default msize is 8k and these allocs probably are fairly efficient,
> but some transports like RDMA allow to increase this to up to 1MB... And
> doing this kind of allocation twice for every packet is going to be very
> slow.
> (not that hogging megabytes of memory was a great practice either!)
> 
> 
> One thing is that the buffers are all going to be the same size for a
> given client (.... except virtio zc buffers, I wonder what I'm missing
> or why that didn't blow up before?)
> Err, that aside I was going to ask if we couldn't find a way to keep a
> pool of these somehow.
> Ideally putting them in another slab so they could be reclaimed if
> necessary, but the size could vary from one client to another, can we
> create a kmem_cache object per client? the KMEM_CACHE macro is not very
> flexible so I don't think that is encouraged... :)
> 
> 
> It's a shame because I really like that patch, I'll try to find time to
> run some light benchmark with varying msizes eventually but I'm not sure
> when I'll find time for that... Hopefully before the 4.19 merge window!

I must admit to having not looked at the fcall aspect of this.  kmalloc
is implemented in terms of slab, so it's not going to be much slower than
using a dedicatd slab (a few instructions to figure out which slab cache
to use).

What would help is splitting the p9_fcall struct from the alloc_size.
kmalloc is pretty effective at allocating power-of-two sized structures,
and 8k + 32 bytes is approximately the worst case scenario.  I'd do this
by embedding the p9_fcall into the p9_req_t and only allocating the
msize, like this:

(I only converted client.c to the new embedded fcall way; all the
transports also need massaging)

Regardless of whether any of the other patches go in, this patch is
worth having; it's really wasting slab allocator memory.

diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index a4dc42c53d18..4b4ac1362ad5 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -95,8 +95,8 @@ struct p9_req_t {
 	int status;
 	int t_err;
 	wait_queue_head_t wq;
-	struct p9_fcall *tc;
-	struct p9_fcall *rc;
+	struct p9_fcall tc;
+	struct p9_fcall rc;
 	void *aux;
 	struct list_head req_list;
 };
diff --git a/net/9p/client.c b/net/9p/client.c
index ecf5dd269f4c..058dfbebdaa2 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -230,15 +230,13 @@ static int parse_opts(char *opts, struct p9_client *clnt)
 	return ret;
 }
 
-static struct p9_fcall *p9_fcall_alloc(int alloc_msize)
+static int p9_fcall_alloc(struct p9_fcall *fc, int alloc_msize)
 {
-	struct p9_fcall *fc;
-	fc = kmalloc(sizeof(struct p9_fcall) + alloc_msize, GFP_NOFS);
-	if (!fc)
-		return NULL;
+	fc->sdata = kmalloc(alloc_msize, GFP_NOFS);
+	if (!fc->sdata)
+		return -ENOMEM;
 	fc->capacity = alloc_msize;
-	fc->sdata = (char *) fc + sizeof(struct p9_fcall);
-	return fc;
+	return 0;
 }
 
 static struct kmem_cache *p9_req_cache;
@@ -262,13 +260,13 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
 	if (!req)
 		return NULL;
 
-	req->tc = p9_fcall_alloc(alloc_msize);
-	req->rc = p9_fcall_alloc(alloc_msize);
-	if (!req->tc || !req->rc)
+	if (p9_fcall_alloc(&req->tc, alloc_msize))
+		goto free;
+	if (p9_fcall_alloc(&req->rc, alloc_msize))
 		goto free;
 
-	p9pdu_reset(req->tc);
-	p9pdu_reset(req->rc);
+	p9pdu_reset(&req->tc);
+	p9pdu_reset(&req->rc);
 	req->status = REQ_STATUS_ALLOC;
 	init_waitqueue_head(&req->wq);
 	INIT_LIST_HEAD(&req->req_list);
@@ -280,7 +278,7 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
 				GFP_NOWAIT);
 	else
 		tag = idr_alloc(&c->reqs, req, 0, P9_NOTAG, GFP_NOWAIT);
-	req->tc->tag = tag;
+	req->tc.tag = tag;
 	spin_unlock_irq(&c->lock);
 	idr_preload_end();
 	if (tag < 0)
@@ -289,8 +287,8 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
 	return req;
 
 free:
-	kfree(req->tc);
-	kfree(req->rc);
+	kfree(req->tc.sdata);
+	kfree(req->rc.sdata);
 	kmem_cache_free(p9_req_cache, req);
 	return ERR_PTR(-ENOMEM);
 }
@@ -329,14 +327,14 @@ EXPORT_SYMBOL(p9_tag_lookup);
 static void p9_free_req(struct p9_client *c, struct p9_req_t *r)
 {
 	unsigned long flags;
-	u16 tag = r->tc->tag;
+	u16 tag = r->tc.tag;
 
 	p9_debug(P9_DEBUG_MUX, "clnt %p req %p tag: %d\n", c, r, tag);
 	spin_lock_irqsave(&c->lock, flags);
 	idr_remove(&c->reqs, tag);
 	spin_unlock_irqrestore(&c->lock, flags);
-	kfree(r->tc);
-	kfree(r->rc);
+	kfree(r->tc.sdata);
+	kfree(r->rc.sdata);
 	kmem_cache_free(p9_req_cache, r);
 }
 
@@ -368,7 +366,7 @@ static void p9_tag_cleanup(struct p9_client *c)
  */
 void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status)
 {
-	p9_debug(P9_DEBUG_MUX, " tag %d\n", req->tc->tag);
+	p9_debug(P9_DEBUG_MUX, " tag %d\n", req->tc.tag);
 
 	/*
 	 * This barrier is needed to make sure any change made to req before
@@ -378,7 +376,7 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status)
 	req->status = status;
 
 	wake_up(&req->wq);
-	p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc->tag);
+	p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc.tag);
 }
 EXPORT_SYMBOL(p9_client_cb);
 
@@ -448,12 +446,12 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
 	int err;
 	int ecode;
 
-	err = p9_parse_header(req->rc, NULL, &type, NULL, 0);
+	err = p9_parse_header(&req->rc, NULL, &type, NULL, 0);
 	/*
 	 * dump the response from server
 	 * This should be after check errors which poplulate pdu_fcall.
 	 */
-	trace_9p_protocol_dump(c, req->rc);
+	trace_9p_protocol_dump(c, &req->rc);
 	if (err) {
 		p9_debug(P9_DEBUG_ERROR, "couldn't parse header %d\n", err);
 		return err;
@@ -463,7 +461,7 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
 
 	if (!p9_is_proto_dotl(c)) {
 		char *ename;
-		err = p9pdu_readf(req->rc, c->proto_version, "s?d",
+		err = p9pdu_readf(&req->rc, c->proto_version, "s?d",
 				  &ename, &ecode);
 		if (err)
 			goto out_err;
@@ -479,7 +477,7 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
 		}
 		kfree(ename);
 	} else {
-		err = p9pdu_readf(req->rc, c->proto_version, "d", &ecode);
+		err = p9pdu_readf(&req->rc, c->proto_version, "d", &ecode);
 		err = -ecode;
 
 		p9_debug(P9_DEBUG_9P, "<<< RLERROR (%d)\n", -ecode);
@@ -513,12 +511,12 @@ static int p9_check_zc_errors(struct p9_client *c, struct p9_req_t *req,
 	int8_t type;
 	char *ename = NULL;
 
-	err = p9_parse_header(req->rc, NULL, &type, NULL, 0);
+	err = p9_parse_header(&req->rc, NULL, &type, NULL, 0);
 	/*
 	 * dump the response from server
 	 * This should be after parse_header which poplulate pdu_fcall.
 	 */
-	trace_9p_protocol_dump(c, req->rc);
+	trace_9p_protocol_dump(c, &req->rc);
 	if (err) {
 		p9_debug(P9_DEBUG_ERROR, "couldn't parse header %d\n", err);
 		return err;
@@ -533,13 +531,13 @@ static int p9_check_zc_errors(struct p9_client *c, struct p9_req_t *req,
 		/* 7 = header size for RERROR; */
 		int inline_len = in_hdrlen - 7;
 
-		len =  req->rc->size - req->rc->offset;
+		len = req->rc.size - req->rc.offset;
 		if (len > (P9_ZC_HDR_SZ - 7)) {
 			err = -EFAULT;
 			goto out_err;
 		}
 
-		ename = &req->rc->sdata[req->rc->offset];
+		ename = &req->rc.sdata[req->rc.offset];
 		if (len > inline_len) {
 			/* We have error in external buffer */
 			if (!copy_from_iter_full(ename + inline_len,
@@ -549,7 +547,7 @@ static int p9_check_zc_errors(struct p9_client *c, struct p9_req_t *req,
 			}
 		}
 		ename = NULL;
-		err = p9pdu_readf(req->rc, c->proto_version, "s?d",
+		err = p9pdu_readf(&req->rc, c->proto_version, "s?d",
 				  &ename, &ecode);
 		if (err)
 			goto out_err;
@@ -565,7 +563,7 @@ static int p9_check_zc_errors(struct p9_client *c, struct p9_req_t *req,
 		}
 		kfree(ename);
 	} else {
-		err = p9pdu_readf(req->rc, c->proto_version, "d", &ecode);
+		err = p9pdu_readf(&req->rc, c->proto_version, "d", &ecode);
 		err = -ecode;
 
 		p9_debug(P9_DEBUG_9P, "<<< RLERROR (%d)\n", -ecode);
@@ -598,7 +596,7 @@ static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq)
 	int16_t oldtag;
 	int err;
 
-	err = p9_parse_header(oldreq->tc, NULL, NULL, &oldtag, 1);
+	err = p9_parse_header(&oldreq->tc, NULL, NULL, &oldtag, 1);
 	if (err)
 		return err;
 
@@ -642,12 +640,12 @@ static struct p9_req_t *p9_client_prepare_req(struct p9_client *c,
 		return req;
 
 	/* marshall the data */
-	p9pdu_prepare(req->tc, req->tc->tag, type);
-	err = p9pdu_vwritef(req->tc, c->proto_version, fmt, ap);
+	p9pdu_prepare(&req->tc, req->tc.tag, type);
+	err = p9pdu_vwritef(&req->tc, c->proto_version, fmt, ap);
 	if (err)
 		goto reterr;
-	p9pdu_finalize(c, req->tc);
-	trace_9p_client_req(c, type, req->tc->tag);
+	p9pdu_finalize(c, &req->tc);
+	trace_9p_client_req(c, type, req->tc.tag);
 	return req;
 reterr:
 	p9_free_req(c, req);
@@ -732,7 +730,7 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
 		goto reterr;
 
 	err = p9_check_errors(c, req);
-	trace_9p_client_res(c, type, req->rc->tag, err);
+	trace_9p_client_res(c, type, req->rc.tag, err);
 	if (!err)
 		return req;
 reterr:
@@ -814,7 +812,7 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
 		goto reterr;
 
 	err = p9_check_zc_errors(c, req, uidata, in_hdrlen);
-	trace_9p_client_res(c, type, req->rc->tag, err);
+	trace_9p_client_res(c, type, req->rc.tag, err);
 	if (!err)
 		return req;
 reterr:
@@ -897,10 +895,10 @@ static int p9_client_version(struct p9_client *c)
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
-	err = p9pdu_readf(req->rc, c->proto_version, "ds", &msize, &version);
+	err = p9pdu_readf(&req->rc, c->proto_version, "ds", &msize, &version);
 	if (err) {
 		p9_debug(P9_DEBUG_9P, "version error %d\n", err);
-		trace_9p_protocol_dump(c, req->rc);
+		trace_9p_protocol_dump(c, &req->rc);
 		goto error;
 	}
 
@@ -1049,9 +1047,9 @@ struct p9_fid *p9_client_attach(struct p9_client *clnt, struct p9_fid *afid,
 		goto error;
 	}
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "Q", &qid);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "Q", &qid);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		p9_free_req(clnt, req);
 		goto error;
 	}
@@ -1106,9 +1104,9 @@ struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname,
 		goto error;
 	}
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "R", &nwqids, &wqids);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "R", &nwqids, &wqids);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		p9_free_req(clnt, req);
 		goto clunk_fid;
 	}
@@ -1173,9 +1171,9 @@ int p9_client_open(struct p9_fid *fid, int mode)
 		goto error;
 	}
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "Qd", &qid, &iounit);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "Qd", &qid, &iounit);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		goto free_and_error;
 	}
 
@@ -1217,9 +1215,9 @@ int p9_client_create_dotl(struct p9_fid *ofid, const char *name, u32 flags, u32
 		goto error;
 	}
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "Qd", qid, &iounit);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "Qd", qid, &iounit);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		goto free_and_error;
 	}
 
@@ -1262,9 +1260,9 @@ int p9_client_fcreate(struct p9_fid *fid, const char *name, u32 perm, int mode,
 		goto error;
 	}
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "Qd", &qid, &iounit);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "Qd", &qid, &iounit);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		goto free_and_error;
 	}
 
@@ -1301,9 +1299,9 @@ int p9_client_symlink(struct p9_fid *dfid, const char *name,
 		goto error;
 	}
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "Q", qid);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "Q", qid);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		goto free_and_error;
 	}
 
@@ -1499,10 +1497,10 @@ p9_client_read(struct p9_fid *fid, u64 offset, struct iov_iter *to, int *err)
 			break;
 		}
 
-		*err = p9pdu_readf(req->rc, clnt->proto_version,
+		*err = p9pdu_readf(&req->rc, clnt->proto_version,
 				   "D", &count, &dataptr);
 		if (*err) {
-			trace_9p_protocol_dump(clnt, req->rc);
+			trace_9p_protocol_dump(clnt, &req->rc);
 			p9_free_req(clnt, req);
 			break;
 		}
@@ -1572,9 +1570,9 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
 			break;
 		}
 
-		*err = p9pdu_readf(req->rc, clnt->proto_version, "d", &count);
+		*err = p9pdu_readf(&req->rc, clnt->proto_version, "d", &count);
 		if (*err) {
-			trace_9p_protocol_dump(clnt, req->rc);
+			trace_9p_protocol_dump(clnt, &req->rc);
 			p9_free_req(clnt, req);
 			break;
 		}
@@ -1616,9 +1614,9 @@ struct p9_wstat *p9_client_stat(struct p9_fid *fid)
 		goto error;
 	}
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "wS", &ignored, ret);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "wS", &ignored, ret);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		p9_free_req(clnt, req);
 		goto error;
 	}
@@ -1669,9 +1667,9 @@ struct p9_stat_dotl *p9_client_getattr_dotl(struct p9_fid *fid,
 		goto error;
 	}
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "A", ret);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "A", ret);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		p9_free_req(clnt, req);
 		goto error;
 	}
@@ -1821,11 +1819,11 @@ int p9_client_statfs(struct p9_fid *fid, struct p9_rstatfs *sb)
 		goto error;
 	}
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "ddqqqqqqd", &sb->type,
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "ddqqqqqqd", &sb->type,
 		&sb->bsize, &sb->blocks, &sb->bfree, &sb->bavail,
 		&sb->files, &sb->ffree, &sb->fsid, &sb->namelen);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		p9_free_req(clnt, req);
 		goto error;
 	}
@@ -1929,9 +1927,9 @@ struct p9_fid *p9_client_xattrwalk(struct p9_fid *file_fid,
 		err = PTR_ERR(req);
 		goto error;
 	}
-	err = p9pdu_readf(req->rc, clnt->proto_version, "q", attr_size);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "q", attr_size);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		p9_free_req(clnt, req);
 		goto clunk_fid;
 	}
@@ -2017,9 +2015,9 @@ int p9_client_readdir(struct p9_fid *fid, char *data, u32 count, u64 offset)
 		goto error;
 	}
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "D", &count, &dataptr);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "D", &count, &dataptr);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		goto free_and_error;
 	}
 	if (rsize < count) {
@@ -2058,9 +2056,9 @@ int p9_client_mknod_dotl(struct p9_fid *fid, const char *name, int mode,
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "Q", qid);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "Q", qid);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		goto error;
 	}
 	p9_debug(P9_DEBUG_9P, "<<< RMKNOD qid %x.%llx.%x\n", qid->type,
@@ -2089,9 +2087,9 @@ int p9_client_mkdir_dotl(struct p9_fid *fid, const char *name, int mode,
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "Q", qid);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "Q", qid);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		goto error;
 	}
 	p9_debug(P9_DEBUG_9P, "<<< RMKDIR qid %x.%llx.%x\n", qid->type,
@@ -2124,9 +2122,9 @@ int p9_client_lock_dotl(struct p9_fid *fid, struct p9_flock *flock, u8 *status)
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "b", status);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "b", status);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		goto error;
 	}
 	p9_debug(P9_DEBUG_9P, "<<< RLOCK status %i\n", *status);
@@ -2155,11 +2153,11 @@ int p9_client_getlock_dotl(struct p9_fid *fid, struct p9_getlock *glock)
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "bqqds", &glock->type,
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "bqqds", &glock->type,
 			&glock->start, &glock->length, &glock->proc_id,
 			&glock->client_id);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		goto error;
 	}
 	p9_debug(P9_DEBUG_9P, "<<< RGETLOCK type %i start %lld length %lld "
@@ -2185,9 +2183,9 @@ int p9_client_readlink(struct p9_fid *fid, char **target)
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
-	err = p9pdu_readf(req->rc, clnt->proto_version, "s", target);
+	err = p9pdu_readf(&req->rc, clnt->proto_version, "s", target);
 	if (err) {
-		trace_9p_protocol_dump(clnt, req->rc);
+		trace_9p_protocol_dump(clnt, &req->rc);
 		goto error;
 	}
 	p9_debug(P9_DEBUG_9P, "<<< RREADLINK target %s\n", *target);

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 5/6] 9p: Use a slab for allocating requests
  2018-07-18 11:49     ` Matthew Wilcox
@ 2018-07-18 12:46       ` Dominique Martinet
  0 siblings, 0 replies; 23+ messages in thread
From: Dominique Martinet @ 2018-07-18 12:46 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Greg Kurz, v9fs-developer, Latchesar Ionkov, Eric Van Hensbergen,
	Ron Minnich, linux-kernel, linux-fsdevel

Matthew Wilcox wrote on Wed, Jul 18, 2018:
> I must admit to having not looked at the fcall aspect of this.  kmalloc
> is implemented in terms of slab, so it's not going to be much slower than
> using a dedicatd slab (a few instructions to figure out which slab cache
> to use).

Yeah, kmalloc-8, kmalloc-16 etc. This is fast and efficient if the
requested amount is a power of two as you said below.


> What would help is splitting the p9_fcall struct from the alloc_size.
> kmalloc is pretty effective at allocating power-of-two sized structures,
> and 8k + 32 bytes is approximately the worst case scenario.  I'd do this
> by embedding the p9_fcall into the p9_req_t and only allocating the
> msize, like this:

Good idea, I'll include something like this in my benchmarks.

I'm still worried about big allocs for bigger rdma msizes (we've had our
share of troubles with mellanox allocating some big 256k queue pairs and
failing pretty often on very memory-fragmented servers, so now that's
fixed I'd rather avoid reproducing the same kind of pattern if
possible...), but the root of the problem is that it's a scam to call
that rdma when the protocol only allows send/recv operations... I guess
this could finally be motivation to work on a v2 allowing for zero-copy
and some scatter-gather lists to allow using fragmented memory.

Thanks for the fast reply as usual.

> (I only converted client.c to the new embedded fcall way; all the
> transports also need massaging)
> 
> Regardless of whether any of the other patches go in, this patch is
> worth having; it's really wasting slab allocator memory.

The other patchs don't have anything wrong, but I agree I'd want to keep
this as well unless the numbers are really horrible.
Still needs checking, though!

> diff --git a/include/net/9p/client.h b/include/net/9p/client.h
> index a4dc42c53d18..4b4ac1362ad5 100644
> --- a/include/net/9p/client.h
> +++ b/include/net/9p/client.h
> @@ -95,8 +95,8 @@ struct p9_req_t {
>  	int status;
>  	int t_err;
>  	wait_queue_head_t wq;
> -	struct p9_fcall *tc;
> -	struct p9_fcall *rc;
> +	struct p9_fcall tc;
> +	struct p9_fcall rc;
>  	void *aux;
>  	struct list_head req_list;
>  };
> diff --git a/net/9p/client.c b/net/9p/client.c
> index ecf5dd269f4c..058dfbebdaa2 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -230,15 +230,13 @@ static int parse_opts(char *opts, struct p9_client *clnt)
>  	return ret;
>  }
>  
> -static struct p9_fcall *p9_fcall_alloc(int alloc_msize)
> +static int p9_fcall_alloc(struct p9_fcall *fc, int alloc_msize)
>  {
> -	struct p9_fcall *fc;
> -	fc = kmalloc(sizeof(struct p9_fcall) + alloc_msize, GFP_NOFS);
> -	if (!fc)
> -		return NULL;
> +	fc->sdata = kmalloc(alloc_msize, GFP_NOFS);
> +	if (!fc->sdata)
> +		return -ENOMEM;
>  	fc->capacity = alloc_msize;
> -	fc->sdata = (char *) fc + sizeof(struct p9_fcall);
> -	return fc;
> +	return 0;
>  }
>  
>  static struct kmem_cache *p9_req_cache;
> @@ -262,13 +260,13 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
>  	if (!req)
>  		return NULL;
>  
> -	req->tc = p9_fcall_alloc(alloc_msize);
> -	req->rc = p9_fcall_alloc(alloc_msize);
> -	if (!req->tc || !req->rc)
> +	if (p9_fcall_alloc(&req->tc, alloc_msize))
> +		goto free;
> +	if (p9_fcall_alloc(&req->rc, alloc_msize))
>  		goto free;
>  
> -	p9pdu_reset(req->tc);
> -	p9pdu_reset(req->rc);
> +	p9pdu_reset(&req->tc);
> +	p9pdu_reset(&req->rc);
>  	req->status = REQ_STATUS_ALLOC;
>  	init_waitqueue_head(&req->wq);
>  	INIT_LIST_HEAD(&req->req_list);
> @@ -280,7 +278,7 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
>  				GFP_NOWAIT);
>  	else
>  		tag = idr_alloc(&c->reqs, req, 0, P9_NOTAG, GFP_NOWAIT);
> -	req->tc->tag = tag;
> +	req->tc.tag = tag;
>  	spin_unlock_irq(&c->lock);
>  	idr_preload_end();
>  	if (tag < 0)
> @@ -289,8 +287,8 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
>  	return req;
>  
>  free:
> -	kfree(req->tc);
> -	kfree(req->rc);
> +	kfree(req->tc.sdata);
> +	kfree(req->rc.sdata);
>  	kmem_cache_free(p9_req_cache, req);
>  	return ERR_PTR(-ENOMEM);
>  }
> @@ -329,14 +327,14 @@ EXPORT_SYMBOL(p9_tag_lookup);
>  static void p9_free_req(struct p9_client *c, struct p9_req_t *r)
>  {
>  	unsigned long flags;
> -	u16 tag = r->tc->tag;
> +	u16 tag = r->tc.tag;
>  
>  	p9_debug(P9_DEBUG_MUX, "clnt %p req %p tag: %d\n", c, r, tag);
>  	spin_lock_irqsave(&c->lock, flags);
>  	idr_remove(&c->reqs, tag);
>  	spin_unlock_irqrestore(&c->lock, flags);
> -	kfree(r->tc);
> -	kfree(r->rc);
> +	kfree(r->tc.sdata);
> +	kfree(r->rc.sdata);
>  	kmem_cache_free(p9_req_cache, r);
>  }
>  
> @@ -368,7 +366,7 @@ static void p9_tag_cleanup(struct p9_client *c)
>   */
>  void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status)
>  {
> -	p9_debug(P9_DEBUG_MUX, " tag %d\n", req->tc->tag);
> +	p9_debug(P9_DEBUG_MUX, " tag %d\n", req->tc.tag);
>  
>  	/*
>  	 * This barrier is needed to make sure any change made to req before
> @@ -378,7 +376,7 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status)
>  	req->status = status;
>  
>  	wake_up(&req->wq);
> -	p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc->tag);
> +	p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc.tag);
>  }
>  EXPORT_SYMBOL(p9_client_cb);
>  
> @@ -448,12 +446,12 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
>  	int err;
>  	int ecode;
>  
> -	err = p9_parse_header(req->rc, NULL, &type, NULL, 0);
> +	err = p9_parse_header(&req->rc, NULL, &type, NULL, 0);
>  	/*
>  	 * dump the response from server
>  	 * This should be after check errors which poplulate pdu_fcall.
>  	 */
> -	trace_9p_protocol_dump(c, req->rc);
> +	trace_9p_protocol_dump(c, &req->rc);
>  	if (err) {
>  		p9_debug(P9_DEBUG_ERROR, "couldn't parse header %d\n", err);
>  		return err;
> @@ -463,7 +461,7 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
>  
>  	if (!p9_is_proto_dotl(c)) {
>  		char *ename;
> -		err = p9pdu_readf(req->rc, c->proto_version, "s?d",
> +		err = p9pdu_readf(&req->rc, c->proto_version, "s?d",
>  				  &ename, &ecode);
>  		if (err)
>  			goto out_err;
> @@ -479,7 +477,7 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
>  		}
>  		kfree(ename);
>  	} else {
> -		err = p9pdu_readf(req->rc, c->proto_version, "d", &ecode);
> +		err = p9pdu_readf(&req->rc, c->proto_version, "d", &ecode);
>  		err = -ecode;
>  
>  		p9_debug(P9_DEBUG_9P, "<<< RLERROR (%d)\n", -ecode);
> @@ -513,12 +511,12 @@ static int p9_check_zc_errors(struct p9_client *c, struct p9_req_t *req,
>  	int8_t type;
>  	char *ename = NULL;
>  
> -	err = p9_parse_header(req->rc, NULL, &type, NULL, 0);
> +	err = p9_parse_header(&req->rc, NULL, &type, NULL, 0);
>  	/*
>  	 * dump the response from server
>  	 * This should be after parse_header which poplulate pdu_fcall.
>  	 */
> -	trace_9p_protocol_dump(c, req->rc);
> +	trace_9p_protocol_dump(c, &req->rc);
>  	if (err) {
>  		p9_debug(P9_DEBUG_ERROR, "couldn't parse header %d\n", err);
>  		return err;
> @@ -533,13 +531,13 @@ static int p9_check_zc_errors(struct p9_client *c, struct p9_req_t *req,
>  		/* 7 = header size for RERROR; */
>  		int inline_len = in_hdrlen - 7;
>  
> -		len =  req->rc->size - req->rc->offset;
> +		len = req->rc.size - req->rc.offset;
>  		if (len > (P9_ZC_HDR_SZ - 7)) {
>  			err = -EFAULT;
>  			goto out_err;
>  		}
>  
> -		ename = &req->rc->sdata[req->rc->offset];
> +		ename = &req->rc.sdata[req->rc.offset];
>  		if (len > inline_len) {
>  			/* We have error in external buffer */
>  			if (!copy_from_iter_full(ename + inline_len,
> @@ -549,7 +547,7 @@ static int p9_check_zc_errors(struct p9_client *c, struct p9_req_t *req,
>  			}
>  		}
>  		ename = NULL;
> -		err = p9pdu_readf(req->rc, c->proto_version, "s?d",
> +		err = p9pdu_readf(&req->rc, c->proto_version, "s?d",
>  				  &ename, &ecode);
>  		if (err)
>  			goto out_err;
> @@ -565,7 +563,7 @@ static int p9_check_zc_errors(struct p9_client *c, struct p9_req_t *req,
>  		}
>  		kfree(ename);
>  	} else {
> -		err = p9pdu_readf(req->rc, c->proto_version, "d", &ecode);
> +		err = p9pdu_readf(&req->rc, c->proto_version, "d", &ecode);
>  		err = -ecode;
>  
>  		p9_debug(P9_DEBUG_9P, "<<< RLERROR (%d)\n", -ecode);
> @@ -598,7 +596,7 @@ static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq)
>  	int16_t oldtag;
>  	int err;
>  
> -	err = p9_parse_header(oldreq->tc, NULL, NULL, &oldtag, 1);
> +	err = p9_parse_header(&oldreq->tc, NULL, NULL, &oldtag, 1);
>  	if (err)
>  		return err;
>  
> @@ -642,12 +640,12 @@ static struct p9_req_t *p9_client_prepare_req(struct p9_client *c,
>  		return req;
>  
>  	/* marshall the data */
> -	p9pdu_prepare(req->tc, req->tc->tag, type);
> -	err = p9pdu_vwritef(req->tc, c->proto_version, fmt, ap);
> +	p9pdu_prepare(&req->tc, req->tc.tag, type);
> +	err = p9pdu_vwritef(&req->tc, c->proto_version, fmt, ap);
>  	if (err)
>  		goto reterr;
> -	p9pdu_finalize(c, req->tc);
> -	trace_9p_client_req(c, type, req->tc->tag);
> +	p9pdu_finalize(c, &req->tc);
> +	trace_9p_client_req(c, type, req->tc.tag);
>  	return req;
>  reterr:
>  	p9_free_req(c, req);
> @@ -732,7 +730,7 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
>  		goto reterr;
>  
>  	err = p9_check_errors(c, req);
> -	trace_9p_client_res(c, type, req->rc->tag, err);
> +	trace_9p_client_res(c, type, req->rc.tag, err);
>  	if (!err)
>  		return req;
>  reterr:
> @@ -814,7 +812,7 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
>  		goto reterr;
>  
>  	err = p9_check_zc_errors(c, req, uidata, in_hdrlen);
> -	trace_9p_client_res(c, type, req->rc->tag, err);
> +	trace_9p_client_res(c, type, req->rc.tag, err);
>  	if (!err)
>  		return req;
>  reterr:
> @@ -897,10 +895,10 @@ static int p9_client_version(struct p9_client *c)
>  	if (IS_ERR(req))
>  		return PTR_ERR(req);
>  
> -	err = p9pdu_readf(req->rc, c->proto_version, "ds", &msize, &version);
> +	err = p9pdu_readf(&req->rc, c->proto_version, "ds", &msize, &version);
>  	if (err) {
>  		p9_debug(P9_DEBUG_9P, "version error %d\n", err);
> -		trace_9p_protocol_dump(c, req->rc);
> +		trace_9p_protocol_dump(c, &req->rc);
>  		goto error;
>  	}
>  
> @@ -1049,9 +1047,9 @@ struct p9_fid *p9_client_attach(struct p9_client *clnt, struct p9_fid *afid,
>  		goto error;
>  	}
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "Q", &qid);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "Q", &qid);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		p9_free_req(clnt, req);
>  		goto error;
>  	}
> @@ -1106,9 +1104,9 @@ struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname,
>  		goto error;
>  	}
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "R", &nwqids, &wqids);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "R", &nwqids, &wqids);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		p9_free_req(clnt, req);
>  		goto clunk_fid;
>  	}
> @@ -1173,9 +1171,9 @@ int p9_client_open(struct p9_fid *fid, int mode)
>  		goto error;
>  	}
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "Qd", &qid, &iounit);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "Qd", &qid, &iounit);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		goto free_and_error;
>  	}
>  
> @@ -1217,9 +1215,9 @@ int p9_client_create_dotl(struct p9_fid *ofid, const char *name, u32 flags, u32
>  		goto error;
>  	}
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "Qd", qid, &iounit);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "Qd", qid, &iounit);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		goto free_and_error;
>  	}
>  
> @@ -1262,9 +1260,9 @@ int p9_client_fcreate(struct p9_fid *fid, const char *name, u32 perm, int mode,
>  		goto error;
>  	}
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "Qd", &qid, &iounit);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "Qd", &qid, &iounit);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		goto free_and_error;
>  	}
>  
> @@ -1301,9 +1299,9 @@ int p9_client_symlink(struct p9_fid *dfid, const char *name,
>  		goto error;
>  	}
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "Q", qid);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "Q", qid);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		goto free_and_error;
>  	}
>  
> @@ -1499,10 +1497,10 @@ p9_client_read(struct p9_fid *fid, u64 offset, struct iov_iter *to, int *err)
>  			break;
>  		}
>  
> -		*err = p9pdu_readf(req->rc, clnt->proto_version,
> +		*err = p9pdu_readf(&req->rc, clnt->proto_version,
>  				   "D", &count, &dataptr);
>  		if (*err) {
> -			trace_9p_protocol_dump(clnt, req->rc);
> +			trace_9p_protocol_dump(clnt, &req->rc);
>  			p9_free_req(clnt, req);
>  			break;
>  		}
> @@ -1572,9 +1570,9 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
>  			break;
>  		}
>  
> -		*err = p9pdu_readf(req->rc, clnt->proto_version, "d", &count);
> +		*err = p9pdu_readf(&req->rc, clnt->proto_version, "d", &count);
>  		if (*err) {
> -			trace_9p_protocol_dump(clnt, req->rc);
> +			trace_9p_protocol_dump(clnt, &req->rc);
>  			p9_free_req(clnt, req);
>  			break;
>  		}
> @@ -1616,9 +1614,9 @@ struct p9_wstat *p9_client_stat(struct p9_fid *fid)
>  		goto error;
>  	}
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "wS", &ignored, ret);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "wS", &ignored, ret);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		p9_free_req(clnt, req);
>  		goto error;
>  	}
> @@ -1669,9 +1667,9 @@ struct p9_stat_dotl *p9_client_getattr_dotl(struct p9_fid *fid,
>  		goto error;
>  	}
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "A", ret);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "A", ret);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		p9_free_req(clnt, req);
>  		goto error;
>  	}
> @@ -1821,11 +1819,11 @@ int p9_client_statfs(struct p9_fid *fid, struct p9_rstatfs *sb)
>  		goto error;
>  	}
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "ddqqqqqqd", &sb->type,
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "ddqqqqqqd", &sb->type,
>  		&sb->bsize, &sb->blocks, &sb->bfree, &sb->bavail,
>  		&sb->files, &sb->ffree, &sb->fsid, &sb->namelen);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		p9_free_req(clnt, req);
>  		goto error;
>  	}
> @@ -1929,9 +1927,9 @@ struct p9_fid *p9_client_xattrwalk(struct p9_fid *file_fid,
>  		err = PTR_ERR(req);
>  		goto error;
>  	}
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "q", attr_size);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "q", attr_size);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		p9_free_req(clnt, req);
>  		goto clunk_fid;
>  	}
> @@ -2017,9 +2015,9 @@ int p9_client_readdir(struct p9_fid *fid, char *data, u32 count, u64 offset)
>  		goto error;
>  	}
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "D", &count, &dataptr);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "D", &count, &dataptr);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		goto free_and_error;
>  	}
>  	if (rsize < count) {
> @@ -2058,9 +2056,9 @@ int p9_client_mknod_dotl(struct p9_fid *fid, const char *name, int mode,
>  	if (IS_ERR(req))
>  		return PTR_ERR(req);
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "Q", qid);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "Q", qid);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		goto error;
>  	}
>  	p9_debug(P9_DEBUG_9P, "<<< RMKNOD qid %x.%llx.%x\n", qid->type,
> @@ -2089,9 +2087,9 @@ int p9_client_mkdir_dotl(struct p9_fid *fid, const char *name, int mode,
>  	if (IS_ERR(req))
>  		return PTR_ERR(req);
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "Q", qid);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "Q", qid);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		goto error;
>  	}
>  	p9_debug(P9_DEBUG_9P, "<<< RMKDIR qid %x.%llx.%x\n", qid->type,
> @@ -2124,9 +2122,9 @@ int p9_client_lock_dotl(struct p9_fid *fid, struct p9_flock *flock, u8 *status)
>  	if (IS_ERR(req))
>  		return PTR_ERR(req);
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "b", status);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "b", status);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		goto error;
>  	}
>  	p9_debug(P9_DEBUG_9P, "<<< RLOCK status %i\n", *status);
> @@ -2155,11 +2153,11 @@ int p9_client_getlock_dotl(struct p9_fid *fid, struct p9_getlock *glock)
>  	if (IS_ERR(req))
>  		return PTR_ERR(req);
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "bqqds", &glock->type,
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "bqqds", &glock->type,
>  			&glock->start, &glock->length, &glock->proc_id,
>  			&glock->client_id);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		goto error;
>  	}
>  	p9_debug(P9_DEBUG_9P, "<<< RGETLOCK type %i start %lld length %lld "
> @@ -2185,9 +2183,9 @@ int p9_client_readlink(struct p9_fid *fid, char **target)
>  	if (IS_ERR(req))
>  		return PTR_ERR(req);
>  
> -	err = p9pdu_readf(req->rc, clnt->proto_version, "s", target);
> +	err = p9pdu_readf(&req->rc, clnt->proto_version, "s", target);
>  	if (err) {
> -		trace_9p_protocol_dump(clnt, req->rc);
> +		trace_9p_protocol_dump(clnt, &req->rc);
>  		goto error;
>  	}
>  	p9_debug(P9_DEBUG_9P, "<<< RREADLINK target %s\n", *target);

-- 
Dominique

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, back to index

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-11 21:02 [PATCH v2 0/6] 9p: Use IDRs more effectively Matthew Wilcox
2018-07-11 21:02 ` [PATCH v2 1/6] 9p: Fix comment on smp_wmb Matthew Wilcox
2018-07-12 11:55   ` [V9fs-developer] " Greg Kurz
2018-07-11 21:02 ` [PATCH v2 2/6] 9p: Change p9_fid_create calling convention Matthew Wilcox
2018-07-12  2:15   ` [V9fs-developer] " piaojun
2018-07-12 11:56   ` Greg Kurz
2018-07-13  1:18   ` jiangyiwen
2018-07-11 21:02 ` [PATCH v2 3/6] 9p: Replace the fidlist with an IDR Matthew Wilcox
2018-07-12 11:17   ` Dominique Martinet
2018-07-12 11:23     ` Matthew Wilcox
2018-07-12 11:30       ` Dominique Martinet
2018-07-13  2:05   ` [V9fs-developer] " jiangyiwen
2018-07-13  2:48     ` Matthew Wilcox
2018-07-11 21:02 ` [PATCH v2 4/6] 9p: Embed wait_queue_head into p9_req_t Matthew Wilcox
2018-07-12 14:36   ` [V9fs-developer] " Greg Kurz
2018-07-12 14:40     ` Dominique Martinet
2018-07-12 14:59       ` Greg Kurz
2018-07-11 21:02 ` [PATCH v2 5/6] 9p: Use a slab for allocating requests Matthew Wilcox
2018-07-18 10:05   ` Dominique Martinet
2018-07-18 11:49     ` Matthew Wilcox
2018-07-18 12:46       ` Dominique Martinet
2018-07-11 21:02 ` [PATCH v2 6/6] 9p: Remove p9_idpool Matthew Wilcox
2018-07-11 23:37 ` [PATCH v2 0/6] 9p: Use IDRs more effectively Dominique Martinet

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox