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

ZHAO Gang gamerh2o at gmail.com
Wed Dec 4 13:32:27 UTC 2013


On Wed, Dec 4, 2013 at 8:01 PM, ZHAO Gang <gamerh2o at gmail.com> wrote:
> On Wed, Dec 4, 2013 at 5:05 PM, Mark Einon <mark.einon at gmail.com> 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 at gmail.com>
>>> ---
>>>  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.
>

By re-examine the code I found the calculation is not correct. The
real impact is too big to apply this patch, so this patch and
following patches should be dropped.

>> 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
>>


More information about the devel mailing list