diff mbox

Reducing stack frame size in stream_process_mem2s()

Message ID 1475513785-30275-1-git-send-email-rutu.shah.26@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

rutuja shah Oct. 3, 2016, 4:56 p.m. UTC 
From: Rutuja Shah <rutu.shah.26@gmail.com>

This patch allocates memory for txbuf array on the heap rather than the stack.
As a result, the stack frame size is reduced.

Signed-off-by: Rutuja Shah <rutu.shah.26@gmail.com>
---
 hw/dma/xilinx_axidma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

rutuja shah Oct. 3, 2016, 5:02 p.m. UTC | #1 
++ stefan Sorry for the typo.
Regards
Rutuja Shah


On Mon, Oct 3, 2016 at 10:26 PM,  <rutu.shah.26@gmail.com> wrote:
> From: Rutuja Shah <rutu.shah.26@gmail.com>
>
> This patch allocates memory for txbuf array on the heap rather than the stack.
> As a result, the stack frame size is reduced.
>
> Signed-off-by: Rutuja Shah <rutu.shah.26@gmail.com>
> ---
>  hw/dma/xilinx_axidma.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index b135a5f..6c63575 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -256,13 +256,14 @@ static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
>                                   StreamSlave *tx_control_dev)
>  {
>      uint32_t prev_d;
> -    unsigned char txbuf[16 * 1024];
> +    unsigned char *txbuf;
>      unsigned int txlen;
>
>      if (!stream_running(s) || stream_idle(s)) {
>          return;
>      }
>
> +    txbuf = g_malloc(16 * 1024);
>      while (1) {
>          stream_desc_load(s, s->regs[R_CURDESC]);
>
> @@ -304,6 +305,7 @@ static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
>              break;
>          }
>      }
> +    g_free(txbuf);
>  }
>
>  static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
> --
> 1.9.1
>
Paolo Bonzini Oct. 3, 2016, 8:39 p.m. UTC | #2 
On 03/10/2016 18:56, rutu.shah.26@gmail.com wrote:
> From: Rutuja Shah <rutu.shah.26@gmail.com>
> 
> This patch allocates memory for txbuf array on the heap rather than the stack.
> As a result, the stack frame size is reduced.
> 
> Signed-off-by: Rutuja Shah <rutu.shah.26@gmail.com>
> ---
>  hw/dma/xilinx_axidma.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index b135a5f..6c63575 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -256,13 +256,14 @@ static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
>                                   StreamSlave *tx_control_dev)
>  {
>      uint32_t prev_d;
> -    unsigned char txbuf[16 * 1024];
> +    unsigned char *txbuf;
>      unsigned int txlen;
>  
>      if (!stream_running(s) || stream_idle(s)) {
>          return;
>      }
>  
> +    txbuf = g_malloc(16 * 1024);

Hello, please place the allocated buffer in struct Stream (see
xilinx_axidma_realize) instead.  This function is a hot path, and it's
better to keep allocations outside it.

Thanks,

Paolo


>      while (1) {
>          stream_desc_load(s, s->regs[R_CURDESC]);
>  
> @@ -304,6 +305,7 @@ static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
>              break;
>          }
>      }
> +    g_free(txbuf);
>  }
>  
>  static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
>
Edgar E. Iglesias Oct. 3, 2016, 10:26 p.m. UTC | #3 
On Mon, Oct 03, 2016 at 10:32:40PM +0530, Rutuja Shah wrote:
> ++ stefan Sorry for the typo.
> Regards
> Rutuja Shah
> 
> 
> On Mon, Oct 3, 2016 at 10:26 PM,  <rutu.shah.26@gmail.com> wrote:
> > From: Rutuja Shah <rutu.shah.26@gmail.com>
> >
> > This patch allocates memory for txbuf array on the heap rather than the stack.
> > As a result, the stack frame size is reduced.
> >
> > Signed-off-by: Rutuja Shah <rutu.shah.26@gmail.com>
> > ---
> >  hw/dma/xilinx_axidma.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> > index b135a5f..6c63575 100644
> > --- a/hw/dma/xilinx_axidma.c
> > +++ b/hw/dma/xilinx_axidma.c
> > @@ -256,13 +256,14 @@ static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
> >                                   StreamSlave *tx_control_dev)
> >  {
> >      uint32_t prev_d;
> > -    unsigned char txbuf[16 * 1024];
> > +    unsigned char *txbuf;
> >      unsigned int txlen;
> >
> >      if (!stream_running(s) || stream_idle(s)) {
> >          return;
> >      }
> >
> > +    txbuf = g_malloc(16 * 1024);

Hi,

Two comments.

We need to move the allocation from the data-path to initialization of the DMA objects. (e.g put txbuf into the Stream struct)
We also need to fix up the use of sizeof txbuf.

Best regards,
Edgar


> >      while (1) {
> >          stream_desc_load(s, s->regs[R_CURDESC]);
> >
> > @@ -304,6 +305,7 @@ static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
> >              break;
> >          }
> >      }
> > +    g_free(txbuf);
> >  }
> >
> >  static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
> > --
> > 1.9.1
> >
rutuja shah Oct. 4, 2016, 1:23 p.m. UTC | #4 
Hi,
Allocating txbuf in struct Stream seems to be good. I can see other
pointers of struct Stream being allocated in xilinx_axidma_realize(),
but I am not able to find their deallocation point anywhere?
Regards
Rutuja Shah


