[PATCH] USB: f_mass_storage: dynamic buffers for better alignment

[PATCH] USB: f_mass_storage: dynamic buffers for better alignment

[Michal Nazarewicz]

"Static" buffers in fsg_buffhd structure (ie. fields which are arrays
rather then pointers to dynamically allocated memory) are not aligned
to any "big" power of two which may lead to poor DMA performance
(copying "by hand" of head or tail) or no DMA at all even if otherwise
hardware supports it.

Therefore, this patch makes mass storage function use kmalloc()ed
buffers which are (because of their size) page aligned (which should
be enough for any hardware).

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/usb/gadget/f_mass_storage.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 84f6491..8945573 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -302,7 +302,6 @@ static const char fsg_string_interface[] = "Mass Storage";
 
 
 #define FSG_NO_INTR_EP 1
-#define FSG_BUFFHD_STATIC_BUFFER 1
 #define FSG_NO_DEVICE_STRINGS    1
 #define FSG_NO_OTG               1
 #define FSG_NO_INTR_EP           1
@@ -2747,13 +2746,18 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
 
 
 	/* Data buffers cyclic list */
-	/* Buffers in buffhds are static -- no need for additional
-	 * allocation. */
 	bh = common->buffhds;
-	i = FSG_NUM_BUFFERS - 1;
-	do {
+	i = FSG_NUM_BUFFERS;
+	for (i = FSG_NUM_BUFFERS;; ++bh) {
+		bh->buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
+		if (!bh->buf) {
+			rc = -ENOMEM;
+			goto error_release;
+		}
+		if (!--i)
+			break;
 		bh->next = bh + 1;
-	} while (++bh, --i);
+	}
 	bh->next = common->buffhds;
 
 
@@ -2861,6 +2865,7 @@ static void fsg_common_release(struct kref *ref)
 		container_of(ref, struct fsg_common, ref);
 	unsigned i = common->nluns;
 	struct fsg_lun *lun = common->luns;
+	struct fsg_buffhd *bh;
 
 	/* If the thread isn't already dead, tell it to exit now */
 	if (common->state != FSG_STATE_TERMINATED) {
@@ -2882,6 +2887,13 @@ static void fsg_common_release(struct kref *ref)
 	}
 
 	kfree(common->luns);
+
+	i = FSG_NUM_BUFFERS;
+	bh = common->buffhds;
+	do {
+		kfree(bh->buf);
+	} while (++bh, --i);
+
 	if (common->free_storage_on_release)
 		kfree(common);
 }
-- 
1.6.5

Re: [PATCH] USB: f_mass_storage: dynamic buffers for better alignment

[Felipe Balbi]

On Mon, Mar 15, 2010 at 11:09:55AM +0100, Michal Nazarewicz wrote:
> "Static" buffers in fsg_buffhd structure (ie. fields which are arrays
> rather then pointers to dynamically allocated memory) are not aligned
> to any "big" power of two which may lead to poor DMA performance

not so true as you can add __attribute__ ((aligned(32))) to those.

