On Tue, Jan 07, 2014 at 09:29:30AM +0000, David Laight wrote:
> > From: 'David Cohen'
> > On Mon, Jan 06, 2014 at 09:26:20AM +0000, David Laight wrote:
> > > > From: David Cohen
> > > > On Fri, Dec 20, 2013 at 09:26:35AM -0000, David Laight wrote:
> > > > > > From: David Cohen
> > > > > The effect of this change is really to remove the first allocation and
> > > > > add 8 bytes (or maybe a pointer) to the start of the second one.
> > > > > So it is extremely unlikely to fail when the old code would work.
> > > >
> > > > Currently struct urb_priv has a dynamic array of pointers to struct
> > > > xhci_td. You're replacing the pointer by structs itself. Now, instead of
> > > > 2 kmallocs() (1 for urb_priv and another for size * xhci_td) we've 1
> > > > kmalloc() with urb_priv + size * xhci_td.
> > >
> > > You misread the old code.
> > > The first malloc was for the header and 'n' pointers.
> > > The second malloc was for 'n' copies of the structure.
> >
> > That's exactly what I described but in a more complicated way :)
> >
> > 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:
--- 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. See below:
/*
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1275,21 +1274,10 @@ int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags)
size = 1;
urb_priv = kzalloc(sizeof(struct urb_priv) +
- size * sizeof(struct xhci_td *), mem_flags);
+ size * sizeof(struct xhci_td), mem_flags);
if (!urb_priv)
return -ENOMEM;
"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.
>
> > Back to your patch description, you mention new kmalloc size may be
> > PAGE_SIZE + 8 bytes, which means kmalloc may have to find 2 consecutive
> > free pages. Of course it is not a big issue, but I don't see a reason to
> > unnecessarily change 2 kmalloc < PAGE_SIZE by one possibly > PAGE_SIZE.
>
> The values of 'n' are unknown. I doubt that the value that just exceeds
> a page happens any more often than those either side of it.
Knowing the regular range of values for "n" (AKA "size" in code above)
does matter in order to know if it's worth to replace 2 kmallocs by one.
SLAB/SLUB provide pretty fast way for smaller kmallocs.
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
