Re: [PATCH 08/11] staging: et131x: simplify rx dma code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Dec 4, 2013 at 5:05 PM, Mark Einon <mark.einon@xxxxxxxxx> wrote:
> On Wed, Dec 04, 2013 at 03:24:18PM +0800, ZHAO Gang wrote:
>> The original code allocate rx dma memory in several dma_alloc_coherent calls,
>> which causes some problems:
>> 1. since dma_alloc_coherent allocate at least one page memory, it wastes some
>>    memory when allocation size is smaller than one page.
>> 2. it causes et131x_rx_dma_memory_free as complex as et131x_rx_dma_memory_alloc
>>
>> Instead, allocate all rx dma memory in one dma_alloc_coherent call makes less code,
>> makes it easy to handle dma allocation error, and makes et131x_rx_dma_memory_free
>> as simple as it could be.
>>
>> Signed-off-by: ZHAO Gang <gamerh2o@xxxxxxxxx>
>> ---
>>  drivers/staging/et131x/et131x.c | 202 ++++++++++++----------------------------
>>  1 file changed, 59 insertions(+), 143 deletions(-)
>
> Hi Zhao,
>
> I haven't had time to look at these last few patches in detail yet, but
> I do have some comments - The code is an improvement in that it reduces
> the number of lines of code, but you're also making the driver more
> susceptible to memory allocation issues, as you're asking for a single
> large block of memory and not several smaller ones. I'm not sure if that
> will have an appreciable impact yet.
>

The first motivation that I want to change the dma alloc code is to
add error check code, then I find it's not trivial to add it, since
there are several dma_alloc_coherent, some in for loop, if dma alloc
failed in the middle, code to free already allocated dma memery will
be hard to read, so I think combine these allocations to one may solve
this problem.

The worst case in dma_alloc_coherent happens when jumbo packet size >=
4096, previous code needs 64K in one allocation at most, this change
needs 85K in one allocation.

When jumbo packet size < 2048, previous code needs 12K in one
allocation at most, this change needs 29K.

When jumbo packet size is between 2048 and 4096, previous code needs
18K in one allocation at most, this change needs 48K.

Hope my calculation is right. I'm not sure if it's acceptable.

> Also, I think a better way to do this allocation would be to create a
> single struct with the items to be allocated, where as few as possible
> of the struct members are of dynamic size
> (drivers/net/ethernet/cirrus/ep93xx_eth.c is a nice example of this) -
> although this reason alone would not be enough to reject the alloc
> patches, but the previous reason might.
>

This way is actually the better way, but it needs lots of changes to
the original code. Maybe I would try it later.

> Cheers,
>
> Mark
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux