[v4] usb: gadget: storage_common: make FSG_NUM_BUFFERS variable size

Message ID 1313659726-9406-1-git-send-email-per.forlin@stericsson.com
State New
Headers show

Commit Message

Per Forlin Aug. 18, 2011, 9:28 a.m.
From: Per Forlin <per.forlin@linaro.org>

FSG_NUM_BUFFERS is set to 2 as default.
Usually 2 buffers are enough to establish a good buffering pipeline.
The number may be increased in order to compensate a for bursty VFS
behaviour.

Here follows a description of system that may require more than
2 buffers.
 * CPU ondemand governor active
 * latency cost for wake up and/or frequency change
 * DMA for IO

Use case description.
 * Data transfer from MMC via VFS to USB.
 * DMA shuffles data from MMC and to USB.
 * The CPU wakes up every now and then to pass data in and out from VFS,
   which cause the bursty VFS behaviour.

Test set up
 * Running dd on the host reading from the mass storage device
 * cmdline: dd if=/dev/sdb of=/dev/null bs=4k count=$((256*100))
 * Caches are dropped on the host and on the device before each run

Measurements on a Snowball board with ondemand_govenor active.

FSG_NUM_BUFFERS 2
104857600 bytes (105 MB) copied, 5.62173 s, 18.7 MB/s
104857600 bytes (105 MB) copied, 5.61811 s, 18.7 MB/s
104857600 bytes (105 MB) copied, 5.57817 s, 18.8 MB/s

FSG_NUM_BUFFERS 4
104857600 bytes (105 MB) copied, 5.26839 s, 19.9 MB/s
104857600 bytes (105 MB) copied, 5.2691 s, 19.9 MB/s
104857600 bytes (105 MB) copied, 5.2711 s, 19.9 MB/s

There may not be one optimal number for all boards. This is why
the number is added to Kconfig. If selecting USB_DEBUG this value may be
set by a module parameter as well.

Signed-off-by: Per Forlin <per.forlin@linaro.org>
---
Change log.
v2: Update after proofreading comments from Michal Nazarewicz
v3: Clarify the description of this patch based on input from Alan Stern
v4: - Introduce a module_param to set number of pipeline buffers
    if USB_DEBUG is set. In order to add this support fsg_common is
    allocated at runtime. The fsg_buffhd list size is appended to fsg_dev
    and fsg_common at runtime allocation.
    - The previous acks from Michal and Alan on v3 are not applicable
    for this version since it's a new implementation.

 drivers/usb/gadget/Kconfig          |   16 ++++++++++++++++
 drivers/usb/gadget/f_mass_storage.c |   10 +++++++---
 drivers/usb/gadget/file_storage.c   |   10 ++++++++--
 drivers/usb/gadget/mass_storage.c   |   16 +++++++++-------
 drivers/usb/gadget/storage_common.c |   20 ++++++++++++++++++--
 5 files changed, 58 insertions(+), 14 deletions(-)

Patch

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 44b6b40..37ec2ce 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -96,6 +96,22 @@  config USB_GADGET_VBUS_DRAW
 	   This value will be used except for system-specific gadget
 	   drivers that have more specific information.
 
