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

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

[Per Forlin]

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(-)

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)
-- 
1.7.4.1

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1870 bytes --]

Hi,

On Thu, Aug 18, 2011 at 11:28:46AM +0200, Per Forlin wrote:
> 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>

Alan, are you ok with the change ? I'm not sure tying the parameter to
CONFIG_USB_DEBUG is a good idea, maybe CONFIG_USB_GADGET_DEBUG or
CONFIG_USB_FILE_STORAGE_TEST is better ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Per Forlin]

On 18 August 2011 13:14, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Thu, Aug 18, 2011 at 11:28:46AM +0200, Per Forlin wrote:
>> 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>
>
> Alan, are you ok with the change ? I'm not sure tying the parameter to
> CONFIG_USB_DEBUG is a good idea, maybe CONFIG_USB_GADGET_DEBUG or
> CONFIG_USB_FILE_STORAGE_TEST is better ?
>
The reason for the module parameter is to enable runtime calibration
of the optimal num_buffers. If the DEBUG-code adds
overhead that affects the performance the calibration will be
incorrect. I haven't measured the overhead of USB_DEBUG.

Regards,
Per

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

[Michal Nazarewicz]

On Thu, 18 Aug 2011 11:28:46 +0200, Per Forlin wrote:
> 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];

I would rather see it as “struct fsg_buffhd *buffhds;” since this change
requires both mass_storage.c and multi.c to be changed.

Alternatively, revert the possibility for fsg_common_init() to take struct
fsg_common as an argument, but that would be more intrusive I think and
I would prefer the former.

>  };
> 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);

Better yet:

kzalloc(sizeof *common + FSG_NUM_BUFFERS * sizeof *(common->buffhds),
GFP_KERNEL);

but like I've said, I would prefer that buffhds is a separate array.

> 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];

IIRC, C99's way of doing it is:  “struct fsg_buffhd buffhds[];”.

>  };
> 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);
> +

Better yet:

kzalloc(sizeof *fsg + FSG_NUM_BUFFERS * sizeof *(fsg->buffhds),
GFP_KERNEL);

>  	if (!fsg)
>  		return -ENOMEM;
>  	spin_lock_init(&fsg->lock);

> 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);

The first argument should be NULL.

> +	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;
>  }


> 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

Could we get rid of the macro all together, and just stick to
“fsg_num_buffers” variable?  Upper case name suggest that it's
a compile time constant which is not true.

> +#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

Similarly, I'd change the name to lowercase as to reflect that sometimes
it may be not a compile-time constant.

> +#endif /* CONFIG_USB_DEBUG */
>
> +#define FSG_NUM_BUFFERS_IS_VALID(num) (num >= 2 && num <= 4 ? true :  
> false)

The “?:” is not needed.

#define FSG_NUM_BUFFERS_IS_VALID(num) ((num) >= 2 && (num) <= 4)

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

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

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 3125 bytes --]

On Thu, 18 Aug 2011, Per Forlin wrote:

> On 18 August 2011 13:14, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Thu, Aug 18, 2011 at 11:28:46AM +0200, Per Forlin wrote:
> >> 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>
> >
> > Alan, are you ok with the change ? I'm not sure tying the parameter to
> > CONFIG_USB_DEBUG is a good idea, maybe CONFIG_USB_GADGET_DEBUG or
> > CONFIG_USB_FILE_STORAGE_TEST is better ?
> >
> The reason for the module parameter is to enable runtime calibration
> of the optimal num_buffers. If the DEBUG-code adds
> overhead that affects the performance the calibration will be
> incorrect. I haven't measured the overhead of USB_DEBUG.

I'm basically okay with this, subject to issues that Michal brought up.  
I think the name of the module parameter should be num_buffers rather
than fsg_num_buffers (use module_param_named) -- the "fsg" prefix will
look odd when applied to the mass-storage gadget, and besides, since
it's a module parameter we already know what module it will be applied
to.  Also, the new module parameter should be documented in the
comments near the start of the source file.

Maybe USB_DEBUG isn't the best option for enabling the module 
parameter.  As far as I'm concerned, Per can change

	#ifdef CONFIG_USB_DEBUG

to

	#if 0	/* Change to #if 1 to enable testing of this parameter */

Editing the source file once and rebuilding it is not any harder than 
changing a Kconfig option and rebuilding the entire USB stack.

Alan Stern

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

[Per Forlin]

2011/8/18 Michal Nazarewicz <mina86@mina86.com>:
> On Thu, 18 Aug 2011 11:28:46 +0200, Per Forlin wrote:
>>
>> 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];
>
> I would rather see it as “struct fsg_buffhd *buffhds;” since this change
> requires both mass_storage.c and multi.c to be changed.
>
If the allocation of buffhds is done separately in fsg_common_init().
mass_storage.c and multi.c doesn't need to be changed. But it's little
tricky to know whether buffhds should be allocated or not.