On Tue, Oct 4, 2016 at 3:56 AM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Mon, Oct 03, 2016 at 10:32:40PM +0530, Rutuja Shah wrote:
>> ++ stefan Sorry for the typo.
>> Regards
>> Rutuja Shah
>>
>>
>> On Mon, Oct 3, 2016 at 10:26 PM,  <rutu.shah.26@gmail.com> wrote:
>> > From: Rutuja Shah <rutu.shah.26@gmail.com>
>> >
>> > This patch allocates memory for txbuf array on the heap rather than the stack.
>> > As a result, the stack frame size is reduced.
>> >
>> > Signed-off-by: Rutuja Shah <rutu.shah.26@gmail.com>
>> > ---
>> >  hw/dma/xilinx_axidma.c | 4 +++-
>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
>> > index b135a5f..6c63575 100644
>> > --- a/hw/dma/xilinx_axidma.c
>> > +++ b/hw/dma/xilinx_axidma.c
>> > @@ -256,13 +256,14 @@ static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
>> >                                   StreamSlave *tx_control_dev)
>> >  {
>> >      uint32_t prev_d;
>> > -    unsigned char txbuf[16 * 1024];
>> > +    unsigned char *txbuf;
>> >      unsigned int txlen;
>> >
>> >      if (!stream_running(s) || stream_idle(s)) {
>> >          return;
>> >      }
>> >
>> > +    txbuf = g_malloc(16 * 1024);
>
> Hi,
>
> Two comments.
>
> We need to move the allocation from the data-path to initialization of the DMA objects. (e.g put txbuf into the Stream struct)
> We also need to fix up the use of sizeof txbuf.
>
> Best regards,
> Edgar
>
>
>> >      while (1) {
>> >          stream_desc_load(s, s->regs[R_CURDESC]);
>> >
>> > @@ -304,6 +305,7 @@ static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
>> >              break;
>> >          }
>> >      }
>> > +    g_free(txbuf);
>> >  }
>> >
>> >  static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
>> > --
>> > 1.9.1
>> >
Edgar E. Iglesias Oct. 4, 2016, 5:19 p.m. UTC | #5 
On Tue, Oct 04, 2016 at 06:53:20PM +0530, Rutuja Shah wrote:
> Hi,
> Allocating txbuf in struct Stream seems to be good. I can see other
> pointers of struct Stream being allocated in xilinx_axidma_realize(),
> but I am not able to find their deallocation point anywhere?

Hi,

Actually, since the allocation is of fixed size you can just
use an array in the Stream struct.
Otherwise, there's an unrealize method that I think is meant
to be used for cleanup.

Best regards,
Edgar


> Regards
> Rutuja Shah
> 
> 
> On Tue, Oct 4, 2016 at 3:56 AM, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > On Mon, Oct 03, 2016 at 10:32:40PM +0530, Rutuja Shah wrote:
> >> ++ stefan Sorry for the typo.
> >> Regards
> >> Rutuja Shah
> >>
> >>
> >> On Mon, Oct 3, 2016 at 10:26 PM,  <rutu.shah.26@gmail.com> wrote:
> >> > From: Rutuja Shah <rutu.shah.26@gmail.com>
> >> >
> >> > This patch allocates memory for txbuf array on the heap rather than the stack.
> >> > As a result, the stack frame size is reduced.
> >> >
> >> > Signed-off-by: Rutuja Shah <rutu.shah.26@gmail.com>
> >> > ---
> >> >  hw/dma/xilinx_axidma.c | 4 +++-
> >> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> >> > index b135a5f..6c63575 100644
> >> > --- a/hw/dma/xilinx_axidma.c
> >> > +++ b/hw/dma/xilinx_axidma.c
> >> > @@ -256,13 +256,14 @@ static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
> >> >                                   StreamSlave *tx_control_dev)
> >> >  {
> >> >      uint32_t prev_d;
> >> > -    unsigned char txbuf[16 * 1024];
> >> > +    unsigned char *txbuf;
> >> >      unsigned int txlen;
> >> >
> >> >      if (!stream_running(s) || stream_idle(s)) {
> >> >          return;
> >> >      }
> >> >
> >> > +    txbuf = g_malloc(16 * 1024);
> >
> > Hi,
> >
> > Two comments.
> >
> > We need to move the allocation from the data-path to initialization of the DMA objects. (e.g put txbuf into the Stream struct)
> > We also need to fix up the use of sizeof txbuf.
> >
> > Best regards,
> > Edgar
> >
> >
> >> >      while (1) {
> >> >          stream_desc_load(s, s->regs[R_CURDESC]);
> >> >
> >> > @@ -304,6 +305,7 @@ static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
> >> >              break;
> >> >          }
> >> >      }
> >> > +    g_free(txbuf);
> >> >  }
> >> >
> >> >  static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
> >> > --
> >> > 1.9.1
> >> >
diff mbox

Patch

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index b135a5f..6c63575 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -256,13 +256,14 @@  static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
                                  StreamSlave *tx_control_dev)
 {
     uint32_t prev_d;
-    unsigned char txbuf[16 * 1024];
+    unsigned char *txbuf;
     unsigned int txlen;
 
     if (!stream_running(s) || stream_idle(s)) {
         return;
     }
 
+    txbuf = g_malloc(16 * 1024);
     while (1) {
         stream_desc_load(s, s->regs[R_CURDESC]);
 
@@ -304,6 +305,7 @@  static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev,
             break;
         }
     }
+    g_free(txbuf);
 }
 
 static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,