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