+config USB_GADGET_STORAGE_NUM_BUFFERS
+	int "Number of storage pipeline buffers"
+	range 2 4
+	default 2
+	help
+	   Usually 2 buffers are enough to establish a good buffering
+	   pipeline. The number may be increased in order to compensate
+	   for a bursty VFS behaviour. For instance there may be cpu wake up
+	   latencies that makes the VFS to appear bursty in a system with
+	   an cpu on-demand governor. Especially if DMA is doing IO to
+	   offload the CPU. In this case the CPU will go into power
+	   save often and spin up occasionally to move data within VFS.
+	   If selecting USB_DEBUG this value may be set by a module
+	   parameter as well.
+	   If unsure, say 2.
+
 #
 # USB Peripheral Controller Support
 #
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 5b93395..3e546d9 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -363,7 +363,6 @@  struct fsg_common {
 
 	struct fsg_buffhd	*next_buffhd_to_fill;
 	struct fsg_buffhd	*next_buffhd_to_drain;
-	struct fsg_buffhd	buffhds[FSG_NUM_BUFFERS];
 
 	int			cmnd_size;
 	u8			cmnd[MAX_COMMAND_SIZE];
@@ -407,6 +406,8 @@  struct fsg_common {
 	char inquiry_string[8 + 16 + 4 + 1];
 
 	struct kref		ref;
+	/* Must be the last entry */
+	struct fsg_buffhd	buffhds[0];
 };
 
 struct fsg_config {
@@ -2728,12 +2729,15 @@  static struct fsg_common *fsg_common_init(struct fsg_common *common,
 
 	/* Allocate? */
 	if (!common) {
-		common = kzalloc(sizeof *common, GFP_KERNEL);
+		common = kzalloc(sizeof(struct fsg_common) +
+				 sizeof(struct fsg_buffhd) * FSG_NUM_BUFFERS,
+				 GFP_KERNEL);
 		if (!common)
 			return ERR_PTR(-ENOMEM);
 		common->free_storage_on_release = 1;
 	} else {
-		memset(common, 0, sizeof *common);
+		memset(common, 0, sizeof(struct fsg_common) +
+		       sizeof(struct fsg_buffhd) * FSG_NUM_BUFFERS);
 		common->free_storage_on_release = 0;
 	}
 
diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index 639e14a..21d366d 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -461,7 +461,6 @@  struct fsg_dev {
 
 	struct fsg_buffhd	*next_buffhd_to_fill;
 	struct fsg_buffhd	*next_buffhd_to_drain;
-	struct fsg_buffhd	buffhds[FSG_NUM_BUFFERS];
 
 	int			thread_wakeup_needed;
 	struct completion	thread_notifier;
@@ -488,6 +487,8 @@  struct fsg_dev {
 	unsigned int		nluns;
 	struct fsg_lun		*luns;
 	struct fsg_lun		*curlun;
+	/* Must be the last entry */
+	struct fsg_buffhd	buffhds[0];
 };
 
 typedef void (*fsg_routine_t)(struct fsg_dev *);
@@ -3587,7 +3588,9 @@  static int __init fsg_alloc(void)
 {
 	struct fsg_dev		*fsg;
 
-	fsg = kzalloc(sizeof *fsg, GFP_KERNEL);
+	fsg = kzalloc(sizeof(struct fsg_dev) +
+		      sizeof(struct fsg_buffhd) * FSG_NUM_BUFFERS, GFP_KERNEL);
+
 	if (!fsg)
 		return -ENOMEM;
 	spin_lock_init(&fsg->lock);
@@ -3605,6 +3608,9 @@  static int __init fsg_init(void)
 	int		rc;
 	struct fsg_dev	*fsg;
 
+	if (!FSG_NUM_BUFFERS_IS_VALID(FSG_NUM_BUFFERS))
+		return -EINVAL;
+
 	if ((rc = fsg_alloc()) != 0)
 		return rc;
 	fsg = the_fsg;
diff --git a/drivers/usb/gadget/mass_storage.c b/drivers/usb/gadget/mass_storage.c
index d3eb274..67c58d0 100644
--- a/drivers/usb/gadget/mass_storage.c
+++ b/drivers/usb/gadget/mass_storage.c
@@ -116,9 +116,8 @@  static int __init msg_do_config(struct usb_configuration *c)
 	static const struct fsg_operations ops = {
 		.thread_exits = msg_thread_exits,
 	};
-	static struct fsg_common common;
+	struct fsg_common *common = NULL;
 
-	struct fsg_common *retp;
 	struct fsg_config config;
 	int ret;
 
@@ -130,12 +129,12 @@  static int __init msg_do_config(struct usb_configuration *c)
 	fsg_config_from_params(&config, &mod_data);
 	config.ops = &ops;
 
-	retp = fsg_common_init(&common, c->cdev, &config);
-	if (IS_ERR(retp))
-		return PTR_ERR(retp);
+	common = fsg_common_init(common, c->cdev, &config);
+	if (IS_ERR(common))
+		return PTR_ERR(common);
 
-	ret = fsg_bind_config(c->cdev, c, &common);
-	fsg_common_put(&common);
+	ret = fsg_bind_config(c->cdev, c, common);
+	fsg_common_put(common);
 	return ret;
 }
 
@@ -179,6 +178,9 @@  MODULE_LICENSE("GPL");
 
 static int __init msg_init(void)
 {
+	if (!FSG_NUM_BUFFERS_IS_VALID(FSG_NUM_BUFFERS))
+		return -EINVAL;
+
 	return usb_composite_probe(&msg_driver, msg_bind);
 }
 module_init(msg_init);
diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
index d3dd227..197eace 100644
--- a/drivers/usb/gadget/storage_common.c
+++ b/drivers/usb/gadget/storage_common.c
@@ -262,8 +262,24 @@  static struct fsg_lun *fsg_lun_from_dev(struct device *dev)
 #define EP0_BUFSIZE	256
 #define DELAYED_STATUS	(EP0_BUFSIZE + 999)	/* An impossibly large value */
 
-/* Number of buffers we will use.  2 is enough for double-buffering */
-#define FSG_NUM_BUFFERS	2
+#ifdef CONFIG_USB_DEBUG
+
+static unsigned int fsg_num_buffers = CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS;
+module_param(fsg_num_buffers, uint, S_IRUGO);
+MODULE_PARM_DESC(fsg_num_buffers, "Number of pipeline buffers");
+#define FSG_NUM_BUFFERS	fsg_num_buffers
+
+#else
+
+/*
+ * Number of buffers we will use.
+ * 2 is usually enough for good buffering pipeline
+ */
+#define FSG_NUM_BUFFERS	CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS
+
+#endif /* CONFIG_USB_DEBUG */
+
+#define FSG_NUM_BUFFERS_IS_VALID(num) (num >= 2 && num <= 4 ? true : false)
 
 /* Default size of buffer length. */
 #define FSG_BUFLEN	((u32)16384)