On Wed, Jan 08, 2014 at 09:25:42AM +0000, David Laight wrote:
> > From: 'David Cohen'
> ...
> > > > The new kmalloc is going to be "n * sizeof(struct) - n * sizeof(pointer)"
> > > > bigger. I don't know what is the usual range of values for "n", but my
> > > > experience with android devices with non-abundant memory size is that
> > > > they are sensible to kmalloc > PAGE_SIZE.
> > >
> > > It is much easier to assume that we are keeping the second malloc
> > > are getting rid of the first one. The header is only one pointer.
> >
> > The header is not only one pointer. I believe the most relevant changes
> > from your patch happen here:
>
> My memory failed me - the 8 bytes are the two integers.
>
> > --- a/drivers/usb/host/xhci.h
> > +++ b/drivers/usb/host/xhci.h
> > @@ -1363,7 +1363,7 @@ struct xhci_scratchpad {
> > struct urb_priv {
> > int length;
> > int td_cnt;
> > - struct xhci_td *td[0];
> > + struct xhci_td td[0];
> > };
> >
> > This is a dynamic array. It's not 1 pointer, it's how many you want it
> > to be...
>
> Yes, but each of the original td[] entries pointed to consecutive
> entries of the original second malloc.
> Maybe, at some point, they were each allocated separately.
>
> > "sizeof(struct urb_priv)" does not consider the dynamic array at all, then
> > you have the second value "size * sizeof(struct xhci_td *)" where "size"
> > is the number of pointers you're going to have in the dynamic array.
> > By doing your change you're increasing in the size of kmalloc in
> > size * (sizeof(struct xhci_td) - sizeof(struct xhci_td *)) bytes.
>
> Yes, but I'm removing a kmalloc of size * (sizeof (struct xhci_td)).
>
> As I keep saying, I've moved the 'length' and 'td_cnt' fields to the
> start of kmalloc that contains the actual data and completely removed
> the allocation of an array of pointers (and all the code that dereferences
> them).
Of course. I should had paid more attention to struct urb_priv content :)
>
> sizeof (struct xhci_td) is something like 32, certainly less than 64 bytes.
> So you need a moderate 'td_cnt' for the kmalloc to exceed a page size.
I actually don't know what's the regular range of 'td_cnt'. But what got my
attention was this comment from patch description:
"The only possible downside is for isochronous tranfers with 64 td
when the allocate is 8+4096 bytes (on 64bit systems) so requires
an additional page."
My experience says even if you keep the pointers in first kmalloc but
let both < PAGE_SIZE, android device will still behave better.
Br, David Cohen
>
> David
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
