On Wed, Dec 4, 2013 at 8:01 PM, ZHAO Gang <gamerh2o@xxxxxxxxx> wrote: > 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. > 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 >> _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