if (!common->buffhds)
  common->buffhds = kzalloc()
This works fine if the common is declared static since all data is 0
by default. If common is allocated by kmalloc and then passed to
fsg_commin_init() this check isn't reliable.
memset of common will erase buffhds pointer as well. A minor issue,
storing this pointer before running memset will fix it. I would like
to propose a different approach.

+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -363,7 +363,7 @@ struct fsg_common {
-	struct fsg_buffhd	buffhds[FSG_NUM_BUFFERS];
+	struct fsg_buffhd	buffhds[CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS];

+++ b/drivers/usb/gadget/file_storage.c
@@ -461,7 +461,7 @@ struct fsg_dev {
-	struct fsg_buffhd	buffhds[FSG_NUM_BUFFERS];
+	struct fsg_buffhd	buffhds[CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS];

+++ b/drivers/usb/gadget/storage_common.c
@@ -52,6 +52,12 @@
+/*
+ * There is a num_buffers module param when USB_GADGET_DEBUG is defined.
+ * This parameter sets the length of the fsg_buffhds array.
+ * The valid range of num_buffers is:
+ * num >= 2 && num <= CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS.
+ */

+#ifdef CONFIG_USB_GADGET_DEBUG_FILES
I am in favor of #ifdef some Kconfig option. This simplifies for
automated build/tests farms where def_configs are being used to
configure the system.
This option should not affect the performance significantly.

+
+static unsigned int fsg_num_buffers = CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS;
+module_param_named(num_buffers, fsg_num_buffers, uint, S_IRUGO);
+MODULE_PARM_DESC(fsg_num_buffers, "Number of pipeline 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) <= CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS)

Keep the length of the buffhds array constant. Use a variable
fsg_num_buffers when iterating that array.
This minimize the code to change. But to the price of using
CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS to declare
and fsg_num_buffers to access.

Is this proposal better or worse?

Thanks,
Per

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

[Michal Nazarewicz]

On Fri, 19 Aug 2011 10:39:24 +0200, Per Forlin <per.forlin@linaro.org>  
wrote:

> 2011/8/18 Michal Nazarewicz <mina86@mina86.com>:
>> On Thu, 18 Aug 2011 11:28:46 +0200, Per Forlin wrote:
>>>
>>> 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];
>>
>> I would rather see it as “struct fsg_buffhd *buffhds;” since this change
>> requires both mass_storage.c and multi.c to be changed.
>>
> If the allocation of buffhds is done separately in fsg_common_init().
> mass_storage.c and multi.c doesn't need to be changed. But it's little
> tricky to know whether buffhds should be allocated or not.

They should be always allocated.  If the code allocate fsg_common itself,  
the
case is obvious.  If caller passes a pointer to fsg_common structure, it is
assumed that the structure is not initialised, thus the function need to
allocate buffers.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

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

[Per Forlin]

2011/8/19 Michal Nazarewicz <mina86@mina86.com>:
> On Fri, 19 Aug 2011 10:39:24 +0200, Per Forlin <per.forlin@linaro.org>
> wrote:
>
>> 2011/8/18 Michal Nazarewicz <mina86@mina86.com>:
>>>
>>> On Thu, 18 Aug 2011 11:28:46 +0200, Per Forlin wrote:
>>>>
>>>> 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];
>>>
>>> I would rather see it as “struct fsg_buffhd *buffhds;” since this change
>>> requires both mass_storage.c and multi.c to be changed.
>>>
>> If the allocation of buffhds is done separately in fsg_common_init().
>> mass_storage.c and multi.c doesn't need to be changed. But it's little
>> tricky to know whether buffhds should be allocated or not.
>
> They should be always allocated.  If the code allocate fsg_common itself,
> the
> case is obvious.  If caller passes a pointer to fsg_common structure, it is
> assumed that the structure is not initialised, thus the function need to
> allocate buffers.
>
Is it true that fsg_common_init() will never be called twice to
initialise the same fsg_common structure. It is always called once
followed by release.
If this is the case it is safe to only:
if (common->buffhds)
  common->buffhds = kzalloc()

Thanks,
Per

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

[Michal Nazarewicz]

On Fri, 19 Aug 2011 13:42:08 +0200, Per Forlin <per.forlin@linaro.org>  
wrote:
> Is it true that fsg_common_init() will never be called twice to
> initialise the same fsg_common structure. It is always called once
> followed by release.
> If this is the case it is safe to only:
> if (common->buffhds)
>   common->buffhds = kzalloc()

You should just always do common->buffhds = kcalloc(fsg_num_buffhds,
sizeof *common->buffhds, GFP_KERNEL) in fsg_common_init() and then
kfree() in release.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--