> @@ -2747,13 +2746,18 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
>  
>  
>  	/* Data buffers cyclic list */
> -	/* Buffers in buffhds are static -- no need for additional
> -	 * allocation. */
>  	bh = common->buffhds;
> -	i = FSG_NUM_BUFFERS - 1;
> -	do {
> +	i = FSG_NUM_BUFFERS;
> +	for (i = FSG_NUM_BUFFERS;; ++bh) {

something like

for (i = 0; i < FSG_NUM_BUFFERS; i++, ++bh) {

wouldn't it do it ??

-- 
balbi

Re: [PATCH] USB: f_mass_storage: dynamic buffers for better alignment

[Michał Nazarewicz]

> On Mon, Mar 15, 2010 at 11:09:55AM +0100, Michal Nazarewicz wrote:
>> "Static" buffers in fsg_buffhd structure (ie. fields which are arrays
>> rather then pointers to dynamically allocated memory) are not aligned
>> to any "big" power of two which may lead to poor DMA performance

On Mon, 15 Mar 2010 19:10:21 +0100, Felipe Balbi <me@felipebalbi.com> wrote:
> not so true as you can add __attribute__ ((aligned(32))) to those.

I admit, I haven't thought about that.  Some fields rearrangement
could help avoid some padding but yes, it can be done.

However, there is one more thing I've had in mind.  Each buffer
is 4 pages (16 KiB) and there are two such buffers in struct
fsg_common therefore the whole size of the structure is
9 pages (> 32 KiB).

I've been simply concerned about using kamlloc() for such big
structures so in the end decided to split it into 3 allocations.

Maybe I'm overeating though?  Or maybe vmalloc() would solve those
problems?  But then again, vmalloc() could degrade DMA performance
on systems w/o scatter-gather.

What do you think?

>>  	bh = common->buffhds;
>> -	i = FSG_NUM_BUFFERS - 1;
>> -	do {
>> +	i = FSG_NUM_BUFFERS;
>> +	for (i = FSG_NUM_BUFFERS;; ++bh) {

> something like
>
> for (i = 0; i < FSG_NUM_BUFFERS; i++, ++bh) {
>
> wouldn't it do it ??

I admit I'm a bit addicted to "downwards to zero" loops and avoiding
checking of the condition prior to the first iteration.  (As such I
often use do-while where others would use for.)

Besides counting to zero is not really an issue here.  I didn't
particularly fancy the "bh[-1]" that have to be used if the break
is not inside the loop, ie:

	bh = common->buffhds;
	rc = -ENOMEM;
	for (i = FSG_NUM_BUFFERS; i--; ++bh) {
		bh->buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
		if (unlikely(!bh->buf))
			goto error_release;
		bh->next = bh + 1;
	}
	bh[-1].next = common->buffhds;

Note also that the last bh->next is assigned twice.

So personally I'd still stick with my version but since
readability is important how about:

	bh = common->buffhds;
	rc = -ENOMEM;
	i = FSG_NUM_BUFFERS;
	for(;;) {
		bh->buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
		if (unlikely(!bh->buf))
			goto error_release;
		if (!--i)
			break;
		bh->next = bh + 1;
		++bh;
	}
	bh->next = common->buffhds;

What do you think?

-- 
Best regards,                                           _     _
  .o. | Liege of Serenely Enlightened Majesty of       o' \,=./ `o
  ..o | Computer Science,  Michał "mina86" Nazarewicz     (o o)
  ooo +---[mina86@mina86.com]---[mina86@jabber.org]---ooO--(_)--Ooo--

Re: [PATCH] USB: f_mass_storage: dynamic buffers for better alignment

[Felipe Balbi]

On Mon, Mar 15, 2010 at 08:20:08PM +0100, Micha?? Nazarewicz wrote:
> > On Mon, Mar 15, 2010 at 11:09:55AM +0100, Michal Nazarewicz wrote:
> >> "Static" buffers in fsg_buffhd structure (ie. fields which are arrays
> >> rather then pointers to dynamically allocated memory) are not aligned
> >> to any "big" power of two which may lead to poor DMA performance
> 
> On Mon, 15 Mar 2010 19:10:21 +0100, Felipe Balbi <me@felipebalbi.com> wrote:
> > not so true as you can add __attribute__ ((aligned(32))) to those.
> 
> I admit, I haven't thought about that.  Some fields rearrangement
> could help avoid some padding but yes, it can be done.
> 
> However, there is one more thing I've had in mind.  Each buffer
> is 4 pages (16 KiB) and there are two such buffers in struct
> fsg_common therefore the whole size of the structure is
> 9 pages (> 32 KiB).
> 
> I've been simply concerned about using kamlloc() for such big
> structures so in the end decided to split it into 3 allocations.
> 
> Maybe I'm overeating though?  Or maybe vmalloc() would solve those
> problems?  But then again, vmalloc() could degrade DMA performance
> on systems w/o scatter-gather.
> 
> What do you think?

I have no opinion anymore :-p

I can only think about the devices I've been working on which would be a
pain to allocate so much memory and would suffer if you use vmalloc()
too, so both would be a no-no for me :-p

> 	bh = common->buffhds;
> 	rc = -ENOMEM;
> 	i = FSG_NUM_BUFFERS;
> 	for(;;) {
> 		bh->buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
> 		if (unlikely(!bh->buf))
> 			goto error_release;
> 		if (!--i)
> 			break;
> 		bh->next = bh + 1;
> 		++bh;
> 	}
> 	bh->next = common->buffhds;
> 
> What do you think?

how about ?

for (i = FSG_NUM_BUFFER; i; i--, ++bh) {
	bh->buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
	if (!bh->buf)
		goto error_release;
}

-- 
balbi

Re: [PATCH] USB: f_mass_storage: dynamic buffers for better alignment

[Michał Nazarewicz]

> On Mon, Mar 15, 2010 at 08:20:08PM +0100, Micha?? Nazarewicz wrote:
>> Each buffer is 4 pages and there are two such buffers in struct
>> fsg_common therefore the size of the structure is 9 pages.
>>
>> I've been simply concerned about using kamlloc() for such big
>> structures so in the end decided to split it into 3 allocations.
>>
>> Maybe I'm overeating though?  Or maybe vmalloc() would solve those
>> problems?  But then again, vmalloc() could degrade DMA performance
>> on systems w/o scatter-gather.

On Mon, 15 Mar 2010 20:28:13 +0100, Felipe Balbi <me@felipebalbi.com> wrote:
> I have no opinion anymore :-p
>
> I can only think about the devices I've been working on which would be a
> pain to allocate so much memory and would suffer if you use vmalloc()
> too, so both would be a no-no for me :-p

So here it is...  I think allocating buffers dynamically via kmalloc()
is the safest way. :)

>> 	bh = common->buffhds;
>> 	rc = -ENOMEM;
>> 	i = FSG_NUM_BUFFERS;
>> 	for(;;) {
>> 		bh->buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
>> 		if (unlikely(!bh->buf))
>> 			goto error_release;
>> 		if (!--i)
>> 			break;
>> 		bh->next = bh + 1;
>> 		++bh;
>> 	}
>> 	bh->next = common->buffhds;

> how about ?
>
> for (i = FSG_NUM_BUFFER; i; i--, ++bh) {
> 	bh->buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
> 	if (!bh->buf)
> 		goto error_release;
> }

bh->next cyclic buffer needs to be initialised.  Otherwise I'd stick
with what you've posted.

-- 
Best regards,                                           _     _
  .o. | Liege of Serenely Enlightened Majesty of       o' \,=./ `o
  ..o | Computer Science,  Michał "mina86" Nazarewicz     (o o)
  ooo +---[mina86@mina86.com]---[mina86@jabber.org]---ooO--(_)--Ooo--

[PATCH] USB: f_mass_storage: dynamic buffers for better alignment

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

Using of "static" buffers in fsg_buffhd structure causes buffers
not to be aligned to any bigger power of two (like cache size for
instance) which as a result may lead to some copying of data or
slower DMA access (as head must to be copied via CPU).  So even
though code gets a bit more complicated this patch changes the
behaviour of mass storage function to use kmalloc()ed buffers which
are (because of their size) page aligned -- that should be enought
for any DMA or other hardware.
---
 drivers/usb/gadget/f_mass_storage.c |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 84f6491..eff339d 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -302,7 +302,6 @@ static const char fsg_string_interface[] = "Mass Storage";
 
 
 #define FSG_NO_INTR_EP 1
-#define FSG_BUFFHD_STATIC_BUFFER 1
 #define FSG_NO_DEVICE_STRINGS    1
 #define FSG_NO_OTG               1
 #define FSG_NO_INTR_EP           1
@@ -2747,13 +2746,19 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
 
 
 	/* Data buffers cyclic list */
-	/* Buffers in buffhds are static -- no need for additional
-	 * allocation. */
 	bh = common->buffhds;
-	i = FSG_NUM_BUFFERS - 1;
+	i = FSG_NUM_BUFFERS;
+	goto buffhds_first_it;
 	do {
 		bh->next = bh + 1;
-	} while (++bh, --i);
+		++bh;
+buffhds_first_it:
+		bh->buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
+		if (unlikely(!bh->buf)) {
+			rc = -ENOMEM;
+			goto error_release;
+		}
+	} while (--i);
 	bh->next = common->buffhds;
 
 
@@ -2861,6 +2866,7 @@ static void fsg_common_release(struct kref *ref)
 		container_of(ref, struct fsg_common, ref);
 	unsigned i = common->nluns;
 	struct fsg_lun *lun = common->luns;
+	struct fsg_buffhd *bh;
 
 	/* If the thread isn't already dead, tell it to exit now */
 	if (common->state != FSG_STATE_TERMINATED) {
@@ -2882,6 +2888,13 @@ static void fsg_common_release(struct kref *ref)
 	}
 
 	kfree(common->luns);
+
+	i = FSG_BUFFHD;
+	bh = common->buffhds;
+	do {
+		kfree(bh->buf);
+	} while (++bh, --i);
+
 	if (common->free_storage_on_release)
 		kfree(common);
 }
-- 
1.7.0

[PATCHv3] USB: f_mass_storage: dynamic buffers for better alignment

[Michal Nazarewicz]

"Static" buffers in fsg_buffhd structure (ie. fields which are arrays
rather then pointers to dynamically allocated memory) are not aligned
to any "big" power of two which may lead to poor DMA performance
(copying "by hand" of head or tail) or no DMA at all even if otherwise
hardware supports it.

Therefore, this patch makes mass storage function use kmalloc()ed
buffers which are (because of their size) page aligned (which should
be enough for any hardware).

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/usb/gadget/f_mass_storage.c |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)

 The buffhds buffers initialisation loop has been refactored.
 Hopefully it's more pleasent to look at now. ;)

 PS. Someone should flog me with a willow branch...  Sorry for the
     previous mail, it was obviously an invalid file.  I must stop
     sending patches after 9pm...

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 84f6491..8945573 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -302,7 +302,6 @@ static const char fsg_string_interface[] = "Mass Storage";
 
 
 #define FSG_NO_INTR_EP 1
-#define FSG_BUFFHD_STATIC_BUFFER 1
 #define FSG_NO_DEVICE_STRINGS    1
 #define FSG_NO_OTG               1
 #define FSG_NO_INTR_EP           1
@@ -2747,13 +2746,19 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
 
 
 	/* Data buffers cyclic list */
-	/* Buffers in buffhds are static -- no need for additional
-	 * allocation. */
 	bh = common->buffhds;
-	i = FSG_NUM_BUFFERS - 1;
+	i = FSG_NUM_BUFFERS;
+	goto buffhds_first_it;
 	do {
 		bh->next = bh + 1;
-	} while (++bh, --i);
+		++bh;
+buffhds_first_it:
+		bh->buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
+		if (unlikely(!bh->buf)) {
+			rc = -ENOMEM;
+			goto error_release;
+		}
+	} while (--i);
 	bh->next = common->buffhds;
 
 
@@ -2861,6 +2865,7 @@ static void fsg_common_release(struct kref *ref)
 		container_of(ref, struct fsg_common, ref);
 	unsigned i = common->nluns;
 	struct fsg_lun *lun = common->luns;
+	struct fsg_buffhd *bh;
 
 	/* If the thread isn't already dead, tell it to exit now */
 	if (common->state != FSG_STATE_TERMINATED) {
@@ -2882,6 +2887,13 @@ static void fsg_common_release(struct kref *ref)
 	}
 
 	kfree(common->luns);
+
+	i = FSG_NUM_BUFFERS;
+	bh = common->buffhds;
+	do {
+		kfree(bh->buf);
+	} while (++bh, --i);
+
 	if (common->free_storage_on_release)
 		kfree(common);
 }
-- 
1.7.0