remove msize limit in virtio transport

The amount of elements in a scatter/gather list is limited to approximately 128 elements. To allow going beyond that limit with subsequent patches, pave the way by turning the one- dimensional sg list array into a two-dimensional array, i.e:

sg[128]

becomes

sgl[nsgl][SG_MAX_SINGLE_ALLOC]

As the value of 'nsgl' is exactly (still) 1 in this commit and the compile-time (compiler and architecture dependent) value of 'SG_MAX_SINGLE_ALLOC' equals approximately the previous hard coded 128 elements, this commit is therefore more of a preparatory refactoring then actual behaviour change.

A custom struct virtqueue_sg is defined instead of using shared API struct sg_table, because the latter would not allow to resize the table after allocation. sg_append_table API OTOH would not fit either, because it requires a list of pages beforehand upon allocation. And both APIs only support all-or-nothing allocation.

Signed-off-by Christian Schoenebeck <[email protected]>

---

The question is whether that should really become 9p specifc SG list code, or whether it should rather be squeezed into shared SG list code base. Opinions by maintainers needed.

 net/9p/trans_virtio.c | 193 ++++++++++++++++++++++++++++++++----------
 1 file changed, 147 insertions(+), 46 deletions(-)

net/9p/trans_virtio.c +27 -6

index 18bdfa64b934..f63cd1b08bca 100644
@@ -36,7 +36,31 @@
 #include <linux/virtio_9p.h>
 #include "trans_common.h"
 
-#define VIRTQUEUE_DEFAULT_NUM	128
+/**
+ * struct virtqueue_sg - (chained) scatter gather lists for virtqueue data
+ * transmission
+ * @nsgl: amount of elements (in first dimension) of array field @sgl
+ * @sgl: two-dimensional array, i.e. sgl[nsgl][SG_MAX_SINGLE_ALLOC]
+ */
+struct virtqueue_sg {
+	unsigned int nsgl;
+	struct scatterlist *sgl[];
+};
+
+/*
+ * Default value for field nsgl in struct virtqueue_sg, which defines the
+ * initial virtio data transmission capacity when this virtio transport is
+ * probed.
+ */
+#define VIRTQUEUE_SG_NSGL_DEFAULT 1
+
+/* maximum value for field nsgl in struct virtqueue_sg */
+#define VIRTQUEUE_SG_NSGL_MAX						\
+	((PAGE_SIZE - sizeof(struct virtqueue_sg)) /			\
+	sizeof(struct scatterlist *))					\
+
+/* last entry per sg list is used for chaining (pointer to next list) */
+#define SG_USER_PAGES_PER_LIST	(SG_MAX_SINGLE_ALLOC - 1)
 
 /* a single mutex to manage channel initialization and attachment */
 static DEFINE_MUTEX(virtio_9p_lock);

@@ -53,8 +77,7 @@ static atomic_t vp_pinned = ATOMIC_INIT(0);
  * @ring_bufs_avail: flag to indicate there is some available in the ring buf
  * @vc_wq: wait queue for waiting for thing to be added to ring buf
  * @p9_max_pages: maximum number of pinned pages
- * @sg: scatter gather list which is used to pack a request (protected?)
- * @sg_n: amount of elements in sg array
+ * @vq_sg: table of scatter gather lists, which are used to pack a request
  * @chan_list: linked list of channels
  *
  * We keep all per-channel information in a structure.

@@ -77,9 +100,7 @@ struct virtio_chan {
 	 * will be placing it in each channel.
 	 */
 	unsigned long p9_max_pages;
-	/* Scatterlist: can be too big for stack. */
-	struct scatterlist *sg;
-	size_t sg_n;
+	struct virtqueue_sg *vq_sg;
 	/**
 	 * @tag: name to identify a mount null terminated
 	 */

@@ -96,6 +117,92 @@ static unsigned int rest_of_page(void *data)
 	return PAGE_SIZE - offset_in_page(data);
 }
 

net/9p/trans_virtio.c +110 -40

+	return &vq_sg->sgl[node][leaf];
+}
+
+/**
+ * vq_sg_npages - returns total number of individual user pages in passed
+ * scatter gather lists
+ * @vq_sg: scatter gather lists to be counted
+ */
+static size_t vq_sg_npages(struct virtqueue_sg *vq_sg)
+{
+	return vq_sg->nsgl * SG_USER_PAGES_PER_LIST;
+}
+
+/**
+ * vq_sg_free - free all memory previously allocated for @vq_sg
+ * @vq_sg: scatter gather lists to be freed
+ */
+static void vq_sg_free(struct virtqueue_sg *vq_sg)
+{
+	unsigned int i;
+
+	if (!vq_sg)
+		return;
+
+	for (i = 0; i < vq_sg->nsgl; ++i) {
+		kfree(vq_sg->sgl[i]);
+	}
+	kfree(vq_sg);
+}
+
+/**
+ * vq_sg_alloc - allocates and returns @nsgl scatter gather lists
+ * @nsgl: amount of scatter gather lists to be allocated
+ * If @nsgl is larger than one then chained lists are used if supported by
+ * architecture.
+ */
+static struct virtqueue_sg *vq_sg_alloc(unsigned int nsgl)
+{
+	struct virtqueue_sg *vq_sg;
+	unsigned int i;
+
+	BUG_ON(!nsgl || nsgl > VIRTQUEUE_SG_NSGL_MAX);
+#ifdef CONFIG_ARCH_NO_SG_CHAIN
+	if (WARN_ON_ONCE(nsgl > 1))
+		return NULL;
+#endif
+
+	vq_sg = kzalloc(sizeof(struct virtqueue_sg) +
+			nsgl * sizeof(struct scatterlist *),
+			GFP_KERNEL);
+
+	if (!vq_sg)
+		return NULL;
+
+	vq_sg->nsgl = nsgl;
+
+	for (i = 0; i < nsgl; ++i) {
+		vq_sg->sgl[i] = kmalloc_array(
+			SG_MAX_SINGLE_ALLOC, sizeof(struct scatterlist),
+			GFP_KERNEL
+		);
+		if (!vq_sg->sgl[i]) {
+			vq_sg_free(vq_sg);
+			return NULL;
+		}
+		sg_init_table(vq_sg->sgl[i], SG_MAX_SINGLE_ALLOC);
+		if (i) {
+			/* chain the lists */
+			sg_chain(vq_sg->sgl[i - 1], SG_MAX_SINGLE_ALLOC,
+				 vq_sg->sgl[i]);
+		}
+	}
+	sg_mark_end(&vq_sg->sgl[nsgl - 1][SG_MAX_SINGLE_ALLOC - 1]);
+	return vq_sg;
+}
+
 /**
  * p9_virtio_close - reclaim resources of a channel
  * @client: client instance

@@ -158,9 +265,8 @@ static void req_done(struct virtqueue *vq)
 
 /**
  * pack_sg_list - pack a scatter gather list from a linear buffer
- * @sg: scatter/gather list to pack into
+ * @vq_sg: scatter/gather lists to pack into
  * @start: which segment of the sg_list to start at
- * @limit: maximum segment to pack data to
  * @data: data to pack into scatter/gather list
  * @count: amount of data to pack into the scatter/gather list
  *

@@ -170,11 +276,12 @@ static void req_done(struct virtqueue *vq)
  *
  */
 
