| Message ID | 1457420446-25276-5-git-send-email-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 8 March 2016 at 14:00, Peter Xu <peterx@redhat.com> wrote: > First of all, this function cannot be inlined even with always_inline, > so removing inline. Please don't mix two different changes in one patch. > After that, make its stack bounded. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > CC: Gerd Hoffmann <kraxel@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > hw/usb/hcd-xhci.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > index 44b6f8c..3dd5a02 100644 > --- a/hw/usb/hcd-xhci.c > +++ b/hw/usb/hcd-xhci.c > @@ -694,18 +694,22 @@ static inline void xhci_dma_read_u32s(XHCIState *xhci, dma_addr_t addr, > } > } > > -static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr, > - uint32_t *buf, size_t len) > +static void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr, > + uint32_t *buf, size_t len) > { > int i; > - uint32_t tmp[len / sizeof(uint32_t)]; > + uint32_t n = len / sizeof(uint32_t); > +#define __BUF_SIZE (12) > + uint32_t tmp[__BUF_SIZE]; > > + assert(__BUF_SIZE >= n); > assert((len % sizeof(uint32_t)) == 0); > > - for (i = 0; i < (len / sizeof(uint32_t)); i++) { > + for (i = 0; i < n; i++) { > tmp[i] = cpu_to_le32(buf[i]); > } > pci_dma_write(PCI_DEVICE(xhci), addr, tmp, len); > +#undef __BUF_SIZE All the patches in this series seem to be following the same pattern of #defining an arbitrary fixed length for the array. This does not at all seem to me to be an improvement. We should be avoiding unbounded stack allocations, but we need to do this by either changing the code to work correctly without an unbounded allocation or by using a heap allocation instead of a stack allocation. In some cases, like this one, the original code isn't even unbounded -- we always call this function with a length parameter which is a small compile time constant, so the stack usage is definitely bounded. So this change is making the code uglier and less flexible for no benefit that I can see. thanks -- PMM
On 08/03/2016 08:00, Peter Xu wrote: > First of all, this function cannot be inlined even with always_inline, > so removing inline. Why? always_inline fixes the error for me. > int i; > - uint32_t tmp[len / sizeof(uint32_t)]; > + uint32_t n = len / sizeof(uint32_t); > +#define __BUF_SIZE (12) > + uint32_t tmp[__BUF_SIZE]; > > + assert(__BUF_SIZE >= n); Instead of a #define, you can use ARRAY_SIZE(tmp). Paolo > assert((len % sizeof(uint32_t)) == 0); > > - for (i = 0; i < (len / sizeof(uint32_t)); i++) { > + for (i = 0; i < n; i++) { > tmp[i] = cpu_to_le32(buf[i]); > } > pci_dma_write(PCI_DEVICE(xhci), addr, tmp, len); > +#undef __BUF_SIZE > } > > static XHCIPort *xhci_lookup_port(XHCIState *xhci, struct USBPort *uport) >
On Tue, Mar 08, 2016 at 02:26:36PM +0700, Peter Maydell wrote: > On 8 March 2016 at 14:00, Peter Xu <peterx@redhat.com> wrote: > > First of all, this function cannot be inlined even with always_inline, > > so removing inline. > > Please don't mix two different changes in one patch. Sorry. Will follow this. > > > After that, make its stack bounded. > > > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > CC: Gerd Hoffmann <kraxel@redhat.com> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > hw/usb/hcd-xhci.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > > index 44b6f8c..3dd5a02 100644 > > --- a/hw/usb/hcd-xhci.c > > +++ b/hw/usb/hcd-xhci.c > > @@ -694,18 +694,22 @@ static inline void xhci_dma_read_u32s(XHCIState *xhci, dma_addr_t addr, > > } > > } > > > > -static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr, > > - uint32_t *buf, size_t len) > > +static void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr, > > + uint32_t *buf, size_t len) > > { > > int i; > > - uint32_t tmp[len / sizeof(uint32_t)]; > > + uint32_t n = len / sizeof(uint32_t); > > +#define __BUF_SIZE (12) > > + uint32_t tmp[__BUF_SIZE]; > > > > + assert(__BUF_SIZE >= n); > > assert((len % sizeof(uint32_t)) == 0); > > > > - for (i = 0; i < (len / sizeof(uint32_t)); i++) { > > + for (i = 0; i < n; i++) { > > tmp[i] = cpu_to_le32(buf[i]); > > } > > pci_dma_write(PCI_DEVICE(xhci), addr, tmp, len); > > +#undef __BUF_SIZE > > All the patches in this series seem to be following the > same pattern of #defining an arbitrary fixed length for > the array. This does not at all seem to me to be an > improvement. We should be avoiding unbounded stack > allocations, but we need to do this by either changing > the code to work correctly without an unbounded allocation > or by using a heap allocation instead of a stack allocation. > > In some cases, like this one, the original code isn't even > unbounded -- we always call this function with a length > parameter which is a small compile time constant, so the > stack usage is definitely bounded. So this change is making > the code uglier and less flexible for no benefit that I > can see. I was trying to avoid the "stack unlimited" warning. There will be a warning generated before applying this patch. The patch solved this. Though I'd say this patch might not be good enough. :( Then, will drop this one too if I have no further better idea. Thanks for review. Peter
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 44b6f8c..3dd5a02 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -694,18 +694,22 @@ static inline void xhci_dma_read_u32s(XHCIState *xhci, dma_addr_t addr, } } -static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr, - uint32_t *buf, size_t len) +static void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr, + uint32_t *buf, size_t len) { int i; - uint32_t tmp[len / sizeof(uint32_t)]; + uint32_t n = len / sizeof(uint32_t); +#define __BUF_SIZE (12) + uint32_t tmp[__BUF_SIZE]; + assert(__BUF_SIZE >= n); assert((len % sizeof(uint32_t)) == 0); - for (i = 0; i < (len / sizeof(uint32_t)); i++) { + for (i = 0; i < n; i++) { tmp[i] = cpu_to_le32(buf[i]); } pci_dma_write(PCI_DEVICE(xhci), addr, tmp, len); +#undef __BUF_SIZE } static XHCIPort *xhci_lookup_port(XHCIState *xhci, struct USBPort *uport)
First of all, this function cannot be inlined even with always_inline, so removing inline. After that, make its stack bounded. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> CC: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/usb/hcd-xhci.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)