-static int pack_sg_list(struct scatterlist *sg, int start,
-			int limit, char *data, int count)
+static int pack_sg_list(struct virtqueue_sg *vq_sg, int start,
+			char *data, int count)
 {
 	int s;
 	int index = start;
+	size_t limit = vq_sg_npages(vq_sg);
 
 	while (count) {
 		s = rest_of_page(data);

@@ -182,13 +289,13 @@ static int pack_sg_list(struct scatterlist *sg, int start,
 			s = count;
 		BUG_ON(index >= limit);
 		/* Make sure we don't terminate early. */
-		sg_unmark_end(&sg[index]);
-		sg_set_buf(&sg[index++], data, s);
+		sg_unmark_end(vq_sg_page(vq_sg, index));
+		sg_set_buf(vq_sg_page(vq_sg, index++), data, s);
 		count -= s;
 		data += s;
 	}
 	if (index-start)
-		sg_mark_end(&sg[index - 1]);
+		sg_mark_end(vq_sg_page(vq_sg, index - 1));
 	return index-start;
 }
 

@@ -208,21 +315,21 @@ static int p9_virtio_cancelled(struct p9_client *client, struct p9_req_t *req)
 /**
  * pack_sg_list_p - Just like pack_sg_list. Instead of taking a buffer,
  * this takes a list of pages.
- * @sg: scatter/gather list to pack into
+ * @vq_sg: scatter/gather lists to pack into
  * @start: which segment of the sg_list to start at
- * @limit: maximum number of pages in sg list.
  * @pdata: a list of pages to add into sg.
  * @nr_pages: number of pages to pack into the scatter/gather list
  * @offs: amount of data in the beginning of first page _not_ to pack
  * @count: amount of data to pack into the scatter/gather list
  */
 static int
-pack_sg_list_p(struct scatterlist *sg, int start, int limit,
+pack_sg_list_p(struct virtqueue_sg *vq_sg, int start,
 	       struct page **pdata, int nr_pages, size_t offs, int count)
 {
 	int i = 0, s;
 	int data_off = offs;
 	int index = start;
+	size_t limit = vq_sg_npages(vq_sg);
 
 	BUG_ON(nr_pages > (limit - start));
 	/*

@@ -235,15 +342,16 @@ pack_sg_list_p(struct scatterlist *sg, int start, int limit,
 			s = count;
 		BUG_ON(index >= limit);
 		/* Make sure we don't terminate early. */
-		sg_unmark_end(&sg[index]);
-		sg_set_page(&sg[index++], pdata[i++], s, data_off);
+		sg_unmark_end(vq_sg_page(vq_sg, index));
+		sg_set_page(vq_sg_page(vq_sg, index++), pdata[i++], s,
+			    data_off);
 		data_off = 0;
 		count -= s;
 		nr_pages--;
 	}
 
 	if (index-start)
-		sg_mark_end(&sg[index - 1]);
+		sg_mark_end(vq_sg_page(vq_sg, index - 1));
 	return index - start;
 }
 

@@ -271,15 +379,13 @@ p9_virtio_request(struct p9_client *client, struct p9_req_t *req)
 
 	out_sgs = in_sgs = 0;
 	/* Handle out VirtIO ring buffers */
-	out = pack_sg_list(chan->sg, 0,
-			   chan->sg_n, req->tc.sdata, req->tc.size);
+	out = pack_sg_list(chan->vq_sg, 0, req->tc.sdata, req->tc.size);
 	if (out)
-		sgs[out_sgs++] = chan->sg;
+		sgs[out_sgs++] = vq_sg_page(chan->vq_sg, 0);
 
-	in = pack_sg_list(chan->sg, out,
-			  chan->sg_n, req->rc.sdata, req->rc.capacity);
+	in = pack_sg_list(chan->vq_sg, out, req->rc.sdata, req->rc.capacity);
 	if (in)
-		sgs[out_sgs + in_sgs++] = chan->sg + out;
+		sgs[out_sgs + in_sgs++] = vq_sg_page(chan->vq_sg, out);
 
 	err = virtqueue_add_sgs(chan->vq, sgs, out_sgs, in_sgs, req,
 				GFP_ATOMIC);

@@ -448,16 +554,15 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 	out_sgs = in_sgs = 0;
 
 	/* out data */
-	out = pack_sg_list(chan->sg, 0,
-			   chan->sg_n, req->tc.sdata, req->tc.size);
+	out = pack_sg_list(chan->vq_sg, 0, req->tc.sdata, req->tc.size);
 
 	if (out)
-		sgs[out_sgs++] = chan->sg;
+		sgs[out_sgs++] = vq_sg_page(chan->vq_sg, 0);
 
 	if (out_pages) {
-		sgs[out_sgs++] = chan->sg + out;
-		out += pack_sg_list_p(chan->sg, out, chan->sg_n,
-				      out_pages, out_nr_pages, offs, outlen);
+		sgs[out_sgs++] = vq_sg_page(chan->vq_sg, out);
+		out += pack_sg_list_p(chan->vq_sg, out, out_pages,
+				      out_nr_pages, offs, outlen);
 	}
 
 	/*

@@ -467,15 +572,14 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 	 * Arrange in such a way that server places header in the
 	 * allocated memory and payload onto the user buffer.
 	 */
-	in = pack_sg_list(chan->sg, out,
-			  chan->sg_n, req->rc.sdata, in_hdr_len);
+	in = pack_sg_list(chan->vq_sg, out, req->rc.sdata, in_hdr_len);
 	if (in)
-		sgs[out_sgs + in_sgs++] = chan->sg + out;
+		sgs[out_sgs + in_sgs++] = vq_sg_page(chan->vq_sg, out);
 
 	if (in_pages) {
-		sgs[out_sgs + in_sgs++] = chan->sg + out + in;
-		in += pack_sg_list_p(chan->sg, out + in, chan->sg_n,
-				     in_pages, in_nr_pages, offs, inlen);
+		sgs[out_sgs + in_sgs++] = vq_sg_page(chan->vq_sg, out + in);
+		in += pack_sg_list_p(chan->vq_sg, out + in, in_pages,
+				     in_nr_pages, offs, inlen);
 	}
 
 	BUG_ON(out_sgs + in_sgs > ARRAY_SIZE(sgs));

@@ -576,14 +680,12 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 		goto fail;
 	}
 
-	chan->sg = kmalloc_array(VIRTQUEUE_DEFAULT_NUM,
-				 sizeof(struct scatterlist), GFP_KERNEL);
-	if (!chan->sg) {
+	chan->vq_sg = vq_sg_alloc(VIRTQUEUE_SG_NSGL_DEFAULT);
+	if (!chan->vq_sg) {
 		pr_err("Failed to allocate virtio 9P channel\n");
 		err = -ENOMEM;
 		goto out_free_chan_shallow;
 	}
-	chan->sg_n = VIRTQUEUE_DEFAULT_NUM;
 
 	chan->vdev = vdev;
 

@@ -596,8 +698,6 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 	chan->vq->vdev->priv = chan;
 	spin_lock_init(&chan->lock);
 
-	sg_init_table(chan->sg, chan->sg_n);
-
 	chan->inuse = false;
 	if (virtio_has_feature(vdev, VIRTIO_9P_MOUNT_TAG)) {
 		virtio_cread(vdev, struct virtio_9p_config, tag_len, &tag_len);

@@ -646,7 +746,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 out_free_vq:
 	vdev->config->del_vqs(vdev);
 out_free_chan:
-	kfree(chan->sg);
+	vq_sg_free(chan->vq_sg);
 out_free_chan_shallow:
 	kfree(chan);
 fail:

@@ -741,7 +841,7 @@ static void p9_virtio_remove(struct virtio_device *vdev)
 	kobject_uevent(&(vdev->dev.kobj), KOBJ_CHANGE);
 	kfree(chan->tag);
 	kfree(chan->vc_wq);
-	kfree(chan->sg);
+	vq_sg_free(chan->vq_sg);
 	kfree(chan);
 
 }

@@ -780,7 +880,8 @@ static struct p9_trans_module p9_virtio_trans = {
 	 * that are not at page boundary, that can result in an extra
 	 * page in zero copy.
 	 */
-	.maxsize = PAGE_SIZE * (VIRTQUEUE_DEFAULT_NUM - 3),
+	.maxsize = PAGE_SIZE *
+		((VIRTQUEUE_SG_NSGL_DEFAULT * SG_USER_PAGES_PER_LIST) - 3),
 	.def = 1,
 	.owner = THIS_MODULE,
 };

Christian Schoenebeck wrote on Tue, Jul 12, 2022 at 04:31:16PM +0200:

Quoted 28 lines

The amount of elements in a scatter/gather list is limited to approximately 128 elements. To allow going beyond that limit with subsequent patches, pave the way by turning the one-

dimensional sg list array into a two-dimensional array, i.e:
sg[128]

becomes

sgl[nsgl][SG_MAX_SINGLE_ALLOC]

As the value of 'nsgl' is exactly (still) 1 in this commit and the compile-time (compiler and architecture dependent) value of 'SG_MAX_SINGLE_ALLOC' equals approximately the previous hard coded 128 elements, this commit is therefore more of a preparatory refactoring then actual behaviour change.

A custom struct virtqueue_sg is defined instead of using shared API struct sg_table, because the latter would not allow to resize the table after allocation. sg_append_table API OTOH would not fit either, because it requires a list of pages beforehand upon allocation. And both APIs only support all-or-nothing allocation.

Signed-off-by Christian Schoenebeck <[email protected]>

---

Quoted 3 lines

The question is whether that should really become 9p specifc SG list code, or whether it should rather be squeezed into shared SG list code base. Opinions by maintainers needed.

hmm from the 9p side I'd say the type is simple enough that we can just keep it here; most people don't want to resize these lists...

How much do you care about the all-or-nothing case you described in this commit message? From the look of it, patch 6 -- at what point did you actually see this being useful?

Quoted 27 lines

 net/9p/trans_virtio.c | 193 ++++++++++++++++++++++++++++++++----------
 1 file changed, 147 insertions(+), 46 deletions(-)

net/9p/trans_virtio.c +2 -1

index 18bdfa64b934..f63cd1b08bca 100644
@@ -36,7 +36,31 @@
 
 #include <linux/virtio_9p.h>
 #include "trans_common.h"
 
-#define VIRTQUEUE_DEFAULT_NUM	128
+/**
+ * struct virtqueue_sg - (chained) scatter gather lists for virtqueue

data
+ * transmission
+ * @nsgl: amount of elements (in first dimension) of array field @sgl
+ * @sgl: two-dimensional array, i.e. sgl[nsgl][SG_MAX_SINGLE_ALLOC]
+ */

net/9p/trans_virtio.c +7 -0

+struct virtqueue_sg {
+	unsigned int nsgl;
+	struct scatterlist *sgl[];
+};
+
+/*
+ * Default value for field nsgl in struct virtqueue_sg, which defines the

net/9p/trans_virtio.c

Quoted 8 lines

+ * initial virtio data transmission capacity when this virtio transport is
+ * probed.
+ */

net/9p/trans_virtio.c +4 -0

+#define VIRTQUEUE_SG_NSGL_DEFAULT 1
+
+/* maximum value for field nsgl in struct virtqueue_sg */
+#define VIRTQUEUE_SG_NSGL_MAX					

net/9p/trans_virtio.c

Quoted 1 line

net/9p/trans_virtio.c +1 -0

+	((PAGE_SIZE - sizeof(struct virtqueue_sg)) /			

net/9p/trans_virtio.c

Quoted 1 line

net/9p/trans_virtio.c +1 -0

+	sizeof(struct scatterlist *))					

Quoted 45 lines

net/9p/trans_virtio.c +4 -2

+
+/* last entry per sg list is used for chaining (pointer to next list) */
+#define SG_USER_PAGES_PER_LIST	(SG_MAX_SINGLE_ALLOC - 1)
 
 /* a single mutex to manage channel initialization and attachment */
 static DEFINE_MUTEX(virtio_9p_lock);

@@ -53,8 +77,7 @@ static atomic_t vp_pinned = ATOMIC_INIT(0);
 
  * @ring_bufs_avail: flag to indicate there is some available in the ring
  buf * @vc_wq: wait queue for waiting for thing to be added to ring buf
  * @p9_max_pages: maximum number of pinned pages
 
- * @sg: scatter gather list which is used to pack a request (protected?)
- * @sg_n: amount of elements in sg array
+ * @vq_sg: table of scatter gather lists, which are used to pack a

request

* @chan_list: linked list of channels
*
* We keep all per-channel information in a structure.

net/9p/trans_virtio.c +1 -3

@@ -77,9 +100,7 @@ struct virtio_chan {
 
 	 * will be placing it in each channel.
 	 */
 
 	unsigned long p9_max_pages;
 
-	/* Scatterlist: can be too big for stack. */
-	struct scatterlist *sg;
-	size_t sg_n;
+	struct virtqueue_sg *vq_sg;

net/9p/trans_virtio.c +0 -0

 	/**
 
 	 * @tag: name to identify a mount null terminated
 	 */

@@ -96,6 +117,92 @@ static unsigned int rest_of_page(void *data)
 
 	return PAGE_SIZE - offset_in_page(data);
 
 }
 

net/9p/trans_virtio.c static unsigned int rest_of_page(void *data) · line 129

Quoted 10 lines

net/9p/trans_virtio.c +9 -0

+/**
+ * vq_sg_page - returns user page for given page index
+ * @vq_sg: scatter gather lists used by this transport
+ * @page: user page index across all scatter gather lists
+ */
+static struct scatterlist *vq_sg_page(struct virtqueue_sg *vq_sg, size_t
 page) +{
+	unsigned int node = page / SG_USER_PAGES_PER_LIST;
+	unsigned int leaf = page % SG_USER_PAGES_PER_LIST;
+	BUG_ON(node >= VIRTQUEUE_SG_NSGL_MAX);

probably awnt to check with vq_sg->sg_n instead?
(we already check sg_n <= MAX on alloc)

Quoted 219 lines

net/9p/trans_virtio.c +34 -0

+	return &vq_sg->sgl[node][leaf];
+}
+
+/**
+ * vq_sg_npages - returns total number of individual user pages in passed
+ * scatter gather lists
+ * @vq_sg: scatter gather lists to be counted
+ */
+static size_t vq_sg_npages(struct virtqueue_sg *vq_sg)
+{
+	return vq_sg->nsgl * SG_USER_PAGES_PER_LIST;
+}
+
+/**
+ * vq_sg_free - free all memory previously allocated for @vq_sg
+ * @vq_sg: scatter gather lists to be freed
+ */
+static void vq_sg_free(struct virtqueue_sg *vq_sg)
+{
+	unsigned int i;
+
+	if (!vq_sg)
+		return;
+
+	for (i = 0; i < vq_sg->nsgl; ++i) {
+		kfree(vq_sg->sgl[i]);
+	}
+	kfree(vq_sg);
+}
+
+/**
+ * vq_sg_alloc - allocates and returns @nsgl scatter gather lists
+ * @nsgl: amount of scatter gather lists to be allocated
+ * If @nsgl is larger than one then chained lists are used if supported

by
+ * architecture.
+ */

net/9p/trans_virtio.c +44 -4

+static struct virtqueue_sg *vq_sg_alloc(unsigned int nsgl)
+{
+	struct virtqueue_sg *vq_sg;
+	unsigned int i;
+
+	BUG_ON(!nsgl || nsgl > VIRTQUEUE_SG_NSGL_MAX);
+#ifdef CONFIG_ARCH_NO_SG_CHAIN
+	if (WARN_ON_ONCE(nsgl > 1))
+		return NULL;
+#endif
+
+	vq_sg = kzalloc(sizeof(struct virtqueue_sg) +
+			nsgl * sizeof(struct scatterlist *),
+			GFP_KERNEL);
+
+	if (!vq_sg)
+		return NULL;
+
+	vq_sg->nsgl = nsgl;
+
+	for (i = 0; i < nsgl; ++i) {
+		vq_sg->sgl[i] = kmalloc_array(
+			SG_MAX_SINGLE_ALLOC, sizeof(struct scatterlist),
+			GFP_KERNEL
+		);
+		if (!vq_sg->sgl[i]) {
+			vq_sg_free(vq_sg);
+			return NULL;
+		}
+		sg_init_table(vq_sg->sgl[i], SG_MAX_SINGLE_ALLOC);
+		if (i) {
+			/* chain the lists */
+			sg_chain(vq_sg->sgl[i - 1], SG_MAX_SINGLE_ALLOC,
+				 vq_sg->sgl[i]);
+		}
+	}
+	sg_mark_end(&vq_sg->sgl[nsgl - 1][SG_MAX_SINGLE_ALLOC - 1]);
+	return vq_sg;
+}
+
 
 /**
 
  * p9_virtio_close - reclaim resources of a channel
  * @client: client instance

@@ -158,9 +265,8 @@ static void req_done(struct virtqueue *vq)
 
 /**
 
  * pack_sg_list - pack a scatter gather list from a linear buffer
 
- * @sg: scatter/gather list to pack into
+ * @vq_sg: scatter/gather lists to pack into
 
  * @start: which segment of the sg_list to start at
 
- * @limit: maximum segment to pack data to
 
  * @data: data to pack into scatter/gather list
  * @count: amount of data to pack into the scatter/gather list
  *

@@ -170,11 +276,12 @@ static void req_done(struct virtqueue *vq)
 
  *
  */
 
-static int pack_sg_list(struct scatterlist *sg, int start,
-			int limit, char *data, int count)
+static int pack_sg_list(struct virtqueue_sg *vq_sg, int start,
+			char *data, int count)
 
 {
 
 	int s;
 	int index = start;
 
+	size_t limit = vq_sg_npages(vq_sg);
 
 	while (count) {
 
 		s = rest_of_page(data);

@@ -182,13 +289,13 @@ static int pack_sg_list(struct scatterlist *sg, int

start,>

net/9p/trans_virtio.c +6 -6

 			s = count;
 
 		BUG_ON(index >= limit);
 		/* Make sure we don't terminate early. */
 
-		sg_unmark_end(&sg[index]);
-		sg_set_buf(&sg[index++], data, s);
+		sg_unmark_end(vq_sg_page(vq_sg, index));
+		sg_set_buf(vq_sg_page(vq_sg, index++), data, s);
 
 		count -= s;
 		data += s;
 
 	}
 	if (index-start)
 
-		sg_mark_end(&sg[index - 1]);
+		sg_mark_end(vq_sg_page(vq_sg, index - 1));
 
 	return index-start;
 
 }

@@ -208,21 +315,21 @@ static int p9_virtio_cancelled(struct p9_client
 *client, struct p9_req_t *req)> 
 /**
 
  * pack_sg_list_p - Just like pack_sg_list. Instead of taking a buffer,
  * this takes a list of pages.
 
- * @sg: scatter/gather list to pack into
+ * @vq_sg: scatter/gather lists to pack into
 
  * @start: which segment of the sg_list to start at
 
- * @limit: maximum number of pages in sg list.
 
  * @pdata: a list of pages to add into sg.
  * @nr_pages: number of pages to pack into the scatter/gather list
  * @offs: amount of data in the beginning of first page _not_ to pack
  * @count: amount of data to pack into the scatter/gather list
  */
 
 static int
 
-pack_sg_list_p(struct scatterlist *sg, int start, int limit,
+pack_sg_list_p(struct virtqueue_sg *vq_sg, int start,
 
 	       struct page **pdata, int nr_pages, size_t offs, int count)
 
 {
 
 	int i = 0, s;
 	int data_off = offs;
 	int index = start;
 
+	size_t limit = vq_sg_npages(vq_sg);
 
 	BUG_ON(nr_pages > (limit - start));
 	/*

@@ -235,15 +342,16 @@ pack_sg_list_p(struct scatterlist *sg, int start,

int limit,>

net/9p/trans_virtio.c +4 -3

 			s = count;
 
 		BUG_ON(index >= limit);
 		/* Make sure we don't terminate early. */
 
-		sg_unmark_end(&sg[index]);
-		sg_set_page(&sg[index++], pdata[i++], s, data_off);
+		sg_unmark_end(vq_sg_page(vq_sg, index));
+		sg_set_page(vq_sg_page(vq_sg, index++), pdata[i++], s,
+			    data_off);
 
 		data_off = 0;
 		count -= s;
 		nr_pages--;
 
 	}
 
 	if (index-start)
 
-		sg_mark_end(&sg[index - 1]);
+		sg_mark_end(vq_sg_page(vq_sg, index - 1));
 
 	return index - start;
 
 }

@@ -271,15 +379,13 @@ p9_virtio_request(struct p9_client *client, struct

p9_req_t *req)>

net/9p/trans_virtio.c +0 -2

 	out_sgs = in_sgs = 0;
 	/* Handle out VirtIO ring buffers */
 
-	out = pack_sg_list(chan->sg, 0,
-			   chan->sg_n, req->tc.sdata, req->tc.size);

net/9p/trans_virtio.c p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,

Quoted 10 lines

net/9p/trans_virtio.c +3 -3

+	out = pack_sg_list(chan->vq_sg, 0, req->tc.sdata, req->tc.size);
 
 	if (out)
 
-		sgs[out_sgs++] = chan->sg;
+		sgs[out_sgs++] = vq_sg_page(chan->vq_sg, 0);
 
-	in = pack_sg_list(chan->sg, out,
-			  chan->sg_n, req->rc.sdata, req->rc.capacity);
+	in = pack_sg_list(chan->vq_sg, out, req->rc.sdata, req-

rc.capacity);

Quoted 22 lines

net/9p/trans_virtio.c +1 -1

 	if (in)
 
-		sgs[out_sgs + in_sgs++] = chan->sg + out;
+		sgs[out_sgs + in_sgs++] = vq_sg_page(chan->vq_sg, out);
 
 	err = virtqueue_add_sgs(chan->vq, sgs, out_sgs, in_sgs, req,
 
 				GFP_ATOMIC);

@@ -448,16 +554,15 @@ p9_virtio_zc_request(struct p9_client *client,

struct p9_req_t *req,>

net/9p/trans_virtio.c +1 -2

 	out_sgs = in_sgs = 0;
 
 	/* out data */
 
-	out = pack_sg_list(chan->sg, 0,
-			   chan->sg_n, req->tc.sdata, req->tc.size);
+	out = pack_sg_list(chan->vq_sg, 0, req->tc.sdata, req->tc.size);
 
 	if (out)

net/9p/trans_virtio.c p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,

Quoted 8 lines

net/9p/trans_virtio.c +1 -4

-		sgs[out_sgs++] = chan->sg;
+		sgs[out_sgs++] = vq_sg_page(chan->vq_sg, 0);
 
 	if (out_pages) {
 
-		sgs[out_sgs++] = chan->sg + out;
-		out += pack_sg_list_p(chan->sg, out, chan->sg_n,
-				      out_pages, out_nr_pages, offs, 

Quoted 20 lines

net/9p/trans_virtio.c +3 -0

+		sgs[out_sgs++] = vq_sg_page(chan->vq_sg, out);
+		out += pack_sg_list_p(chan->vq_sg, out, out_pages,
+				      out_nr_pages, offs, outlen);
 
 	}
 
 	/*

@@ -467,15 +572,14 @@ p9_virtio_zc_request(struct p9_client *client,

struct p9_req_t *req,>

net/9p/trans_virtio.c +1 -2

 	 * Arrange in such a way that server places header in the
 	 * allocated memory and payload onto the user buffer.
 	 */
 
-	in = pack_sg_list(chan->sg, out,
-			  chan->sg_n, req->rc.sdata, in_hdr_len);
+	in = pack_sg_list(chan->vq_sg, out, req->rc.sdata, in_hdr_len);
 
 	if (in)

net/9p/trans_virtio.c p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,

Quoted 8 lines

net/9p/trans_virtio.c +1 -4

-		sgs[out_sgs + in_sgs++] = chan->sg + out;
+		sgs[out_sgs + in_sgs++] = vq_sg_page(chan->vq_sg, out);
 
 	if (in_pages) {
 
-		sgs[out_sgs + in_sgs++] = chan->sg + out + in;
-		in += pack_sg_list_p(chan->sg, out + in, chan->sg_n,
-				     in_pages, in_nr_pages, offs, 

net/9p/trans_virtio.c

Quoted 1 line

net/9p/trans_virtio.c +1 -0

+		sgs[out_sgs + in_sgs++] = vq_sg_page(chan->vq_sg, out + 

Quoted 7 lines

net/9p/trans_virtio.c +2 -0

+		in += pack_sg_list_p(chan->vq_sg, out + in, in_pages,
+				     in_nr_pages, offs, inlen);
 
 	}
 
 	BUG_ON(out_sgs + in_sgs > ARRAY_SIZE(sgs));

net/9p/trans_virtio.c static int p9_virtio_probe(struct virtio_device · line 582

Quoted 8 lines

net/9p/trans_virtio.c +0 -2

@@ -576,14 +680,12 @@ static int p9_virtio_probe(struct virtio_device
 *vdev)> 
 		goto fail;
 
 	}
 
-	chan->sg = kmalloc_array(VIRTQUEUE_DEFAULT_NUM,
-				 sizeof(struct scatterlist), 

Quoted 19 lines

net/9p/trans_virtio.c +2 -2

-	if (!chan->sg) {
+	chan->vq_sg = vq_sg_alloc(VIRTQUEUE_SG_NSGL_DEFAULT);
+	if (!chan->vq_sg) {
 
 		pr_err("Failed to allocate virtio 9P channel\n");
 		err = -ENOMEM;
 		goto out_free_chan_shallow;
 
 	}
 
-	chan->sg_n = VIRTQUEUE_DEFAULT_NUM;
 
 	chan->vdev = vdev;

@@ -596,8 +698,6 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 
 	chan->vq->vdev->priv = chan;
 	spin_lock_init(&chan->lock);
 

net/9p/trans_virtio.c

Quoted 7 lines

net/9p/trans_virtio.c +0 -2

-	sg_init_table(chan->sg, chan->sg_n);
-
 
 	chan->inuse = false;

net/9p/trans_virtio.c +0 -0

 	if (virtio_has_feature(vdev, VIRTIO_9P_MOUNT_TAG)) {
 
 		virtio_cread(vdev, struct virtio_9p_config, tag_len, 

Quoted 29 lines

net/9p/trans_virtio.c +1 -1

@@ -646,7 +746,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 
 out_free_vq:
 	vdev->config->del_vqs(vdev);
 
 out_free_chan:
-	kfree(chan->sg);
+	vq_sg_free(chan->vq_sg);
 

out_free_chan_shallow:

net/9p/trans_virtio.c +1 -1

 	kfree(chan);
 
 fail:

@@ -741,7 +841,7 @@ static void p9_virtio_remove(struct virtio_device
 *vdev)> 
 	kobject_uevent(&(vdev->dev.kobj), KOBJ_CHANGE);
 	kfree(chan->tag);
 	kfree(chan->vc_wq);
 
-	kfree(chan->sg);
+	vq_sg_free(chan->vq_sg);
 

net/9p/trans_virtio.c +0 -0

 	kfree(chan);
 
 }

@@ -780,7 +880,8 @@ static struct p9_trans_module p9_virtio_trans = {
 

net/9p/trans_virtio.c static struct p9_trans_module p9_virtio_trans = {

Quoted 7 lines

net/9p/trans_virtio.c +2 -1

 	 * that are not at page boundary, that can result in an extra
 	 * page in zero copy.
 	 */
 
-	.maxsize = PAGE_SIZE * (VIRTQUEUE_DEFAULT_NUM - 3),
+	.maxsize = PAGE_SIZE *
+		((VIRTQUEUE_SG_NSGL_DEFAULT * SG_USER_PAGES_PER_LIST) - 

Quoted 5 lines

net/9p/trans_virtio.c +0 -0

 	.def = 1,

.owner = THIS_MODULE,

};

This 9p client implementation is yet using linear message buffers for most message types, i.e. they use kmalloc() et al. for allocating continuous physical memory pages, which is usually limited to 4MB buffers. Use KMALLOC_MAX_SIZE though instead of a hard coded 4MB for constraining this more safely.

Unfortunately we cannot simply replace the existing kmalloc() calls by vmalloc() ones, because that would yield in non-logical kernel addresses (for any vmalloc(>4MB) that is) which are in general not accessible by hosts like QEMU.

In future we would replace those linear buffers by scatter/gather lists to eventually get rid of this limit (struct p9_fcall's sdata member by p9_fcall_init() and struct p9_fid's rdir member by v9fs_alloc_rdir_buf()).

Signed-off-by Christian Schoenebeck <[email protected]>

---

Hmm, that's a bit too simple, as we also need a bit of headroom for transport specific overhead. So maybe this has to be handled by each transport appropriately instead?

 net/9p/client.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

net/9p/client.c +11 -0

index 20054addd81b..fab939541c81 100644
@@ -1042,6 +1042,17 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
 	p9_debug(P9_DEBUG_MUX, "clnt %p trans %p msize %d protocol %d\n",
 		 clnt, clnt->trans_mod, clnt->msize, clnt->proto_version);
 
+	/*
+	 * due to linear message buffers being used by client ATM
+	 */
+	if (clnt->msize > KMALLOC_MAX_SIZE) {
+		clnt->msize = KMALLOC_MAX_SIZE;
+		pr_info("Limiting 'msize' to %zu as this is the maximum "
+			"supported by this client version.\n",
+			(size_t) KMALLOC_MAX_SIZE
+		);
+	}
+
 	err = clnt->trans_mod->create(clnt, dev_name, options);
 	if (err)
 		goto put_trans;

This new function calculates a buffer size suitable for holding the intended 9p request or response. For rather small message types (which applies to almost all 9p message types actually) simply use hard coded values. For some variable-length and potentially large message types calculate a more precise value according to what data is actually transmitted to avoid unnecessarily huge buffers.

Signed-off-by Christian Schoenebeck <[email protected]>

---
 net/9p/protocol.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++
 net/9p/protocol.h |   2 +
 2 files changed, 156 insertions(+)

net/9p/protocol.c +45 -0

index 3754c33e2974..49939e8cde2a 100644
@@ -23,6 +23,160 @@
 
 #include <trace/events/9p.h>
 
+/* len[2] text[len] */
+#define P9_STRLEN(s) \
+	(2 + min_t(size_t, s ? strlen(s) : 0, USHRT_MAX))
+
+/**
+ * p9_msg_buf_size - Returns a buffer size sufficiently large to hold the
+ * intended 9p message.
+ * @c: client
+ * @type: message type
+ * @fmt: format template for assembling request message
+ * (see p9pdu_vwritef)
+ * @ap: variable arguments to be fed to passed format template
+ * (see p9pdu_vwritef)
+ *
+ * Note: Even for response types (P9_R*) the format template and variable
+ * arguments must always be for the originating request type (P9_T*).
+ */
+size_t p9_msg_buf_size(struct p9_client *c, enum p9_msg_t type,
+			const char *fmt, va_list ap)
+{
+	/* size[4] type[1] tag[2] */
+	const int hdr = 4 + 1 + 2;
+	/* ename[s] errno[4] */
+	const int rerror_size = hdr + P9_ERRMAX + 4;
+	/* ecode[4] */
+	const int rlerror_size = hdr + 4;
+	const int err_size =
+		c->proto_version == p9_proto_2000L ? rlerror_size : rerror_size;
+
+	switch (type) {
+
+	/* message types not used at all */
+	case P9_TERROR:
+	case P9_TLERROR:
+	case P9_TAUTH:
+	case P9_RAUTH:
+		BUG();
+
+	/* variable length & potentially large message types */
+	case P9_TATTACH:
+		BUG_ON(strcmp("ddss?u", fmt));
+		va_arg(ap, int32_t);
+		va_arg(ap, int32_t);
+		{
+			const char *uname = va_arg(ap, const char *);

net/9p/protocol.c +14 -0

+			size_t wname_all;
+			const char **wnames = va_arg(ap, const char **);
+			for (i = 0, wname_all = 0; i < nwname; ++i) {
+				wname_all += P9_STRLEN(wnames[i]);
+			}
+			/* fid[4] newfid[4] nwname[2] nwname*(wname[s]) */
+			return hdr + 4 + 4 + 2 + wname_all;
+		}
+	case P9_RWALK:
+		BUG_ON(strcmp("ddT", fmt));
+		va_arg(ap, int32_t);
+		va_arg(ap, int32_t);
+		{
+			uint nwname = va_arg(ap, int);

net/9p/protocol.c +30 -0

+				/* fid[4] name[s] perm[4] mode[1] */
+				return hdr + 4 + P9_STRLEN(name) + 4 + 1;
+			{
+				va_arg(ap, int32_t);
+				va_arg(ap, int);
+				{
+					const char *ext = va_arg(ap, const char *);
+					/* fid[4] name[s] perm[4] mode[1] extension[s] */
+					return hdr + 4 + P9_STRLEN(name) + 4 + 1 + P9_STRLEN(ext);
+				}
+			}
+		}
+	case P9_TLCREATE:
+		BUG_ON(strcmp("dsddg", fmt));
+		va_arg(ap, int32_t);
+		{
+			const char *name = va_arg(ap, const char *);
+			/* fid[4] name[s] flags[4] mode[4] gid[4] */
+			return hdr + 4 + P9_STRLEN(name) + 4 + 4 + 4;
+		}
+	case P9_RREAD:
+	case P9_RREADDIR:
+		BUG_ON(strcmp("dqd", fmt));
+		va_arg(ap, int32_t);
+		va_arg(ap, int64_t);
+		{
+			const int32_t count = va_arg(ap, int32_t);
+			/* count[4] data[count] */
+			return max_t(size_t, hdr + 4 + count, err_size);
+		}

net/9p/protocol.c +5 -0

+		return 4 * 1024;
+
+	}
+}
+
 static int
 p9pdu_writef(struct p9_fcall *pdu, int proto_version, const char *fmt, ...);
 

net/9p/protocol.h +0 -0

index 6d719c30331a..ad2283d1f96b 100644

Christian Schoenebeck wrote on Tue, Jul 12, 2022 at 04:31:33PM +0200:

Quoted 8 lines

This new function calculates a buffer size suitable for holding the intended 9p request or response. For rather small message types (which applies to almost all 9p message types actually) simply use hard coded values. For some variable-length and potentially large message types calculate a more precise value according to what data is actually transmitted to avoid unnecessarily huge buffers.

Signed-off-by Christian Schoenebeck <[email protected]>

Overally already had checked a few times but I just went through client.c va argument lists as well this time, just a few nitpicks.

Quoted 66 lines

---

 net/9p/protocol.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++
 net/9p/protocol.h |   2 +
 2 files changed, 156 insertions(+)

net/9p/protocol.c +52 -0

index 3754c33e2974..49939e8cde2a 100644
@@ -23,6 +23,160 @@
 
 #include <trace/events/9p.h>
 
+/* len[2] text[len] */
+#define P9_STRLEN(s) \
+	(2 + min_t(size_t, s ? strlen(s) : 0, USHRT_MAX))
+
+/**
+ * p9_msg_buf_size - Returns a buffer size sufficiently large to hold the
+ * intended 9p message.
+ * @c: client
+ * @type: message type
+ * @fmt: format template for assembling request message
+ * (see p9pdu_vwritef)
+ * @ap: variable arguments to be fed to passed format template
+ * (see p9pdu_vwritef)
+ *
+ * Note: Even for response types (P9_R*) the format template and variable
+ * arguments must always be for the originating request type (P9_T*).
+ */
+size_t p9_msg_buf_size(struct p9_client *c, enum p9_msg_t type,
+			const char *fmt, va_list ap)
+{
+	/* size[4] type[1] tag[2] */
+	const int hdr = 4 + 1 + 2;
+	/* ename[s] errno[4] */
+	const int rerror_size = hdr + P9_ERRMAX + 4;
+	/* ecode[4] */
+	const int rlerror_size = hdr + 4;
+	const int err_size =
+		c->proto_version == p9_proto_2000L ? rlerror_size : rerror_size;
+
+	switch (type) {
+
+	/* message types not used at all */
+	case P9_TERROR:
+	case P9_TLERROR:
+	case P9_TAUTH:
+	case P9_RAUTH:
+		BUG();
+
+	/* variable length & potentially large message types */
+	case P9_TATTACH:
+		BUG_ON(strcmp("ddss?u", fmt));
+		va_arg(ap, int32_t);
+		va_arg(ap, int32_t);
+		{
+			const char *uname = va_arg(ap, const char *);
+			const char *aname = va_arg(ap, const char *);
+			/* fid[4] afid[4] uname[s] aname[s] n_uname[4] */
+			return hdr + 4 + 4 + P9_STRLEN(uname) + P9_STRLEN(aname) + 4;
+		}
+	case P9_TWALK:
+		BUG_ON(strcmp("ddT", fmt));
+		va_arg(ap, int32_t);

net/9p/protocol.c line 80

Quoted 3 lines

net/9p/protocol.c +3 -0

+		va_arg(ap, int32_t);
+		{
+			uint i, nwname = max(va_arg(ap, int), 0);

I was about to say that the max is useless as for loop would be cut short, but these are unsigned... So the code in protocol.c p9pdu_vwritef 'T' has a bug (int cast directly to uint16): do you want to fix it or shall I go ahead?

Quoted 19 lines

net/9p/protocol.c +19 -0

+			size_t wname_all;
+			const char **wnames = va_arg(ap, const char **);
+			for (i = 0, wname_all = 0; i < nwname; ++i) {
+				wname_all += P9_STRLEN(wnames[i]);
+			}
+			/* fid[4] newfid[4] nwname[2] nwname*(wname[s]) */
+			return hdr + 4 + 4 + 2 + wname_all;
+		}
+	case P9_RWALK:
+		BUG_ON(strcmp("ddT", fmt));
+		va_arg(ap, int32_t);
+		va_arg(ap, int32_t);
+		{
+			uint nwname = va_arg(ap, int);
+			/* nwqid[2] nwqid*(wqid[13]) */
+			return max_t(size_t, hdr + 2 + nwname * 13, err_size);
+		}
+	case P9_TCREATE:
+		BUG_ON(strcmp("dsdb?s", fmt));

net/9p/protocol.c line 104

Quoted 5 lines

net/9p/protocol.c +5 -0

+		va_arg(ap, int32_t);
+		{
+			const char *name = va_arg(ap, const char *);
+			if ((c->proto_version != p9_proto_2000u) &&
+			    (c->proto_version != p9_proto_2000L))

(I don't think 9p2000.L can call TCREATE, but it doesn't really hurt either)

Quoted 39 lines

net/9p/protocol.c +39 -0

+				/* fid[4] name[s] perm[4] mode[1] */
+				return hdr + 4 + P9_STRLEN(name) + 4 + 1;
+			{
+				va_arg(ap, int32_t);
+				va_arg(ap, int);
+				{
+					const char *ext = va_arg(ap, const char *);
+					/* fid[4] name[s] perm[4] mode[1] extension[s] */
+					return hdr + 4 + P9_STRLEN(name) + 4 + 1 + P9_STRLEN(ext);
+				}
+			}
+		}
+	case P9_TLCREATE:
+		BUG_ON(strcmp("dsddg", fmt));
+		va_arg(ap, int32_t);
+		{
+			const char *name = va_arg(ap, const char *);
+			/* fid[4] name[s] flags[4] mode[4] gid[4] */
+			return hdr + 4 + P9_STRLEN(name) + 4 + 4 + 4;
+		}
+	case P9_RREAD:
+	case P9_RREADDIR:
+		BUG_ON(strcmp("dqd", fmt));
+		va_arg(ap, int32_t);
+		va_arg(ap, int64_t);
+		{
+			const int32_t count = va_arg(ap, int32_t);
+			/* count[4] data[count] */
+			return max_t(size_t, hdr + 4 + count, err_size);
+		}
+	case P9_TWRITE:
+		BUG_ON(strcmp("dqV", fmt));
+		va_arg(ap, int32_t);
+		va_arg(ap, int64_t);
+		{
+			const int32_t count = va_arg(ap, int32_t);
+			/* fid[4] offset[8] count[4] data[count] */
+			return hdr + 4 + 8 + 4 + count;
+		}

net/9p/protocol.c line 144

Quoted 1 line

net/9p/protocol.c +1 -0

+	case P9_TRENAMEAT:

if we have trenameat we probably want trename, tunlinkat as well? What's your criteria for counting individually vs slapping 8k at it?

In this particular case, oldname/newname are single component names within a directory so this is capped at 2*(4+256), that could easily fit in 4k without bothering.

net/9p/protocol.c line 151

Quoted 7 lines

net/9p/protocol.c +7 -0

+		BUG_ON(strcmp("dsds", fmt));
+		va_arg(ap, int32_t);
+		{
+			const char *oldname = va_arg(ap, const char *);
+			va_arg(ap, int32_t);
+			{
+				const char *newname = va_arg(ap, const char *);

(style nitpick) I don't see the point of nesting another level of indentation here, it feels cleaner to declare oldname/newname at the start of the block and be done with it.

Quoted 4 lines

net/9p/protocol.c +4 -0

+				/* olddirfid[4] oldname[s] newdirfid[4] newname[s] */
+				return hdr + 4 + P9_STRLEN(oldname) + 4 + P9_STRLEN(newname);
+			}
+		}

net/9p/protocol.c line 161

Quoted 6 lines

net/9p/protocol.c +6 -0

+	case P9_RERROR:
+		return rerror_size;
+	case P9_RLERROR:
+		return rlerror_size;
+
+	/* small message types */

ditto: what's your criteria for 4k vs 8k?

net/9p/protocol.c line 162

Quoted 1 line

net/9p/protocol.c +1 -0

+	case P9_TSTAT:

this is just fid[4], so 4k is more than enough

net/9p/protocol.c line 163

Quoted 1 line

net/9p/protocol.c +1 -0

+	case P9_RSTAT:

also fixed size 4+4+8+8+8+8+8+8+4 -- fits in 4k.

net/9p/protocol.c line 164

Quoted 1 line

net/9p/protocol.c +1 -0

+	case P9_TSYMLINK:

that one has symlink target which can be arbitrarily long (filesystem specific, 4k is the usual limit for linux but some filesystem I don't know might handle more -- it might be worth going through the trouble of going through it.

Ah, we can't support an arbitrary length as we won't know the size for rreadlink before the reply comes, so we have to set some arbitrary max. Okay for 8k.

net/9p/protocol.c line 165

Quoted 1 line

net/9p/protocol.c +1 -0

+	case P9_RREADLINK:

Ok as above.

net/9p/protocol.c line 166

Quoted 1 line

net/9p/protocol.c +1 -0

+	case P9_TXATTRWALK:

xattr names seem capped at 256, could fit 4k but ok for 8.

net/9p/protocol.c line 167

Quoted 1 line

net/9p/protocol.c +1 -0

+	case P9_TXATTRCREATE:

same, ok for either 4 or 8.

net/9p/protocol.c line 168

Quoted 1 line

net/9p/protocol.c +1 -0

+	case P9_TLINK:

name is component name inside directory so capped at 256, but ok.

net/9p/protocol.c line 169

Quoted 1 line

net/9p/protocol.c +1 -0

+	case P9_TMKDIR:

same

net/9p/protocol.c line 170

Quoted 1 line

net/9p/protocol.c +1 -0

+	case P9_TUNLINKAT:

same

Quoted 3 lines

net/9p/protocol.c +3 -0

+		return 8 * 1024;
+
+	/* tiny message types */

net/9p/protocol.c line 174

Quoted 1 line

net/9p/protocol.c +1 -0

+	default:

I went through things we didn't list:
mknod has a name but it's also component within directory, so should be consistent with mkdir/unlinkat
trename, same.

tlock contains client_id which comes from hostname.. I think that's capped at 256 as well? so ok for 4k.

rest all looks ok to me.

Quoted 11 lines

net/9p/protocol.c line 80

Quoted 6 lines

net/9p/protocol.c +6 -0

+	case P9_TWALK:
+		BUG_ON(strcmp("ddT", fmt));
+		va_arg(ap, int32_t);
+		va_arg(ap, int32_t);
+		{
+			uint i, nwname = max(va_arg(ap, int), 0);

I was about to say that the max is useless as for loop would be cut short, but these are unsigned... So the code in protocol.c p9pdu_vwritef 'T' has a bug (int cast directly to uint16): do you want to fix it or shall I go ahead?

I'd either send a separate patch today for fixing 'T', or if you want to handle it by yourself, then just go ahead.

Quoted 10 lines

net/9p/protocol.c line 104

Quoted 7 lines

net/9p/protocol.c +7 -0

+	case P9_TCREATE:
+		BUG_ON(strcmp("dsdb?s", fmt));
+		va_arg(ap, int32_t);
+		{
+			const char *name = va_arg(ap, const char *);
+			if ((c->proto_version != p9_proto_2000u) &&
+			    (c->proto_version != p9_proto_2000L))

(I don't think 9p2000.L can call TCREATE, but it doesn't really hurt either)

Yes, Tcreate is only 9p2000 and 9p2000.u. Semantically this particular check here means "if proto == 9p.2000". I can't remember anymore why I came up with this inverted form here. I'll change it to "if (c->proto_version == p9_proto_legacy)".

Quoted 7 lines

net/9p/protocol.c line 144

Quoted 1 line

net/9p/protocol.c +1 -0

+	case P9_TRENAMEAT:

if we have trenameat we probably want trename, tunlinkat as well? What's your criteria for counting individually vs slapping 8k at it?

In this particular case, oldname/newname are single component names within a directory so this is capped at 2*(4+256), that could easily fit in 4k without bothering.

I have not taken the Linux kernel's current filename limit NAME_MAX
(255) as basis, in that case you would be right. Instead I looked up what the maximum filename length among file systems in general was, and saw that ReiserFS supports up to slightly below 4k? So I took 4k as basis for the calculation used here, and the intention was to make this code more future proof. Because revisiting this code later on always takes quite some time and always has this certain potential to miss out details.

Independent of the decision; additionally it might make sense to add something like:

#if NAME_MAX > 255

# error p9_msg_buf_size() needs adjustments
#endif

Quoted 11 lines

net/9p/protocol.c line 151

Quoted 7 lines

net/9p/protocol.c +7 -0

+		BUG_ON(strcmp("dsds", fmt));
+		va_arg(ap, int32_t);
+		{
+			const char *oldname = va_arg(ap, const char *);
+			va_arg(ap, int32_t);
+			{
+				const char *newname = va_arg(ap, const char *);

(style nitpick) I don't see the point of nesting another level of indentation here, it feels cleaner to declare oldname/newname at the start of the block and be done with it.

Because va_arg(ap, int32_t); must remain between those two declarations, and I think either the compiler or style check script was barking at me. But I will recheck, if possible I will remove the additional block scope here.

Quoted 3 lines

net/9p/protocol.c line 161

Quoted 1 line

net/9p/protocol.c +1 -0

+	/* small message types */

ditto: what's your criteria for 4k vs 8k?

As above, 4k being the basis for directory entry names, plus PATH_MAX (4k) as basis for maximum path length.

However looking at it again, if NAME_MAX == 4k was assumed exactly, then Tsymlink would have the potential to exceed 8k, as it has name[s] and symtgt[s] plus the other fields.

Quoted 2 lines

net/9p/protocol.c line 162

Quoted 1 line

net/9p/protocol.c +1 -0

+	case P9_TSTAT:

this is just fid[4], so 4k is more than enough

I guess that was a typo and should have been Twstat instead?

Quoted 2 lines

net/9p/protocol.c line 163

Quoted 1 line

net/9p/protocol.c +1 -0

+	case P9_RSTAT:

also fixed size 4+4+8+8+8+8+8+8+4 -- fits in 4k.

Rstat contains stat[n] which in turn contains variable-length string fields (filename, owner name, group name)

Quoted 6 lines

net/9p/protocol.c line 164

Quoted 2 lines

net/9p/protocol.c +1 -0

+	case P9_TSYMLINK:

that one has symlink target which can be arbitrarily long (filesystem specific, 4k is the usual limit for linux but some filesystem I don't know might handle more -- it might be worth going through the trouble of going through it.

Like mentioned above, if exactly NAME_MAX == 4k was assumed, then Tsymlink may even be >8k.

Quoted 1 line

rest all looks ok to me.

Thanks for the review! I know, that's really a dry patch to look at. :)

Christian Schoenebeck wrote on Wed, Jul 13, 2022 at 03:06:01PM +0200:

Quoted 12 lines

Quoted 11 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+	case P9_TWALK:
+		BUG_ON(strcmp("ddT", fmt));
+		va_arg(ap, int32_t);
+		va_arg(ap, int32_t);
+		{
+			uint i, nwname = max(va_arg(ap, int), 0);

I was about to say that the max is useless as for loop would be cut short, but these are unsigned... So the code in protocol.c p9pdu_vwritef 'T' has a bug (int cast directly to uint16): do you want to fix it or shall I go ahead?

Quoted 2 lines

I'd either send a separate patch today for fixing 'T', or if you want to handle it by yourself, then just go ahead.

I'd appreciate if you have time, doesn't make much difference though

Quoted 11 lines

Quoted 10 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+	case P9_TCREATE:
+		BUG_ON(strcmp("dsdb?s", fmt));
+		va_arg(ap, int32_t);
+		{
+			const char *name = va_arg(ap, const char *);
+			if ((c->proto_version != p9_proto_2000u) &&
+			    (c->proto_version != p9_proto_2000L))

(I don't think 9p2000.L can call TCREATE, but it doesn't really hurt either)

Quoted 4 lines

Yes, Tcreate is only 9p2000 and 9p2000.u. Semantically this particular check here means "if proto == 9p.2000". I can't remember anymore why I came up with this inverted form here. I'll change it to "if (c->proto_version == p9_proto_legacy)".

Sounds good.

Quoted 8 lines

Quoted 7 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	case P9_TRENAMEAT:

if we have trenameat we probably want trename, tunlinkat as well? What's your criteria for counting individually vs slapping 8k at it?

In this particular case, oldname/newname are single component names within a directory so this is capped at 2*(4+256), that could easily fit in 4k without bothering.

Quoted 8 lines

I have not taken the Linux kernel's current filename limit NAME_MAX
(255) as basis, in that case you would be right. Instead I looked up what the maximum filename length among file systems in general was, and saw that ReiserFS supports up to slightly below 4k? So I took 4k as basis for the calculation used here, and the intention was to make this code more future proof. Because revisiting this code later on always takes quite some time and always has this certain potential to miss out details.

hmm, that's pretty deeply engrained into the VFS but I guess it might change eventually, yes.

I don't mind as long as we're consistent (cf. unlink/mkdir below), in practice measuring doesn't cost much.

Quoted 6 lines

Independent of the decision; additionally it might make sense to add something like:

#if NAME_MAX > 255

# error p9_msg_buf_size() needs adjustments
#endif

That's probably an understatement but I don't mind either way, it doesn't hurt.

Quoted 12 lines

Quoted 11 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+		BUG_ON(strcmp("dsds", fmt));
+		va_arg(ap, int32_t);
+		{
+			const char *oldname = va_arg(ap, const char *);
+			va_arg(ap, int32_t);
+			{
+				const char *newname = va_arg(ap, const char *);

(style nitpick) I don't see the point of nesting another level of indentation here, it feels cleaner to declare oldname/newname at the start of the block and be done with it.

Quoted 4 lines

Because va_arg(ap, int32_t); must remain between those two declarations, and I think either the compiler or style check script was barking at me. But I will recheck, if possible I will remove the additional block scope here.

Yes, I think it'd need to look like this:

	case foo:
		BUG_ON(...)
		va_arg(ap, int32_t);
		{
			const char *oldname = va_arg(ap, const char *);
			const char *newname;
			va_arg(ap, int32_t);
			newname = va_arg(ap, const_char *);
			...
		}
or
		{
			const char *oldname, *newname;
			oldname = va_arg(ap, const char *);
			va_arg(ap, int32_t)
			newname = va_arg(ap, const char *);
			...
		}

I guess the later is slightly easier on the eyes

Quoted 10 lines

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	/* small message types */

ditto: what's your criteria for 4k vs 8k?

As above, 4k being the basis for directory entry names, plus PATH_MAX (4k) as basis for maximum path length.

However looking at it again, if NAME_MAX == 4k was assumed exactly, then Tsymlink would have the potential to exceed 8k, as it has name[s] and symtgt[s] plus the other fields.

yes.

Quoted 4 lines

Quoted 2 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	case P9_TSTAT:

this is just fid[4], so 4k is more than enough

I guess that was a typo and should have been Twstat instead?

Ah, had missed this because 9p2000.L's version of stat[n] is fixed size. Sounds good.

Quoted 5 lines

Quoted 2 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	case P9_RSTAT:

also fixed size 4+4+8+8+8+8+8+8+4 -- fits in 4k.

Rstat contains stat[n] which in turn contains variable-length string fields (filename, owner name, group name)

Right, same mistake.

Quoted 8 lines

Quoted 5 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	case P9_TSYMLINK:

that one has symlink target which can be arbitrarily long (filesystem specific, 4k is the usual limit for linux but some filesystem I don't know might handle more -- it might be worth going through the trouble of going through it.

Like mentioned above, if exactly NAME_MAX == 4k was assumed, then Tsymlink may even be >8k.

And all the other remarks are 'yes if we assume bigger NAME_MAX' -- I'm happy either way.

Quoted 4 lines

Quoted 1 line

rest all looks ok to me.

Thanks for the review! I know, that's really a dry patch to look at. :)

Thanks for writing it in the first place ;)

--
Dominique

So far 'msize' was simply used for all 9p message types, which is far too much and slowed down performance tremendously with large values for user configurable 'msize' option.

Let's stop this waste by using the new p9_msg_buf_size() function for allocating more appropriate, smaller buffers according to what is actually sent over the wire.

Only exception: RDMA transport is currently excluded from this, as it would not cope with it. [1]

Link https://lore.kernel.org/all/[email protected]/ [1] open here

Signed-off-by Christian Schoenebeck <[email protected]>

---

Is the !strcmp(c->trans_mod->name, "rdma") check in this patch maybe a bit too hack-ish? Should there rather be transport API extension instead?

Christian Schoenebeck wrote on Tue, Jul 12, 2022 at 04:31:36PM +0200:

Quoted 10 lines

So far 'msize' was simply used for all 9p message types, which is far too much and slowed down performance tremendously with large values for user configurable 'msize' option.

Let's stop this waste by using the new p9_msg_buf_size() function for allocating more appropriate, smaller buffers according to what is actually sent over the wire.

Only exception: RDMA transport is currently excluded from this, as it would not cope with it. [1]

Dominique Martinet wrote on Wed, Jul 13, 2022 at 04:33:35AM +0900:

Quoted 11 lines

Christian Schoenebeck wrote on Tue, Jul 12, 2022 at 04:31:36PM +0200:

Quoted 10 lines

So far 'msize' was simply used for all 9p message types, which is far too much and slowed down performance tremendously with large values for user configurable 'msize' option.

Let's stop this waste by using the new p9_msg_buf_size() function for allocating more appropriate, smaller buffers according to what is actually sent over the wire.

Only exception: RDMA transport is currently excluded from this, as it would not cope with it. [1]

Thinking back on RDMA:
- vs. one or two buffers as discussed in another thread, rdma will still require two buffers, we post the receive buffer before sending as we could otherwise be raced (reply from server during the time it'd take to recycle the send buffer)
In practice the recv buffers should act liks a fifo and we might be able to post the buffer we're about to send for recv before sending it and it shouldn't be overwritten until it's sent, but that doesn't look quite good.

- for this particular patch, we can still allocate smaller short buffers for requests, so we should probably keep tsize to 0.
rsize there really isn't much we can do without a protocol change though...

Quoted 4 lines

- for this particular patch, we can still allocate smaller short buffers for requests, so we should probably keep tsize to 0.
rsize there really isn't much we can do without a protocol change though...

Good to know! I don't have any RDMA setup here to test, so I rely on what you say and adjust this in v6 accordingly, along with the strcmp -> flag change of course.

As this flag is going to be very RDMA-transport specific, I'm still scratching my head for a good name though.

Christian Schoenebeck wrote on Wed, Jul 13, 2022 at 11:19:48AM +0200:

Quoted 8 lines

Quoted 4 lines

- for this particular patch, we can still allocate smaller short buffers for requests, so we should probably keep tsize to 0.
rsize there really isn't much we can do without a protocol change though...

Good to know! I don't have any RDMA setup here to test, so I rely on what you say and adjust this in v6 accordingly, along with the strcmp -> flag change of course.

Yeah... I've got a connect-x 3 (mlx4, got a cheap old one) card laying around, I need to find somewhere to plug it in and actually run some validation again at some point.
Haven't used 9p/RDMA since I left my previous work in 2020...

I'll try to find time for that before the merge

Quoted 2 lines

As this flag is going to be very RDMA-transport specific, I'm still scratching my head for a good name though.

The actual limitation is that receive buffers are pooled, so something to like pooled_rcv_buffers or shared_rcv_buffers or anything along that line?

Quoted 23 lines · On Dienstag, 12. Juli 2022 23:11:42 CEST Dominique Martinet wrote:

Dominique Martinet wrote on Wed, Jul 13, 2022 at 04:33:35AM +0900:

Quoted 11 lines

Christian Schoenebeck wrote on Tue, Jul 12, 2022 at 04:31:36PM +0200:

Quoted 10 lines

So far 'msize' was simply used for all 9p message types, which is far too much and slowed down performance tremendously with large values for user configurable 'msize' option.

Let's stop this waste by using the new p9_msg_buf_size() function for allocating more appropriate, smaller buffers according to what is actually sent over the wire.

Only exception: RDMA transport is currently excluded from this, as it would not cope with it. [1]

Thinking back on RDMA:
- vs. one or two buffers as discussed in another thread, rdma will still require two buffers, we post the receive buffer before sending as we could otherwise be raced (reply from server during the time it'd take to recycle the send buffer)
In practice the recv buffers should act liks a fifo and we might be able to post the buffer we're about to send for recv before sending it and it shouldn't be overwritten until it's sent, but that doesn't look quite good.

Quoted 4 lines

- for this particular patch, we can still allocate smaller short buffers for requests, so we should probably keep tsize to 0.
rsize there really isn't much we can do without a protocol change though...

Good to know! I don't have any RDMA setup here to test, so I rely on what you say and adjust this in v6 accordingly, along with the strcmp -> flag change of course.

As this flag is going to be very RDMA-transport specific, I'm still scratching my head for a good name though.

Quoted 2 lines

As this flag is going to be very RDMA-transport specific, I'm still scratching my head for a good name though.

Or, instead of inventing some exotic flag name, maybe introducing an enum for the individual 9p transport types?

OVERVIEW OF PATCHES:

* Patches 1..6 remove the msize limitation from the 'virtio' transport (i.e. the 9p 'virtio' transport itself actually supports >4MB now, tested successfully with an experimental QEMU version and some dirty 9p Linux client hacks up to msize=128MB).

* Patch 7 limits msize for all transports to 4 MB for now as >4MB would need more work on 9p client level (see commit log of patch 7 for details).

* Patches 8..11 tremendously reduce unnecessarily huge 9p message sizes and therefore provide performance gain as well. So far, almost all 9p messages simply allocated message buffers exactly msize large, even for messages that actually just needed few bytes. So these patches make sense by themselves, independent of this overall series, however for this series even more, because the larger msize, the more this issue would have hurt otherwise.

Alright; anything I didn't reply to looks good to me.

Christian Schoenebeck wrote on Tue, Jul 12, 2022 at 04:35:54PM +0200:

Quoted 4 lines

OVERVIEW OF PATCHES:

* Patches 1..6 remove the msize limitation from the 'virtio' transport

Quoted 4 lines

(i.e. the 9p 'virtio' transport itself actually supports >4MB now, tested
successfully with an experimental QEMU version and some dirty 9p Linux client hacks up to msize=128MB).

I have no problem with this except for the small nitpicks I gave, but would be tempted to delay this part for one more cycle as it's really independant -- what do you think?

Quoted 7 lines

* Patch 7 limits msize for all transports to 4 MB for now as >4MB would need>
more work on 9p client level (see commit log of patch 7 for details).

* Patches 8..11 tremendously reduce unnecessarily huge 9p message sizes and

Quoted 7 lines

therefore provide performance gain as well. So far, almost all 9p messages
simply allocated message buffers exactly msize large, even for messages that actually just needed few bytes. So these patches make sense by themselves, independent of this overall series, however for this series even more, because the larger msize, the more this issue would have hurt otherwise.

time-wise we're getting close to the merge window already (probably in 2 weeks), how confident are you in this?
I can take patches 8..11 in -next now and probably find some time to test over next weekend, are we good?